[PATCH] ALSA: hda: enable the runtime_pm for non-vgaswitcheroo hda controllers

[Hui Wang]

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 at 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
>>>>>>>