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: <CAAd53p5nkE+QdxJwF_mEsNiEHvRwg+4D7yP77H6CDrMWPOX_zQ@mail.gmail.com>
Date:   Thu, 6 Jul 2023 16:20:21 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>, 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

On Wed, Jul 5, 2023 at 6:33 PM Hans de Goede <hdegoede@...hat.com> 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@...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).

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.

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