Re: [PATCH 1/2] ALSA: hda: tas2781: Fix missing setup at runtime PM

[Takashi Iwai <tiwai@xxxxxxx>]

On Wed, 09 Oct 2024 17:34:48 +0200,
Gergo Koteles wrote:
> 
> On Wed, 2024-10-09 at 16:55 +0200, Takashi Iwai wrote:
> > aOn Wed, 09 Oct 2024 16:32:52 +0200,
> > Gergo Koteles wrote:
> > > 
> > > Hi Takashi,
> > > 
> > > On Tue, 2024-10-08 at 14:26 +0200, Takashi Iwai wrote:
> > > > tas2781_runtime_suspend() clears the playback_started flag, hence it
> > > > leads to inconsistent state after runtime suspend is triggered, as if
> > > > the device hasn't been opened yet.  Also, the counterpart,
> > > > alc2781_runtime_resume() doesn't call tasdevice_tuning_switch(), and
> > > > this also causes the inconsistency.
> > > > 
> > > > This patch corrects the superfluous flag clearance and the missing
> > > > call.
> > > > 
> > > > Fixes: 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")
> > > > Link: https://bugzilla.suse.com/show_bug.cgi?id=1230132
> > > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> > > > ---
> > > >  sound/pci/hda/tas2781_hda_i2c.c | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
> > > > index 370d847517f9..6d173b721fd0 100644
> > > > --- a/sound/pci/hda/tas2781_hda_i2c.c
> > > > +++ b/sound/pci/hda/tas2781_hda_i2c.c
> > > > @@ -864,10 +864,8 @@ static int tas2781_runtime_suspend(struct device *dev)
> > > >  	/* The driver powers up the amplifiers at module load time.
> > > >  	 * Stop the playback if it's unused.
> > > >  	 */
> > > > -	if (tas_hda->priv->playback_started) {
> > > > +	if (tas_hda->priv->playback_started)
> > > >  		tasdevice_tuning_switch(tas_hda->priv, 1);
> > > > -		tas_hda->priv->playback_started = false;
> > > > -	}
> > > >  
> > > 
> > > This if-branch were added because I wanted to power up the amplifier at
> > > module load time in tasdev_fw_ready() with:
> > > 
> > >  tasdevice_tuning_switch(tas_hda->priv, 0);
> > >  tas_hda->priv->playback_started = true;
> > > 
> > > but I didn't want it to stay powered forever, if there aren't any 
> > > tas2781_hda_playback_hook() call with HDA_GEN_PCM_ACT_CLOSE action.
> > > 
> > > This seemed like a good idea at the time because the music didn't have
> > > to be restarted (for HDA_GEN_PCM_ACT_OPEN action) when the module was
> > > reloaded. But this is a rare use case, today I don't think it's
> > > important to have it in there.
> > > 
> > > In general cases, tas2781_hda_playback_hook() turns the amplifier on
> > > and off, and sets the playback_started flag.
> > > If I understand correctly, the playback_started will be off at the time
> > > of runtime_suspend()/runtime_resume() calls.
> > > 
> > > So I think the mentioned two lines from the tasdev_fw_ready() function
> > > and the mentioned if-branch can be deleted.
> > 
> > Ah, right, there is a tweak of runtime PM there, which makes harder to
> > track the state change...
> > Can we just drop the amp in tasdev_fw_ready()? 
> > 
> 
> I think, yes. I tested it with TAS2563 (Lenovo Yoga 7 14ARB7 Laptop).
> 
> > Then the whole runtime
> > suspend can be dropped.  And, the conditional amp off/on can be moved
> > again back to the system suspend/resume.  That makes less confusing.
> > 
> 
> The runtime_resume() checks whether "Speaker Program Id" kcontrol (as
> cur_prog variable) has changed with tasdevice_prmg_load() function. If
> so, updates it, so I don't think it can be dropped easily without
> moving this functionality somewhere else.

Yes, I meant to drop only runtime suspend, too.

> I'm not sure if the calibration data needs to be updated before each
> power-up or only after a program change.

That leads to another issue: it seems that the speaker program id
change doesn't influence on the behavior immediately, and it appears
only after the runtime resume.  It's a bit tricky, and better to be
documented somewhere.


Takashi