| Message ID | 20230719023223.2971481-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] ACPI: video: Put ACPI video and its child devices to D0 at boot | expand |
On Wed, Jul 19, 2023 at 4:33 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: No, it doesn't and it won't. For the reason why please see the comment in acpi_bus_init_power(). BTW, missing _PSC for a device that has _PS0 violates Section 7.3.6. _PSC (Power State Current) of the ACPI specification, so this is a platform firmware problem. > 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 () > } > } > ... > } > ... > } > > The \_SB.PC00.GFX0.DD1F is the panel device, and its _PS0 needs to be > executed to make the brightness control work properly. > > _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 use acpi_device_fix_up_power_extended() > to put ACPI video and its child devices to D0 to workaround the issue. The above paragraph is misleading the reader into thinking that the issue was introduced by a kernel change while the issue really is there in the platform firmware and it is the missing _PSC. Blaming kernel changes for platform firmware defects is not really productive. So this really is a workaround for defective platform firmware on the affected systems and it should be documented as such. > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217683 > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > v2: > - Wording > - Bugzilla > - Add comment > > drivers/acpi/acpi_video.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > index 62f4364e4460..bac614e9fe56 100644 > --- a/drivers/acpi/acpi_video.c > +++ b/drivers/acpi/acpi_video.c > @@ -2027,6 +2027,13 @@ static int acpi_video_bus_add(struct acpi_device *device) > if (error) > goto err_put_video; > > + > + /* > + * Some HP laptops require ACPI video's child devices have _PS0 > + * evaluated to have functional panel brightness control. So the comment should mention the platform firmware defect and it shouldn't say "some" but explicitly identify the defective platforms. > + */ > + acpi_device_fix_up_power_extended(device); > + And the code change itself is simple enough, so it can be made as long as it is fine with Hans. > 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", > --
On Fri, Jul 21, 2023 at 1:32 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jul 19, 2023 at 4:33 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: > > No, it doesn't and it won't. For the reason why please see the > comment in acpi_bus_init_power(). Understood. > > BTW, missing _PSC for a device that has _PS0 violates Section 7.3.6. > _PSC (Power State Current) of the ACPI specification, so this is a > platform firmware problem. This means most if not all platforms have this problem. > > > 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 () > > } > > } > > ... > > } > > ... > > } > > > > The \_SB.PC00.GFX0.DD1F is the panel device, and its _PS0 needs to be > > executed to make the brightness control work properly. > > > > _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 use acpi_device_fix_up_power_extended() > > to put ACPI video and its child devices to D0 to workaround the issue. > > The above paragraph is misleading the reader into thinking that the > issue was introduced by a kernel change while the issue really is > there in the platform firmware and it is the missing _PSC. > > Blaming kernel changes for platform firmware defects is not really productive. Right, I'll reword the message and make it clear that it's firmware's issue. > > So this really is a workaround for defective platform firmware on the > affected systems and it should be documented as such. Sure. > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217683 > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > v2: > > - Wording > > - Bugzilla > > - Add comment > > > > drivers/acpi/acpi_video.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > > index 62f4364e4460..bac614e9fe56 100644 > > --- a/drivers/acpi/acpi_video.c > > +++ b/drivers/acpi/acpi_video.c > > @@ -2027,6 +2027,13 @@ static int acpi_video_bus_add(struct acpi_device *device) > > if (error) > > goto err_put_video; > > > > + > > + /* > > + * Some HP laptops require ACPI video's child devices have _PS0 > > + * evaluated to have functional panel brightness control. > > So the comment should mention the platform firmware defect and it > shouldn't say "some" but explicitly identify the defective platforms. OK. > > > + */ > > + acpi_device_fix_up_power_extended(device); > > + > > And the code change itself is simple enough, so it can be made as long > as it is fine with Hans. OK. Kai-Heng > > > 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..bac614e9fe56 100644 --- a/drivers/acpi/acpi_video.c +++ b/drivers/acpi/acpi_video.c @@ -2027,6 +2027,13 @@ static int acpi_video_bus_add(struct acpi_device *device) if (error) goto err_put_video; + + /* + * Some HP laptops require ACPI video's child devices have _PS0 + * evaluated to have functional panel brightness control. + */ + 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 () } } ... } ... } The \_SB.PC00.GFX0.DD1F is the panel device, and its _PS0 needs to be executed to make the brightness control work properly. _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 use acpi_device_fix_up_power_extended() to put ACPI video and its child devices to D0 to workaround the issue. Link: https://bugzilla.kernel.org/show_bug.cgi?id=217683 Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v2: - Wording - Bugzilla - Add comment drivers/acpi/acpi_video.c | 7 +++++++ 1 file changed, 7 insertions(+)