Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f5a4f802-d6a1-050e-ec70-701048ab1a2f@redhat.com>
Date:   Wed, 5 Jul 2023 12:33:08 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH] ACPI: video: Invoke _PS0 at boot for ACPI video
To:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc:     lenb@kernel.org, linux-acpi@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230704074506.2304939-1-kai.heng.feng@canonical.com>
 <CAJZ5v0jwO1NJ_x2t3RV-kYDmVY9UtyexznCSZMAmQ-gK4dWCmA@mail.gmail.com>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <CAJZ5v0jwO1NJ_x2t3RV-kYDmVY9UtyexznCSZMAmQ-gK4dWCmA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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