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",
>>>> --
>>>
>>
>
