Re: [PATCH] ACPI: video: Invoke _PS0 at boot for ACPI video

[Hans de Goede]

Hi,

On 7/6/23 10:20, Kai-Heng Feng wrote:
> On Wed, Jul 5, 2023 at 6:33 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On 7/4/23 18:58, Rafael J. Wysocki wrote:
>>> On Tue, Jul 4, 2023 at 9:46 AM Kai-Heng Feng
>>> <kai.heng.feng@xxxxxxxxxxxxx> wrote:
>>>>
>>>> Screen brightness can only be changed once on some HP laptops.
>>>>
>>>> Vendor identified the root cause as Linux doesn't invoke _PS0 at boot
>>>> for all ACPI devices:
>>>
>>> This part of the changelog is confusing, because the evaluation of
>>> _PS0 is not a separate operation.  _PS0 gets evaluated when devices
>>> undergo transitions from low-power states to D0.
>>>
>>>>     Scope (\_SB.PC00.GFX0)
>>>>     {
>>>>         Scope (DD1F)
>>>>         {
>>>>             Method (_PS0, 0, Serialized)  // _PS0: Power State 0
>>>>             {
>>>>                 If (CondRefOf (\_SB.PC00.LPCB.EC0.SSBC))
>>>>                 {
>>>>                     \_SB.PC00.LPCB.EC0.SSBC ()
>>>>                 }
>>>>             }
>>>>             ...
>>>>         }
>>>>         ...
>>>>     }
>>>>
>>>> _PS0 doesn't get invoked for all ACPI devices because of commit
>>>> 7cd8407d53ef ("ACPI / PM: Do not execute _PS0 for devices without _PSC
>>>> during initialization").
>>
>> So this _PS0 which seems to be the one which needs to run here,
>> is not the _PS0 for the GFX0 ACPI device, but rather for a child ACPI device-node which describes the connector (assumed based on the small part of quoted DSDT, the actual definition of the DD1F device-node is missing).
> 
> I'll file a bugzilla and attach a full acpidump there.
> 
>>
>> Having a _PS0 method on a connector object is really weird IMHO. But if we need to invoke such a _PS0 method then IMHO that really should be done in the drm/kms driver. E.g. at least the i915 code already contains code to map the ACPI connector objects to the drm_connector objects, so it should be relatively easily to make that try and do a power-transition to D0 when enabling the connector.
> 
> Or put all ACPI devices to D0 at boot?
> According to the BIOS folks that's what Windows does.
> This way we can drop acpi_device_fix_up_power* helpers altogether.

Doing that will leave any devices for which we lack a driver at D0 for ever,
so that IMHO is not a good idea.

I guess calling acpi_device_fix_up_power_extended(device) from the
ACPI-video code, so that the connector sub-objects are put in D0 is
somewhat ok. Although I would prefer to see you first try to do
the same thing from the i915 driver instead.

If we do end up doing this from the acpi-video code please add
a comment above the call why this is done; and as Rafael mentioned
the commit msg needs to better explain things too.

Regards,

Hans



> 
>>
>> Also can you provide some more info on the hw on which this is being seen:
>>
>> 1. What GPU(s) is/are being used
> 
> Intel GFX.
> 
> AFAIK AMD based laptops also require this fixup too.
> 
>> 2. If there is a mux for hybrid gfx in which mode is the mux set ?
> 
> No. This happens to mux-less and dGPU-less laptops.
> 
>> 3. Wjich method is used to control the brightness (which backlight-class-devices show up under /sys/class/backlight) ?
> 
> intel_backlight.
> 
>>
>> And can you add this info to the commit msg for the next version of the patch ?
> 
> Sure.
> Can putting all devices to D0 be considered too? It's a better
> solution for the long wrong.
> 
> Kai-Heng
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>>
>>> And yes, Linux doesn't put all of the ACPI devices into D0 during
>>> initialization, but the above commit has a little to do with that.
>>>
>>>> For now explicitly call _PS0 for ACPI video to workaround the issue.
>>>
>>> This is not what the patch is doing.
>>>
>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
>>>> ---
>>>>  drivers/acpi/acpi_video.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
>>>> index 62f4364e4460..793259bd18c8 100644
>>>> --- a/drivers/acpi/acpi_video.c
>>>> +++ b/drivers/acpi/acpi_video.c
>>>> @@ -2027,6 +2027,8 @@ static int acpi_video_bus_add(struct acpi_device *device)
>>>>         if (error)
>>>>                 goto err_put_video;
>>>>
>>>> +       acpi_device_fix_up_power_extended(device);
>>>> +
>>>
>>> I would like to know what Hans thinks about this.
>>>
>>>>         pr_info("%s [%s] (multi-head: %s  rom: %s  post: %s)\n",
>>>>                ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
>>>>                video->flags.multihead ? "yes" : "no",
>>>> --
>>>
>>
>