| Message ID | 20241008122658.9549-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ALSA: hda: tas2781: Fix missing setup at runtime PM | expand |
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
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
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
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
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;
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(-)