| Message ID | 20200414101405.6992-1-hui.wang@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda: enable the runtime_pm for non-vgaswitcheroo hda controllers | expand |
On Tue, 14 Apr 2020 12:14:05 +0200, Hui Wang wrote: > > Before the pci_driver->probe() is called, the pci subsystem calls > runtime_forbib() and runtime_get_sync() on this pci dev, so only call > runtime_put_autosuspend() is not enough to enable the runtime_pm on > this device. > > For controllers with vgaswitcheroo feature, the pci/quirks.c will call > runtime_allow() for this dev, then the controllers could enter > rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like > Intel hda controllers, the runtime_pm is not enabled even it calls > put_autosuspend(). Need to call runtime_allow() for those controllers > in the hda driver. > > Signed-off-by: Hui Wang <hui.wang@canonical.com> Was this behavior changed from the earlier kernels? I thought this was left untouched because it's supposed to be set via udev rules or such. OTOH, enabling the runtime PM is almost mandatory for modern systems, and I'm fine to apply this kind of forcible enablement. thanks, Takashi > --- > sound/pci/hda/hda_intel.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index 8519051a426e..779705bef88b 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) > > if (azx_has_pm_runtime(chip)) { > pm_runtime_use_autosuspend(&pci->dev); > + if (!use_vga_switcheroo(chip)) > + pm_runtime_allow(&pci->dev); > pm_runtime_put_autosuspend(&pci->dev); > } > > -- > 2.17.1 >
On 2020/4/14 下午6:27, Takashi Iwai wrote: > On Tue, 14 Apr 2020 12:14:05 +0200, > Hui Wang wrote: >> Before the pci_driver->probe() is called, the pci subsystem calls >> runtime_forbib() and runtime_get_sync() on this pci dev, so only call >> runtime_put_autosuspend() is not enough to enable the runtime_pm on >> this device. >> >> For controllers with vgaswitcheroo feature, the pci/quirks.c will call >> runtime_allow() for this dev, then the controllers could enter >> rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like >> Intel hda controllers, the runtime_pm is not enabled even it calls >> put_autosuspend(). Need to call runtime_allow() for those controllers >> in the hda driver. >> >> Signed-off-by: Hui Wang <hui.wang@canonical.com> > Was this behavior changed from the earlier kernels? I thought this > was left untouched because it's supposed to be set via udev rules or > such. Oh, I don't know that, according to my test with ubuntu rootfs, the runtime pm is not enabled on Intel's hda controllers. But with the sof driver, the controller driver calls runtime_allow() (soc/sof/sof-pci-dev.c), so I sent this patch. Regards, Hui. > OTOH, enabling the runtime PM is almost mandatory for modern systems, > and I'm fine to apply this kind of forcible enablement. > > > thanks, > > Takashi > > >> --- >> sound/pci/hda/hda_intel.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c >> index 8519051a426e..779705bef88b 100644 >> --- a/sound/pci/hda/hda_intel.c >> +++ b/sound/pci/hda/hda_intel.c >> @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) >> >> if (azx_has_pm_runtime(chip)) { >> pm_runtime_use_autosuspend(&pci->dev); >> + if (!use_vga_switcheroo(chip)) >> + pm_runtime_allow(&pci->dev); >> pm_runtime_put_autosuspend(&pci->dev); >> } >> >> -- >> 2.17.1 >>
On Tue, 14 Apr 2020 14:35:50 +0200, Hui Wang wrote: > > > On 2020/4/14 下午6:27, Takashi Iwai wrote: > > On Tue, 14 Apr 2020 12:14:05 +0200, > > Hui Wang wrote: > >> Before the pci_driver->probe() is called, the pci subsystem calls > >> runtime_forbib() and runtime_get_sync() on this pci dev, so only call > >> runtime_put_autosuspend() is not enough to enable the runtime_pm on > >> this device. > >> > >> For controllers with vgaswitcheroo feature, the pci/quirks.c will call > >> runtime_allow() for this dev, then the controllers could enter > >> rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like > >> Intel hda controllers, the runtime_pm is not enabled even it calls > >> put_autosuspend(). Need to call runtime_allow() for those controllers > >> in the hda driver. > >> > >> Signed-off-by: Hui Wang <hui.wang@canonical.com> > > Was this behavior changed from the earlier kernels? I thought this > > was left untouched because it's supposed to be set via udev rules or > > such. > > Oh, I don't know that, according to my test with ubuntu rootfs, the > runtime pm is not enabled on Intel's hda controllers. But with the sof > driver, the controller driver calls runtime_allow() > (soc/sof/sof-pci-dev.c), so I sent this patch. OK, I just watned to know the situation. So it's no regression but rather to align the behavior among drivers, and that's fine. Let's take the patch, then. thanks, Takashi > > Regards, > > Hui. > > > OTOH, enabling the runtime PM is almost mandatory for modern systems, > > and I'm fine to apply this kind of forcible enablement. > > > > > > thanks, > > > > Takashi > > > > > >> --- > >> sound/pci/hda/hda_intel.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > >> index 8519051a426e..779705bef88b 100644 > >> --- a/sound/pci/hda/hda_intel.c > >> +++ b/sound/pci/hda/hda_intel.c > >> @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) > >> if (azx_has_pm_runtime(chip)) { > >> pm_runtime_use_autosuspend(&pci->dev); > >> + if (!use_vga_switcheroo(chip)) > >> + pm_runtime_allow(&pci->dev); > >> pm_runtime_put_autosuspend(&pci->dev); > >> } > >> -- > >> 2.17.1 > >> >
On Tue, 14 Apr 2020 14:40:35 +0200, Takashi Iwai wrote: > > On Tue, 14 Apr 2020 14:35:50 +0200, > Hui Wang wrote: > > > > > > On 2020/4/14 下午6:27, Takashi Iwai wrote: > > > On Tue, 14 Apr 2020 12:14:05 +0200, > > > Hui Wang wrote: > > >> Before the pci_driver->probe() is called, the pci subsystem calls > > >> runtime_forbib() and runtime_get_sync() on this pci dev, so only call > > >> runtime_put_autosuspend() is not enough to enable the runtime_pm on > > >> this device. > > >> > > >> For controllers with vgaswitcheroo feature, the pci/quirks.c will call > > >> runtime_allow() for this dev, then the controllers could enter > > >> rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like > > >> Intel hda controllers, the runtime_pm is not enabled even it calls > > >> put_autosuspend(). Need to call runtime_allow() for those controllers > > >> in the hda driver. > > >> > > >> Signed-off-by: Hui Wang <hui.wang@canonical.com> > > > Was this behavior changed from the earlier kernels? I thought this > > > was left untouched because it's supposed to be set via udev rules or > > > such. > > > > Oh, I don't know that, according to my test with ubuntu rootfs, the > > runtime pm is not enabled on Intel's hda controllers. But with the sof > > driver, the controller driver calls runtime_allow() > > (soc/sof/sof-pci-dev.c), so I sent this patch. > > OK, I just watned to know the situation. So it's no regression but > rather to align the behavior among drivers, and that's fine. > Let's take the patch, then. On the second thought, we can drop the vga_switcheroo check and always allow it? We do want to enable in anyway, and calling it twice should be fine. thanks, Takashi > > > thanks, > > Takashi > > > > > Regards, > > > > Hui. > > > > > OTOH, enabling the runtime PM is almost mandatory for modern systems, > > > and I'm fine to apply this kind of forcible enablement. > > > > > > > > > thanks, > > > > > > Takashi > > > > > > > > >> --- > > >> sound/pci/hda/hda_intel.c | 2 ++ > > >> 1 file changed, 2 insertions(+) > > >> > > >> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > > >> index 8519051a426e..779705bef88b 100644 > > >> --- a/sound/pci/hda/hda_intel.c > > >> +++ b/sound/pci/hda/hda_intel.c > > >> @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) > > >> if (azx_has_pm_runtime(chip)) { > > >> pm_runtime_use_autosuspend(&pci->dev); > > >> + if (!use_vga_switcheroo(chip)) > > >> + pm_runtime_allow(&pci->dev); > > >> pm_runtime_put_autosuspend(&pci->dev); > > >> } > > >> -- > > >> 2.17.1 > > >> > >
Dear Hui, Takashi, Op 14-04-2020 om 13:35 schreef Hui Wang: > > On 2020/4/14 下午6:27, Takashi Iwai wrote: >> On Tue, 14 Apr 2020 12:14:05 +0200, >> Hui Wang wrote: >>> Before the pci_driver->probe() is called, the pci subsystem calls >>> runtime_forbib() and runtime_get_sync() on this pci dev, so only call >>> runtime_put_autosuspend() is not enough to enable the runtime_pm on >>> this device. >>> >>> For controllers with vgaswitcheroo feature, the pci/quirks.c will call >>> runtime_allow() for this dev, then the controllers could enter >>> rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like >>> Intel hda controllers, the runtime_pm is not enabled even it calls >>> put_autosuspend(). Need to call runtime_allow() for those controllers >>> in the hda driver. From what I can tell there are no ill effects of calling runtime_allow() twice. Technically, the check against use_vga_switcheroo() is thus redundant. Is there a good reason why runtime_allow() is called in the pci quirks rather than in hda_intel? Is it a suggestion to perform this call in hda_intel regardless of whether it's a switcheroo-device or not, and removing calls to runtime_allow() from the PCI quirks? Thanks. Best, Roy >>> >>> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> Was this behavior changed from the earlier kernels? I thought this >> was left untouched because it's supposed to be set via udev rules or >> such. > > Oh, I don't know that, according to my test with ubuntu rootfs, the > runtime pm is not enabled on Intel's hda controllers. But with the sof > driver, the controller driver calls runtime_allow() > (soc/sof/sof-pci-dev.c), so I sent this patch. > > Regards, > > Hui. > >> OTOH, enabling the runtime PM is almost mandatory for modern systems, >> and I'm fine to apply this kind of forcible enablement. >> >> >> thanks, >> >> Takashi >> >> >>> --- >>> sound/pci/hda/hda_intel.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c >>> index 8519051a426e..779705bef88b 100644 >>> --- a/sound/pci/hda/hda_intel.c >>> +++ b/sound/pci/hda/hda_intel.c >>> @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) >>> if (azx_has_pm_runtime(chip)) { >>> pm_runtime_use_autosuspend(&pci->dev); >>> + if (!use_vga_switcheroo(chip)) >>> + pm_runtime_allow(&pci->dev); >>> pm_runtime_put_autosuspend(&pci->dev); >>> } >>> -- >>> 2.17.1 >>>
On 2020/4/14 下午8:41, Takashi Iwai wrote: > On Tue, 14 Apr 2020 14:40:35 +0200, > Takashi Iwai wrote: >> On Tue, 14 Apr 2020 14:35:50 +0200, >> Hui Wang wrote: >>> >>> On 2020/4/14 下午6:27, Takashi Iwai wrote: >>>> On Tue, 14 Apr 2020 12:14:05 +0200, >>>> Hui Wang wrote: >>>>> Before the pci_driver->probe() is called, the pci subsystem calls >>>>> runtime_forbib() and runtime_get_sync() on this pci dev, so only call >>>>> runtime_put_autosuspend() is not enough to enable the runtime_pm on >>>>> this device. >>>>> >>>>> For controllers with vgaswitcheroo feature, the pci/quirks.c will call >>>>> runtime_allow() for this dev, then the controllers could enter >>>>> rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like >>>>> Intel hda controllers, the runtime_pm is not enabled even it calls >>>>> put_autosuspend(). Need to call runtime_allow() for those controllers >>>>> in the hda driver. >>>>> >>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com> >>>> Was this behavior changed from the earlier kernels? I thought this >>>> was left untouched because it's supposed to be set via udev rules or >>>> such. >>> Oh, I don't know that, according to my test with ubuntu rootfs, the >>> runtime pm is not enabled on Intel's hda controllers. But with the sof >>> driver, the controller driver calls runtime_allow() >>> (soc/sof/sof-pci-dev.c), so I sent this patch. >> OK, I just watned to know the situation. So it's no regression but >> rather to align the behavior among drivers, and that's fine. >> Let's take the patch, then. > On the second thought, we can drop the vga_switcheroo check and always > allow it? We do want to enable in anyway, and calling it twice should > be fine. > Yes, could drop the check, it is fine to call runtime_allow() twice. Thanks, Hui. > thanks, > > Takashi > > >> >> thanks, >> >> Takashi >> >>> Regards, >>> >>> Hui. >>> >>>> OTOH, enabling the runtime PM is almost mandatory for modern systems, >>>> and I'm fine to apply this kind of forcible enablement. >>>> >>>> >>>> thanks, >>>> >>>> Takashi >>>> >>>> >>>>> --- >>>>> sound/pci/hda/hda_intel.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c >>>>> index 8519051a426e..779705bef88b 100644 >>>>> --- a/sound/pci/hda/hda_intel.c >>>>> +++ b/sound/pci/hda/hda_intel.c >>>>> @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) >>>>> if (azx_has_pm_runtime(chip)) { >>>>> pm_runtime_use_autosuspend(&pci->dev); >>>>> + if (!use_vga_switcheroo(chip)) >>>>> + pm_runtime_allow(&pci->dev); >>>>> pm_runtime_put_autosuspend(&pci->dev); >>>>> } >>>>> -- >>>>> 2.17.1 >>>>>
On 2020/4/14 下午8:42, Roy Spliet wrote: > Dear Hui, Takashi, > > Op 14-04-2020 om 13:35 schreef Hui Wang: >> >> On 2020/4/14 下午6:27, Takashi Iwai wrote: >>> On Tue, 14 Apr 2020 12:14:05 +0200, >>> Hui Wang wrote: >>>> Before the pci_driver->probe() is called, the pci subsystem calls >>>> runtime_forbib() and runtime_get_sync() on this pci dev, so only call >>>> runtime_put_autosuspend() is not enough to enable the runtime_pm on >>>> this device. >>>> >>>> For controllers with vgaswitcheroo feature, the pci/quirks.c will call >>>> runtime_allow() for this dev, then the controllers could enter >>>> rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like >>>> Intel hda controllers, the runtime_pm is not enabled even it calls >>>> put_autosuspend(). Need to call runtime_allow() for those controllers >>>> in the hda driver. > > From what I can tell there are no ill effects of calling > runtime_allow() twice. Technically, the check against > use_vga_switcheroo() is thus redundant. > Is there a good reason why runtime_allow() is called in the pci quirks > rather than in hda_intel? Is it a suggestion to perform this call in > hda_intel regardless of whether it's a switcheroo-device or not, and > removing calls to runtime_allow() from the PCI quirks? I guess after the hda driver calls the _allow() unconditionally, we could remove the _allow() in the pci/quirks.c. But it is no harm keeping it since _allow() could be called many times. Thanks, Hui. > Thanks. Best, > > Roy >>>> >>>> Signed-off-by: Hui Wang <hui.wang@canonical.com> >>> Was this behavior changed from the earlier kernels? I thought this >>> was left untouched because it's supposed to be set via udev rules or >>> such. >> >> Oh, I don't know that, according to my test with ubuntu rootfs, the >> runtime pm is not enabled on Intel's hda controllers. But with the >> sof driver, the controller driver calls runtime_allow() >> (soc/sof/sof-pci-dev.c), so I sent this patch. >> >> Regards, >> >> Hui. >> >>> OTOH, enabling the runtime PM is almost mandatory for modern systems, >>> and I'm fine to apply this kind of forcible enablement. >>> >>> >>> thanks, >>> >>> Takashi >>> >>> >>>> --- >>>> sound/pci/hda/hda_intel.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c >>>> index 8519051a426e..779705bef88b 100644 >>>> --- a/sound/pci/hda/hda_intel.c >>>> +++ b/sound/pci/hda/hda_intel.c >>>> @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) >>>> if (azx_has_pm_runtime(chip)) { >>>> pm_runtime_use_autosuspend(&pci->dev); >>>> + if (!use_vga_switcheroo(chip)) >>>> + pm_runtime_allow(&pci->dev); >>>> pm_runtime_put_autosuspend(&pci->dev); >>>> } >>>> -- >>>> 2.17.1 >>>>
Op 14-04-2020 om 13:56 schreef Hui Wang: > > On 2020/4/14 下午8:42, Roy Spliet wrote: >> Dear Hui, Takashi, >> >> Op 14-04-2020 om 13:35 schreef Hui Wang: >>> >>> On 2020/4/14 下午6:27, Takashi Iwai wrote: >>>> On Tue, 14 Apr 2020 12:14:05 +0200, >>>> Hui Wang wrote: >>>>> Before the pci_driver->probe() is called, the pci subsystem calls >>>>> runtime_forbib() and runtime_get_sync() on this pci dev, so only call >>>>> runtime_put_autosuspend() is not enough to enable the runtime_pm on >>>>> this device. >>>>> >>>>> For controllers with vgaswitcheroo feature, the pci/quirks.c will call >>>>> runtime_allow() for this dev, then the controllers could enter >>>>> rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like >>>>> Intel hda controllers, the runtime_pm is not enabled even it calls >>>>> put_autosuspend(). Need to call runtime_allow() for those controllers >>>>> in the hda driver. >> >> From what I can tell there are no ill effects of calling >> runtime_allow() twice. Technically, the check against >> use_vga_switcheroo() is thus redundant. >> Is there a good reason why runtime_allow() is called in the pci quirks >> rather than in hda_intel? Is it a suggestion to perform this call in >> hda_intel regardless of whether it's a switcheroo-device or not, and >> removing calls to runtime_allow() from the PCI quirks? > > I guess after the hda driver calls the _allow() unconditionally, we > could remove the _allow() in the pci/quirks.c. But it is no harm keeping > it since _allow() could be called many times. After a bit of research, I agree! It's better left in place in the PCI quirks, such that in the case that hda_intel doesn't load or bind for whatever reason (not compiled in e.g. an embedded kernel, new/unknown PCI vendor/device identifier), the GPUs HDA device can still run-time suspend. Thank you! Best, Roy > > Thanks, > > Hui. > >> Thanks. Best, >> >> Roy >>>>> >>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com> >>>> Was this behavior changed from the earlier kernels? I thought this >>>> was left untouched because it's supposed to be set via udev rules or >>>> such. >>> >>> Oh, I don't know that, according to my test with ubuntu rootfs, the >>> runtime pm is not enabled on Intel's hda controllers. But with the >>> sof driver, the controller driver calls runtime_allow() >>> (soc/sof/sof-pci-dev.c), so I sent this patch. >>> >>> Regards, >>> >>> Hui. >>> >>>> OTOH, enabling the runtime PM is almost mandatory for modern systems, >>>> and I'm fine to apply this kind of forcible enablement. >>>> >>>> >>>> thanks, >>>> >>>> Takashi >>>> >>>> >>>>> --- >>>>> sound/pci/hda/hda_intel.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c >>>>> index 8519051a426e..779705bef88b 100644 >>>>> --- a/sound/pci/hda/hda_intel.c >>>>> +++ b/sound/pci/hda/hda_intel.c >>>>> @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) >>>>> if (azx_has_pm_runtime(chip)) { >>>>> pm_runtime_use_autosuspend(&pci->dev); >>>>> + if (!use_vga_switcheroo(chip)) >>>>> + pm_runtime_allow(&pci->dev); >>>>> pm_runtime_put_autosuspend(&pci->dev); >>>>> } >>>>> -- >>>>> 2.17.1 >>>>>
On Tue, 14 Apr 2020 14:59:01 +0200, Roy Spliet wrote: > > Op 14-04-2020 om 13:56 schreef Hui Wang: > > > > On 2020/4/14 下午8:42, Roy Spliet wrote: > >> Dear Hui, Takashi, > >> > >> Op 14-04-2020 om 13:35 schreef Hui Wang: > >>> > >>> On 2020/4/14 下午6:27, Takashi Iwai wrote: > >>>> On Tue, 14 Apr 2020 12:14:05 +0200, > >>>> Hui Wang wrote: > >>>>> Before the pci_driver->probe() is called, the pci subsystem calls > >>>>> runtime_forbib() and runtime_get_sync() on this pci dev, so only call > >>>>> runtime_put_autosuspend() is not enough to enable the runtime_pm on > >>>>> this device. > >>>>> > >>>>> For controllers with vgaswitcheroo feature, the pci/quirks.c will call > >>>>> runtime_allow() for this dev, then the controllers could enter > >>>>> rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like > >>>>> Intel hda controllers, the runtime_pm is not enabled even it calls > >>>>> put_autosuspend(). Need to call runtime_allow() for those controllers > >>>>> in the hda driver. > >> > >> From what I can tell there are no ill effects of calling > >> runtime_allow() twice. Technically, the check against > >> use_vga_switcheroo() is thus redundant. > >> Is there a good reason why runtime_allow() is called in the pci > >> quirks rather than in hda_intel? Is it a suggestion to perform this > >> call in hda_intel regardless of whether it's a switcheroo-device or > >> not, and removing calls to runtime_allow() from the PCI quirks? > > > > I guess after the hda driver calls the _allow() unconditionally, we > > could remove the _allow() in the pci/quirks.c. But it is no harm > > keeping it since _allow() could be called many times. > > After a bit of research, I agree! It's better left in place in the PCI > quirks, such that in the case that hda_intel doesn't load or bind for > whatever reason (not compiled in e.g. an embedded kernel, new/unknown > PCI vendor/device identifier), the GPUs HDA device can still run-time > suspend. Right, some background information is found in the original commit that introduced the quirk (07f4f97d7b4b). Hui, care to resend the patch with that change, also a bit refreshing the patch description? thanks, Takashi > Thank you! Best, > > Roy > > > > > Thanks, > > > > Hui. > > > >> Thanks. Best, > >> > >> Roy > >>>>> > >>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com> > >>>> Was this behavior changed from the earlier kernels? I thought this > >>>> was left untouched because it's supposed to be set via udev rules or > >>>> such. > >>> > >>> Oh, I don't know that, according to my test with ubuntu rootfs, > >>> the runtime pm is not enabled on Intel's hda controllers. But with > >>> the sof driver, the controller driver calls runtime_allow() > >>> (soc/sof/sof-pci-dev.c), so I sent this patch. > >>> > >>> Regards, > >>> > >>> Hui. > >>> > >>>> OTOH, enabling the runtime PM is almost mandatory for modern systems, > >>>> and I'm fine to apply this kind of forcible enablement. > >>>> > >>>> > >>>> thanks, > >>>> > >>>> Takashi > >>>> > >>>> > >>>>> --- > >>>>> sound/pci/hda/hda_intel.c | 2 ++ > >>>>> 1 file changed, 2 insertions(+) > >>>>> > >>>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > >>>>> index 8519051a426e..779705bef88b 100644 > >>>>> --- a/sound/pci/hda/hda_intel.c > >>>>> +++ b/sound/pci/hda/hda_intel.c > >>>>> @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) > >>>>> if (azx_has_pm_runtime(chip)) { > >>>>> pm_runtime_use_autosuspend(&pci->dev); > >>>>> + if (!use_vga_switcheroo(chip)) > >>>>> + pm_runtime_allow(&pci->dev); > >>>>> pm_runtime_put_autosuspend(&pci->dev); > >>>>> } > >>>>> -- > >>>>> 2.17.1 > >>>>> >
On 2020/4/14 下午9:06, Takashi Iwai wrote: > On Tue, 14 Apr 2020 14:59:01 +0200, > Roy Spliet wrote: >> Op 14-04-2020 om 13:56 schreef Hui Wang: >>> On 2020/4/14 下午8:42, Roy Spliet wrote: >>>> Dear Hui, Takashi, >>>> >>>> Op 14-04-2020 om 13:35 schreef Hui Wang: >>>>> On 2020/4/14 下午6:27, Takashi Iwai wrote: >>>>>> On Tue, 14 Apr 2020 12:14:05 +0200, >>>>>> Hui Wang wrote: >>>>>>> Before the pci_driver->probe() is called, the pci subsystem calls >>>>>>> runtime_forbib() and runtime_get_sync() on this pci dev, so only call >>>>>>> runtime_put_autosuspend() is not enough to enable the runtime_pm on >>>>>>> this device. >>>>>>> >>>>>>> For controllers with vgaswitcheroo feature, the pci/quirks.c will call >>>>>>> runtime_allow() for this dev, then the controllers could enter >>>>>>> rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like >>>>>>> Intel hda controllers, the runtime_pm is not enabled even it calls >>>>>>> put_autosuspend(). Need to call runtime_allow() for those controllers >>>>>>> in the hda driver. >>>> From what I can tell there are no ill effects of calling >>>> runtime_allow() twice. Technically, the check against >>>> use_vga_switcheroo() is thus redundant. >>>> Is there a good reason why runtime_allow() is called in the pci >>>> quirks rather than in hda_intel? Is it a suggestion to perform this >>>> call in hda_intel regardless of whether it's a switcheroo-device or >>>> not, and removing calls to runtime_allow() from the PCI quirks? >>> I guess after the hda driver calls the _allow() unconditionally, we >>> could remove the _allow() in the pci/quirks.c. But it is no harm >>> keeping it since _allow() could be called many times. >> After a bit of research, I agree! It's better left in place in the PCI >> quirks, such that in the case that hda_intel doesn't load or bind for >> whatever reason (not compiled in e.g. an embedded kernel, new/unknown >> PCI vendor/device identifier), the GPUs HDA device can still run-time >> suspend. > Right, some background information is found in the original commit > that introduced the quirk (07f4f97d7b4b). > > Hui, care to resend the patch with that change, also a bit refreshing > the patch description? OK. Thanks. > > thanks, > > Takashi > > >> Thank you! Best, >> >> Roy >> >>> Thanks, >>> >>> Hui. >>> >>>> Thanks. Best, >>>> >>>> Roy >>>>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com> >>>>>> Was this behavior changed from the earlier kernels? I thought this >>>>>> was left untouched because it's supposed to be set via udev rules or >>>>>> such. >>>>> Oh, I don't know that, according to my test with ubuntu rootfs, >>>>> the runtime pm is not enabled on Intel's hda controllers. But with >>>>> the sof driver, the controller driver calls runtime_allow() >>>>> (soc/sof/sof-pci-dev.c), so I sent this patch. >>>>> >>>>> Regards, >>>>> >>>>> Hui. >>>>> >>>>>> OTOH, enabling the runtime PM is almost mandatory for modern systems, >>>>>> and I'm fine to apply this kind of forcible enablement. >>>>>> >>>>>> >>>>>> thanks, >>>>>> >>>>>> Takashi >>>>>> >>>>>> >>>>>>> --- >>>>>>> sound/pci/hda/hda_intel.c | 2 ++ >>>>>>> 1 file changed, 2 insertions(+) >>>>>>> >>>>>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c >>>>>>> index 8519051a426e..779705bef88b 100644 >>>>>>> --- a/sound/pci/hda/hda_intel.c >>>>>>> +++ b/sound/pci/hda/hda_intel.c >>>>>>> @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) >>>>>>> if (azx_has_pm_runtime(chip)) { >>>>>>> pm_runtime_use_autosuspend(&pci->dev); >>>>>>> + if (!use_vga_switcheroo(chip)) >>>>>>> + pm_runtime_allow(&pci->dev); >>>>>>> pm_runtime_put_autosuspend(&pci->dev); >>>>>>> } >>>>>>> -- >>>>>>> 2.17.1 >>>>>>>
Hi Hui,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on sound/for-next]
[also build test ERROR on next-20200414]
[cannot apply to v5.7-rc1]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Hui-Wang/ALSA-hda-enable-the-runtime_pm-for-non-vgaswitcheroo-hda-controllers/20200414-232444
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
sound/pci/hda/hda_intel.c: In function 'azx_probe_continue':
>> sound/pci/hda/hda_intel.c:362:41: error: 'struct azx' has no member named 'use_vga_switcheroo'
#define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo)
^
>> sound/pci/hda/hda_intel.c:2359:8: note: in expansion of macro 'use_vga_switcheroo'
if (!use_vga_switcheroo(chip))
^~~~~~~~~~~~~~~~~~
vim +362 sound/pci/hda/hda_intel.c
c87693da69f979 Lu, Han 2015-11-19 328
9477c58e3308f5 Takashi Iwai 2011-05-25 329 /* quirks for ATI SB / AMD Hudson */
9477c58e3308f5 Takashi Iwai 2011-05-25 330 #define AZX_DCAPS_PRESET_ATI_SB \
37e661ee10c6d0 Takashi Iwai 2014-11-25 331 (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_POSFIX_LPIB |\
37e661ee10c6d0 Takashi Iwai 2014-11-25 332 AZX_DCAPS_SNOOP_TYPE(ATI))
9477c58e3308f5 Takashi Iwai 2011-05-25 333
9477c58e3308f5 Takashi Iwai 2011-05-25 334 /* quirks for ATI/AMD HDMI */
9477c58e3308f5 Takashi Iwai 2011-05-25 335 #define AZX_DCAPS_PRESET_ATI_HDMI \
db79afa1e57925 Benjamin Herrenschmidt 2014-11-24 336 (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_POSFIX_LPIB|\
db79afa1e57925 Benjamin Herrenschmidt 2014-11-24 337 AZX_DCAPS_NO_MSI64)
9477c58e3308f5 Takashi Iwai 2011-05-25 338
37e661ee10c6d0 Takashi Iwai 2014-11-25 339 /* quirks for ATI HDMI with snoop off */
37e661ee10c6d0 Takashi Iwai 2014-11-25 340 #define AZX_DCAPS_PRESET_ATI_HDMI_NS \
37e661ee10c6d0 Takashi Iwai 2014-11-25 341 (AZX_DCAPS_PRESET_ATI_HDMI | AZX_DCAPS_SNOOP_OFF)
37e661ee10c6d0 Takashi Iwai 2014-11-25 342
c02f77d32d2c45 Takashi Iwai 2019-08-06 343 /* quirks for AMD SB */
c02f77d32d2c45 Takashi Iwai 2019-08-06 344 #define AZX_DCAPS_PRESET_AMD_SB \
c02f77d32d2c45 Takashi Iwai 2019-08-06 345 (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_AMD_WORKAROUND |\
c02f77d32d2c45 Takashi Iwai 2019-08-06 346 AZX_DCAPS_SNOOP_TYPE(ATI) | AZX_DCAPS_PM_RUNTIME)
c02f77d32d2c45 Takashi Iwai 2019-08-06 347
9477c58e3308f5 Takashi Iwai 2011-05-25 348 /* quirks for Nvidia */
9477c58e3308f5 Takashi Iwai 2011-05-25 349 #define AZX_DCAPS_PRESET_NVIDIA \
3ab7511eafdd5c Ard Biesheuvel 2016-10-17 350 (AZX_DCAPS_NO_MSI | AZX_DCAPS_CORBRP_SELF_CLEAR |\
37e661ee10c6d0 Takashi Iwai 2014-11-25 351 AZX_DCAPS_SNOOP_TYPE(NVIDIA))
9477c58e3308f5 Takashi Iwai 2011-05-25 352
5ae763b1bc573e Takashi Iwai 2012-05-08 353 #define AZX_DCAPS_PRESET_CTHDA \
37e661ee10c6d0 Takashi Iwai 2014-11-25 354 (AZX_DCAPS_NO_MSI | AZX_DCAPS_POSFIX_LPIB |\
cadd16ea33a938 Takashi Iwai 2015-10-27 355 AZX_DCAPS_NO_64BIT |\
37e661ee10c6d0 Takashi Iwai 2014-11-25 356 AZX_DCAPS_4K_BDLE_BOUNDARY | AZX_DCAPS_SNOOP_OFF)
5ae763b1bc573e Takashi Iwai 2012-05-08 357
a82d51ed24bb79 Takashi Iwai 2012-04-26 358 /*
2b760d88a0fcd8 Lukas Wunner 2015-09-04 359 * vga_switcheroo support
a82d51ed24bb79 Takashi Iwai 2012-04-26 360 */
a82d51ed24bb79 Takashi Iwai 2012-04-26 361 #ifdef SUPPORT_VGA_SWITCHEROO
5cb543dba98675 Takashi Iwai 2012-08-09 @362 #define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo)
dd23e1d566d0f7 Takashi Iwai 2019-08-27 363 #define needs_eld_notify_link(chip) ((chip)->bus.keep_power)
5cb543dba98675 Takashi Iwai 2012-08-09 364 #else
5cb543dba98675 Takashi Iwai 2012-08-09 365 #define use_vga_switcheroo(chip) 0
37a3a98ef601f8 Takashi Iwai 2018-09-10 366 #define needs_eld_notify_link(chip) false
5cb543dba98675 Takashi Iwai 2012-08-09 367 #endif
5cb543dba98675 Takashi Iwai 2012-08-09 368
:::::: The code at line 362 was first introduced by commit
:::::: 5cb543dba9867588786f87af2e64fca371b69283 ALSA: hda - Deferred probing with request_firmware_nowait()
:::::: TO: Takashi Iwai <tiwai@suse.de>
:::::: CC: Takashi Iwai <tiwai@suse.de>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 8519051a426e..779705bef88b 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2356,6 +2356,8 @@ static int azx_probe_continue(struct azx *chip) if (azx_has_pm_runtime(chip)) { pm_runtime_use_autosuspend(&pci->dev); + if (!use_vga_switcheroo(chip)) + pm_runtime_allow(&pci->dev); pm_runtime_put_autosuspend(&pci->dev); }
Before the pci_driver->probe() is called, the pci subsystem calls runtime_forbib() and runtime_get_sync() on this pci dev, so only call runtime_put_autosuspend() is not enough to enable the runtime_pm on this device. For controllers with vgaswitcheroo feature, the pci/quirks.c will call runtime_allow() for this dev, then the controllers could enter rt_idle/suspend/resume, but for non-vgaswitcheroo controllers like Intel hda controllers, the runtime_pm is not enabled even it calls put_autosuspend(). Need to call runtime_allow() for those controllers in the hda driver. Signed-off-by: Hui Wang <hui.wang@canonical.com> --- sound/pci/hda/hda_intel.c | 2 ++ 1 file changed, 2 insertions(+)