Message ID | s5hwozh8fxz.wl-tiwai@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi, > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: tiistai 13. helmikuuta 2018 10.14 > To: Saarinen, Jani <jani.saarinen@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; > Kumar, Abhijeet <abhijeet.kumar@intel.com> > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core > codec functions to sync power state" > > On Tue, 13 Feb 2018 08:34:40 +0100, > Saarinen, Jani wrote: > > > > HI, > > > -----Original Message----- > > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On > > > Behalf Of Takashi Iwai > > > Sent: tiistai 13. helmikuuta 2018 7.11 > > > To: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: intel-gfx@lists.freedesktop.org; Kumar, Abhijeet > > > <abhijeet.kumar@intel.com> > > > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use > > > of core codec functions to sync power state" > > > > > > On Mon, 12 Feb 2018 18:29:53 +0100, > > > Chris Wilson wrote: > > > > > > > > This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2. > > > > > > > > Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions > > > > to sync power state") > > > > Cc: Abhijeet Kumar <abhijeet.kumar@intel.com> > > > > Cc: Takashi Iwai <tiwai@suse.de> > > > > > > Did the patch break anything? > > > I don't understand it without any real context... > > Yes. See resutls from link what is fixed by reverting: > > https://patchwork.freedesktop.org/series/38097/ > > Test pm_rpm: > > Subgroup basic-pci-d3-state: > > fail -> PASS (fi-hsw-4770) > > fail -> PASS (fi-bdw-5557u) > > Subgroup basic-rte: > > fail -> PASS (fi-hsw-4770) > > fail -> PASS (fi-bdw-5557u) > > Could you investigate why does the revert fix? Maybe you instead. > Two functions are almost identical. The difference is that the new one has a > counter and quit at 500 iteration, and it has a msleep(200) at the error exit. But > both shouldn't matter for the normal operation... > > FWIW, below is the patch to make the new function identical with the original > function. Does it change the behavior? > Just send it for ml so that CI can test. > > thanks, > > Takashi Br, Jani > > > --- a/sound/hda/hdac_device.c > +++ b/sound/hda/hdac_device.c > @@ -1079,15 +1079,13 @@ unsigned int snd_hdac_sync_power_state(struct > hdac_device *codec, > hda_nid_t nid, unsigned int power_state) { > unsigned long end_time = jiffies + msecs_to_jiffies(500); > - unsigned int state, actual_state, count; > + unsigned int state, actual_state; > > - for (count = 0; count < 500; count++) { > + for (;;) { > state = snd_hdac_codec_read(codec, nid, 0, > AC_VERB_GET_POWER_STATE, 0); > - if (state & AC_PWRST_ERROR) { > - msleep(20); > + if (state & AC_PWRST_ERROR) > break; > - } > actual_state = (state >> 4) & 0x0f; > if (actual_state == power_state) > break;
On 2/13/2018 1:44 PM, Takashi Iwai wrote: > On Tue, 13 Feb 2018 08:34:40 +0100, > Saarinen, Jani wrote: >> HI, >>> -----Original Message----- >>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >>> Takashi Iwai >>> Sent: tiistai 13. helmikuuta 2018 7.11 >>> To: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: intel-gfx@lists.freedesktop.org; Kumar, Abhijeet >>> <abhijeet.kumar@intel.com> >>> Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core >>> codec functions to sync power state" >>> >>> On Mon, 12 Feb 2018 18:29:53 +0100, >>> Chris Wilson wrote: >>>> This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2. >>>> >>>> Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to >>>> sync power state") >>>> Cc: Abhijeet Kumar <abhijeet.kumar@intel.com> >>>> Cc: Takashi Iwai <tiwai@suse.de> >>> Did the patch break anything? >>> I don't understand it without any real context... >> Yes. See resutls from link what is fixed by reverting: >> https://patchwork.freedesktop.org/series/38097/ >> Test pm_rpm: >> Subgroup basic-pci-d3-state: >> fail -> PASS (fi-hsw-4770) >> fail -> PASS (fi-bdw-5557u) >> Subgroup basic-rte: >> fail -> PASS (fi-hsw-4770) >> fail -> PASS (fi-bdw-5557u) > Could you investigate why does the revert fix? > Two functions are almost identical. The difference is that the new > one has a counter and quit at 500 iteration, and it has a msleep(200) > at the error exit. But both shouldn't matter for the normal > operation... > > FWIW, below is the patch to make the new function identical with the > original function. Does it change the behavior? > > > thanks, > > Takashi > > > --- a/sound/hda/hdac_device.c > +++ b/sound/hda/hdac_device.c > @@ -1079,15 +1079,13 @@ unsigned int snd_hdac_sync_power_state(struct hdac_device *codec, > hda_nid_t nid, unsigned int power_state) > { > unsigned long end_time = jiffies + msecs_to_jiffies(500); > - unsigned int state, actual_state, count; > + unsigned int state, actual_state; > > - for (count = 0; count < 500; count++) { > + for (;;) { I believe changing it back to finite should fix the regression. > state = snd_hdac_codec_read(codec, nid, 0, > AC_VERB_GET_POWER_STATE, 0); > - if (state & AC_PWRST_ERROR) { > - msleep(20); When device raises the error state bit, we might endup sending two verbs in back to back manner.I would still recommend to have a 20ms sleep in-order to let the device clean up it's internal state. > + if (state & AC_PWRST_ERROR) > break; > - } > actual_state = (state >> 4) & 0x0f; > if (actual_state == power_state) > break;
On Tue, 13 Feb 2018 09:26:32 +0100, Saarinen, Jani wrote: > > Hi, > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: tiistai 13. helmikuuta 2018 10.14 > > To: Saarinen, Jani <jani.saarinen@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; > > Kumar, Abhijeet <abhijeet.kumar@intel.com> > > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core > > codec functions to sync power state" > > > > On Tue, 13 Feb 2018 08:34:40 +0100, > > Saarinen, Jani wrote: > > > > > > HI, > > > > -----Original Message----- > > > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On > > > > Behalf Of Takashi Iwai > > > > Sent: tiistai 13. helmikuuta 2018 7.11 > > > > To: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: intel-gfx@lists.freedesktop.org; Kumar, Abhijeet > > > > <abhijeet.kumar@intel.com> > > > > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use > > > > of core codec functions to sync power state" > > > > > > > > On Mon, 12 Feb 2018 18:29:53 +0100, > > > > Chris Wilson wrote: > > > > > > > > > > This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2. > > > > > > > > > > Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions > > > > > to sync power state") > > > > > Cc: Abhijeet Kumar <abhijeet.kumar@intel.com> > > > > > Cc: Takashi Iwai <tiwai@suse.de> > > > > > > > > Did the patch break anything? > > > > I don't understand it without any real context... > > > Yes. See resutls from link what is fixed by reverting: > > > https://patchwork.freedesktop.org/series/38097/ > > > Test pm_rpm: > > > Subgroup basic-pci-d3-state: > > > fail -> PASS (fi-hsw-4770) > > > fail -> PASS (fi-bdw-5557u) > > > Subgroup basic-rte: > > > fail -> PASS (fi-hsw-4770) > > > fail -> PASS (fi-bdw-5557u) > > > > Could you investigate why does the revert fix? > Maybe you instead. Once when a proper bug report is sent to us upstream :) > > Two functions are almost identical. The difference is that the new one has a > > counter and quit at 500 iteration, and it has a msleep(200) at the error exit. But > > both shouldn't matter for the normal operation... > > > > FWIW, below is the patch to make the new function identical with the original > > function. Does it change the behavior? > > > Just send it for ml so that CI can test. I guess Abhijeet can manage it better. It's Intel stuff, after all. Takashi
On 2/13/2018 2:00 PM, Takashi Iwai wrote: > On Tue, 13 Feb 2018 09:26:32 +0100, > Saarinen, Jani wrote: >> Hi, >>> -----Original Message----- >>> From: Takashi Iwai [mailto:tiwai@suse.de] >>> Sent: tiistai 13. helmikuuta 2018 10.14 >>> To: Saarinen, Jani <jani.saarinen@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; >>> Kumar, Abhijeet <abhijeet.kumar@intel.com> >>> Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core >>> codec functions to sync power state" >>> >>> On Tue, 13 Feb 2018 08:34:40 +0100, >>> Saarinen, Jani wrote: >>>> HI, >>>>> -----Original Message----- >>>>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On >>>>> Behalf Of Takashi Iwai >>>>> Sent: tiistai 13. helmikuuta 2018 7.11 >>>>> To: Chris Wilson <chris@chris-wilson.co.uk> >>>>> Cc: intel-gfx@lists.freedesktop.org; Kumar, Abhijeet >>>>> <abhijeet.kumar@intel.com> >>>>> Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use >>>>> of core codec functions to sync power state" >>>>> >>>>> On Mon, 12 Feb 2018 18:29:53 +0100, >>>>> Chris Wilson wrote: >>>>>> This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2. >>>>>> >>>>>> Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions >>>>>> to sync power state") >>>>>> Cc: Abhijeet Kumar <abhijeet.kumar@intel.com> >>>>>> Cc: Takashi Iwai <tiwai@suse.de> >>>>> Did the patch break anything? >>>>> I don't understand it without any real context... >>>> Yes. See resutls from link what is fixed by reverting: >>>> https://patchwork.freedesktop.org/series/38097/ >>>> Test pm_rpm: >>>> Subgroup basic-pci-d3-state: >>>> fail -> PASS (fi-hsw-4770) >>>> fail -> PASS (fi-bdw-5557u) >>>> Subgroup basic-rte: >>>> fail -> PASS (fi-hsw-4770) >>>> fail -> PASS (fi-bdw-5557u) >>> Could you investigate why does the revert fix? >> Maybe you instead. > Once when a proper bug report is sent to us upstream :) > >>> Two functions are almost identical. The difference is that the new one has a >>> counter and quit at 500 iteration, and it has a msleep(200) at the error exit. But >>> both shouldn't matter for the normal operation... >>> >>> FWIW, below is the patch to make the new function identical with the original >>> function. Does it change the behavior? >>> >> Just send it for ml so that CI can test. > I guess Abhijeet can manage it better. It's Intel stuff, after all. I would make those changes and share it to ml. > > Takashi
Quoting Takashi Iwai (2018-02-13 08:30:52) > On Tue, 13 Feb 2018 09:26:32 +0100, > Saarinen, Jani wrote: > > > > Hi, > > > -----Original Message----- > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > Sent: tiistai 13. helmikuuta 2018 10.14 > > > To: Saarinen, Jani <jani.saarinen@intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; > > > Kumar, Abhijeet <abhijeet.kumar@intel.com> > > > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core > > > codec functions to sync power state" > > > > > > On Tue, 13 Feb 2018 08:34:40 +0100, > > > Saarinen, Jani wrote: > > > > > > > > HI, > > > > > -----Original Message----- > > > > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On > > > > > Behalf Of Takashi Iwai > > > > > Sent: tiistai 13. helmikuuta 2018 7.11 > > > > > To: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Cc: intel-gfx@lists.freedesktop.org; Kumar, Abhijeet > > > > > <abhijeet.kumar@intel.com> > > > > > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use > > > > > of core codec functions to sync power state" > > > > > > > > > > On Mon, 12 Feb 2018 18:29:53 +0100, > > > > > Chris Wilson wrote: > > > > > > > > > > > > This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2. > > > > > > > > > > > > Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions > > > > > > to sync power state") > > > > > > Cc: Abhijeet Kumar <abhijeet.kumar@intel.com> > > > > > > Cc: Takashi Iwai <tiwai@suse.de> > > > > > > > > > > Did the patch break anything? > > > > > I don't understand it without any real context... > > > > Yes. See resutls from link what is fixed by reverting: > > > > https://patchwork.freedesktop.org/series/38097/ > > > > Test pm_rpm: > > > > Subgroup basic-pci-d3-state: > > > > fail -> PASS (fi-hsw-4770) > > > > fail -> PASS (fi-bdw-5557u) > > > > Subgroup basic-rte: > > > > fail -> PASS (fi-hsw-4770) > > > > fail -> PASS (fi-bdw-5557u) > > > > > > Could you investigate why does the revert fix? > > Maybe you instead. > > Once when a proper bug report is sent to us upstream :) Now there's a challenge for the dim masters, make dim fixes provide the right link. As it was it only gave the authors' email, and I thought the heads up was appropriate. -Chris
--- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -1079,15 +1079,13 @@ unsigned int snd_hdac_sync_power_state(struct hdac_device *codec, hda_nid_t nid, unsigned int power_state) { unsigned long end_time = jiffies + msecs_to_jiffies(500); - unsigned int state, actual_state, count; + unsigned int state, actual_state; - for (count = 0; count < 500; count++) { + for (;;) { state = snd_hdac_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0); - if (state & AC_PWRST_ERROR) { - msleep(20); + if (state & AC_PWRST_ERROR) break; - } actual_state = (state >> 4) & 0x0f; if (actual_state == power_state) break;