RE: [PATCH] ASoC: rt1320-sdw: don't poll PDE state on power-down

["Shuming [范書銘]" <shumingf@realtek.com>]

> >>>>>>> rt1320_pde23_event(PRE_PMD) writes REQ_POWER_STATE=PS3
> then
> >>>>>>> polls ACTUAL_POWER_STATE. DAPM fires PRE_PMD while the
> SoundWire
> >>>>>>> data port is still streaming — the codec cannot reach PS3 until
> >>>>>>> the port stops, so the poll always times out during playback.
> >>>>>>> This blocks
> >>>>>>> snd_ctl_elem_write() for 2-3s making mute unresponsive.
> >>>>>>>
> >>>>>>> Remove the poll from PRE_PMD in rt1320_pde23_event() and
> >>>>>>> rt1320_pde11_event(). The codec transitions to PS3 on its own
> >>>>>>> once the data port becomes inactive. Keep the POST_PMU poll — on
> >>>>>>> power-up the domain must reach PS0 before audio can flow.
> >>>>>>>
> >>>>>>> Fixes: f465d10cd731 ("ASoC: rt1320: Add support for version C")
> >>>>>>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> >>>>>>> ---
> >>>>>>>  sound/soc/codecs/rt1320-sdw.c | 2 --
> >>>>>>>  1 file changed, 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/sound/soc/codecs/rt1320-sdw.c
> >>>>>>> b/sound/soc/codecs/rt1320-sdw.c index
> >>>>>>> 192faa431b5e9..e42e8b6de853e 100644
> >>>>>>> --- a/sound/soc/codecs/rt1320-sdw.c
> >>>>>>> +++ b/sound/soc/codecs/rt1320-sdw.c
> >>>>>>> @@ -2000,7 +2000,6 @@ static int rt1320_pde11_event(struct
> snd_soc_dapm_widget *w,
> >>>>>>>               regmap_write(rt1320->regmap,
> >>>>>>>                       SDW_SDCA_CTL(FUNC_NUM_MIC,
> RT1320_SDCA_ENT_PDE11,
> >>>>>>>
> RT1320_SDCA_CTL_REQ_POWER_STATE, 0), ps3);
> >>>>>>> -             rt1320_pde_transition_delay(rt1320,
> FUNC_NUM_MIC, RT1320_SDCA_ENT_PDE11, ps3);
> >>>>>>>               break;
> >>>>>>>       default:
> >>>>>>>               break;
> >>>>>>> @@ -2028,7 +2027,6 @@ static int rt1320_pde23_event(struct
> snd_soc_dapm_widget *w,
> >>>>>>>               regmap_write(rt1320->regmap,
> >>>>>>>                       SDW_SDCA_CTL(FUNC_NUM_AMP,
> RT1320_SDCA_ENT_PDE23,
> >>>>>>>
> RT1320_SDCA_CTL_REQ_POWER_STATE, 0), ps3);
> >>>>>>> -             rt1320_pde_transition_delay(rt1320,
> FUNC_NUM_AMP, RT1320_SDCA_ENT_PDE23, ps3);
> >>>>>>
> >>>>>> no, sorry, that's clearly wrong.
> >>>>>>
> >>>>>> When we touch the requested power state, we *shall* wait for the
> power state to be reached.
> >>>>>> removing the transition delay is silly.
> >>>>>>
> >>>>>> You would really need to find out why streaming still occurs when you
> get a DAPM event. That doesn't seem right in the first place.
> >>>>>>
> >>>>>
> >>>>>  The stream is active by design — this is a mute, not a stream close.
> >>>>>
> >>>>> To reproduce:
> >>>>>
> >>>>>   1. Play audio through rt1320 speaker (aplay or PipeWire)
> >>>>>   2. Press speaker mute key (or amixer cset on both FU21 channels)
> >>>>>   3. Second channel mute takes 2-3s to return
> >>>>>
> >>>>> Muting both channels removes the last DAPM path consumer of PDE23.
> >>>>> DAPM fires PRE_PMD while PipeWire still holds the PCM open —
> >>>>> DP1 keeps streaming zeros. The codec cannot reach PS3 while its
> >>>>> data port is active, so the poll always times out. The mute
> >>>>> register write that actually silences the speaker happens after this
> useless wait.
> >>>>>
> >>>>> I confirmed via SoundWire debugfs reads: ACTUAL_POWER_STATE
> stays
> >>>>> PS0 while the port streams and only transitions to PS3 after
> >>>>> PipeWire calls hw_free (~3s later). The poll can never succeed in this
> path.
> >>>>>
> >>>>> Every other Realtek SDCA amp driver does fire-and-forget on
> >>>>> PRE_PMD with no transition delay — rt712_sdca_pde23_event(),
> >>>>> rt721_sdca_pde41_event(), rt1316_pde24_event(), and
> >>>>> rt1017_sdca_pde23_event() all just write PS3 and return. rt1320 is the
> only one that polls on power-down.
> >>>>>
> >>>>> The POST_PMU poll is kept — on power-up the domain must reach PS0
> >>>>> before audio can flow, and the port is not yet active at that
> >>>>> point so the transition succeeds immediately.
> >>>>
> >>>> well that's an interesting issue. IMHO all those drivers are broken then...
> >>>> A mute should let the ports active and not start any power transitions on
> the codec side.
> >>>> That's what should be fixed first.
> >>>>
> >>>> I am not sure either why PipeWire would send a hw_free on a mute
> either, that's a sure what to have audible delays and glitches. Mute != Stop.
> >>>>
> >>>
> >>> hw_free is not involved here. The mute is purely at the codec's FU
> >>> level (FU_MUTE=1 silences the output) — the PCM stream remains open
> >>> and
> >>> DP1 stays active. SoundWire debugfs confirms: after mute during
> >>> playback, REQ_POWER_STATE=PS3 but ACTUAL_POWER_STATE remains
> PS0.
> >>> The codec cannot transition while the port is streaming. As long as
> >>> the application keeps playing, this state persists indefinitely.
> >>>
> >>> The power transition is not triggered by PipeWire either. It is
> >>> standard DAPM behavior: muting both FU21 channels removes the last
> >>> active path consumer of PDE23, so DAPM evaluates the widget as
> >>> unused and fires PRE_PMD. This is how every rt7xx/rt13xx driver's DAPM
> graph works today.
> >>>
> >>> The poll itself is broken in this path: it cannot succeed while the
> >>> port is active, always times out, and the handler continues anyway.
> >>> Removing it fixes the user-facing 2-3s mute latency without changing
> >>> any DAPM topology or power semantics — the REQ_POWER_STATE=PS3
> write
> >>> is still issued, and the codec transitions once the port stops.
> >>
> >> well we're talking past each other. The only thing we agree on is that going
> to PS3 while audio is still streaming is really bad.
> >>
> >> To repeat myself, there is *no reason* why a mute should trigger a
> power-down.
> >> I am not expert enough in DAPM but that isn't a desired behavior.
> >>
> >> Don't fix this unwanted behavior with a change in how the power state
> transition is implemented, fix the unwanted behavior instead.
> >>
> >>
> >
> > The current rt1320 behavior is consistent with how this driver models
> > the playback path in DAPM.
> >
> > DAPM is documented to make power decisions from stream activity and
> > mixer settings, with those decisions derived from the routing graph
> > (`Documentation/sound/soc/dapm.rst`).
> >
> > In rt1320, the speaker mute controls are
> `SOC_DAPM_SINGLE_AUTODISABLE`
> > switches on the two `FU21` mute bits, and the playback path is `DP1RX
> > -> FU21 -> OT23 L/R -> SPOL/SPOR`, with `FU21` also depending on `PDE
> > 23`.
> >
> > With that graph, muting both speaker switches removes the active
> > speaker path, so `PDE 23` receiving `PRE_PMD` is consistent with the
> > current DAPM model of this driver.
> >
> > Given that, I think the immediate bug is the synchronous wait in
> > `rt1320_pde23_event(PRE_PMD)`, not the fact that DAPM attempts to
> > power down the now-unused speaker supply path.
> 
> My point is that the DAPM model seems wrong. Muting should not have any
> impacts on power management.
> 
> For some reason the RT1320 driver differs from previous codec drivers:
> 
> static const struct snd_kcontrol_new rt1320_spk_l_dac =
>         SOC_DAPM_SINGLE_AUTODISABLE("Switch",
>                 SDW_SDCA_CTL(FUNC_NUM_AMP,
> RT1320_SDCA_ENT_FU21, RT1320_SDCA_CTL_FU_MUTE, CH_01),
>                 0, 1, 1);
> 
> static const struct snd_kcontrol_new rt1318_sto_dac =
>         SOC_DAPM_DOUBLE_R("Switch",
>                 SDW_SDCA_CTL(FUNC_NUM_SMART_AMP,
> RT1318_SDCA_ENT_FU21, RT1318_SDCA_CTL_FU_MUTE, CH_L),
>                 SDW_SDCA_CTL(FUNC_NUM_SMART_AMP,
> RT1318_SDCA_ENT_FU21, RT1318_SDCA_CTL_FU_MUTE, CH_R),
>                 0, 1, 1);
> 
> And the main question is "why"?
> There's been no change in the topology definition, FU21 plays exactly the
> same role but is handled differently.
> 
> Maybe there are separate DACs for left and right, but that doesn't mean we
> want muting to impact PDE management.
> If you look at the helper that is being introduced, no one has any plans to
> remove the synchronous wait after changing power states.
> 
> Shuming, can you please chime in?
> 

Hi Aaron,

Sorry, I don't have a clear understanding of the root cause at the moment.
Could you let me know which project this issue was encountered in?
Also, could you please contact Realtek's PM? 
We can involve the owner of this project to help reproduce and investigate the issue on the Realtek side.