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@xxxxxxxxxxxxx>
> >>>> 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
> >>>>>
>
