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