| Message ID | 20230704074506.2304939-1-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: video: Invoke _PS0 at boot for ACPI video | expand |
On Wed, Jul 5, 2023 at 6:33 PM Hans de Goede <hdegoede@redhat.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@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). 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@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", > >> -- > > >
Hi, On 7/6/23 10:20, Kai-Heng Feng wrote: > On Wed, Jul 5, 2023 at 6:33 PM Hans de Goede <hdegoede@redhat.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@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). > > 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@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", >>>> -- >>> >> >
Hi Hans, On Mon, Jul 10, 2023 at 8:54 PM Hans de Goede <hdegoede@redhat.com> wrote: [snipped] > > 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. I was told by vendor that the same design is used on AMD based laptops too. So I think putting this in ACPI-video is a better approach. > > 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. Sure. Kai-Heng > > 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@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", > >>>> -- > >>> > >> > > >
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); + 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",
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: 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"). For now explicitly call _PS0 for ACPI video to workaround the issue. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/acpi/acpi_video.c | 2 ++ 1 file changed, 2 insertions(+)