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

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

[Takashi Iwai]

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@suse.de>
---
 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;
-	}
 
 	mutex_unlock(&tas_hda->priv->codec_lock);
 
@@ -889,6 +887,9 @@ static int tas2781_runtime_resume(struct device *dev)
 	 */
 	tasdevice_apply_calibration(tas_hda->priv);
 
+	if (tas_hda->priv->playback_started)
+		tasdevice_tuning_switch(tas_hda->priv, 0);
+
 	mutex_unlock(&tas_hda->priv->codec_lock);
 
 	return 0;
-- 
2.43.0

[PATCH 2/2] ALSA: hda: tas2781: Simplify system PM with runtime PM

[Takashi Iwai]

Now most of the calls in both system and runtime PMs are identical,
use pm_runtime_force_suspend() and *_resume() for simplifying the
code.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/tas2781_hda_i2c.c | 27 ++-------------------------
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index 6d173b721fd0..c35c676202b7 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -901,19 +901,7 @@ static int tas2781_system_suspend(struct device *dev)
 
 	dev_dbg(tas_hda->priv->dev, "System Suspend\n");
 
-	mutex_lock(&tas_hda->priv->codec_lock);
-
-	/* Shutdown chip before system suspend */
-	if (tas_hda->priv->playback_started)
-		tasdevice_tuning_switch(tas_hda->priv, 1);
-
-	mutex_unlock(&tas_hda->priv->codec_lock);
-
-	/*
-	 * Reset GPIO may be shared, so cannot reset here.
-	 * However beyond this point, amps may be powered down.
-	 */
-	return 0;
+	return pm_runtime_force_suspend(dev);
 }
 
 static int tas2781_system_resume(struct device *dev)
@@ -924,26 +912,15 @@ static int tas2781_system_resume(struct device *dev)
 	dev_dbg(tas_hda->priv->dev, "System Resume\n");
 
 	mutex_lock(&tas_hda->priv->codec_lock);
-
 	for (i = 0; i < tas_hda->priv->ndev; i++) {
 		tas_hda->priv->tasdevice[i].cur_book = -1;
 		tas_hda->priv->tasdevice[i].cur_prog = -1;
 		tas_hda->priv->tasdevice[i].cur_conf = -1;
 	}
 	tasdevice_reset(tas_hda->priv);
-	tasdevice_prmg_load(tas_hda->priv, tas_hda->priv->cur_prog);
-
-	/* If calibrated data occurs error, dsp will still work with default
-	 * calibrated data inside algo.
-	 */
-	tasdevice_apply_calibration(tas_hda->priv);
-
-	if (tas_hda->priv->playback_started)
-		tasdevice_tuning_switch(tas_hda->priv, 0);
-
 	mutex_unlock(&tas_hda->priv->codec_lock);
 
-	return 0;
+	return pm_runtime_force_resume(dev);
 }
 
 static const struct dev_pm_ops tas2781_hda_pm_ops = {
-- 
2.43.0

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

[Gergo Koteles]

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@suse.de>
> ---
>  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.


Thanks,
Gergo Koteles

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

[Takashi Iwai]

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@suse.de>
> > ---
> >  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()?  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.

In anyway, it seems that my patches aren't really correct.  Let's
discard them at first.


thanks,

Takashi

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

[Gergo Koteles]

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@suse.de>
> > > ---
> > >  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.

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


Thanks,
Gergo Koteles

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

[Takashi Iwai]

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@suse.de>
> > > > ---
> > > >  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