linux-kernel - Re: [PATCH] ACPI: video: Invoke

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f5a4f802-d6a1-050e-ec70-701048ab1a2f@redhat.com>
Date:   Wed, 5 Jul 2023 12:33:08 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Kai-Heng Feng <kai.heng.feng@...onical.com>
Cc:     lenb@...nel.org, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ACPI: video: Invoke _PS0 at boot for ACPI video

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@...onical.com> 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).

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.

Also can you provide some more info on the hw on which this is being seen:

1. What GPU(s) is/are being used
2. If there is a mux for hybrid gfx in which mode is the mux set ?
3. Wjich method is used to control the brightness (which backlight-class-devices show up under /sys/class/backlight) ?

And can you add this info to the commit msg for the next version of the patch ?

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@...onical.com>
>> ---
>>  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",
>> --
>