[RFC 0/2] ACPI: video: prefer native over vendor

[RFC 0/2] ACPI: video: prefer native over vendor

[Hans de Goede]

Hi Rafael, Matthew,

Here is a second attempt at always registering only a single
/sys/class/backlight device per panel.

This first round of testing has shown that native works well even on
systems so old that the don't have acpi_video backlight control support.

This patch series makes native be preferred over vendor, which should
avoid the problems seen with the 6.1 changes before the fixes.

ATM there is one known model where this will cause a regression,
the Sony Vaio PCG-FRV3 from 2003. I plan to add a DMI quirk for that
in the next version of this series, but I'm waiting for some more
testing (to check that the vendor interface does actually work) first.

I will also do another blogpost, focussing on asking users to see
if they have a laptop which provides a combination of vendor + native
backlight interfaces, which may be impacted by this series. This is
the main reason why this is a RFC for now.

Note this applies on top of my series with fixes for 6.1:
https://lore.kernel.org/linux-acpi/20221104212108.73537-1-hdegoede@redhat.com/

So we need to be a bit careful with merging this to avoid
non trivial conflicts. Assuming that series gets send to Linus
for 6.1 through the pdx86 tree, it might be best for me to
rebase pdx86/for-next on top of the fixes and then this can
(also) be merged through pdx86/for-next without conflicts.

Regards,

Hans

p.s.

Matthew, this should keep your custom coreboot laptop working since
acpi_video_backlight_use_native() will return true there now.


Hans de Goede (2):
  ACPI: video: Simplify __acpi_video_get_backlight_type()
  ACPI: video: prefer native over vendor

 drivers/acpi/video_detect.c | 52 ++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 33 deletions(-)

-- 
2.37.3

[RFC 1/2] ACPI: video: Simplify __acpi_video_get_backlight_type()

[Hans de Goede]

Simplify __acpi_video_get_backlight_type() removing a nested if which
makes the flow harder to follow.

This also results in having only 1 exit point with
return acpi_backlight_native instead of 2.

Note this drops the (video_caps & ACPI_VIDEO_BACKLIGHT) check from
the if (acpi_osi_is_win8() && native_available) return native path.
Windows 8's hardware certification requirements include that there must
be ACPI video bus backlight control, so the ACPI_VIDEO_BACKLIGHT check
is redundant.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/video_detect.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index b2a616287638..ec7421a3986f 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -687,6 +687,16 @@ static bool google_cros_ec_present(void)
 	return acpi_dev_found("GOOG0004") || acpi_dev_found("GOOG000C");
 }
 
+/*
+ * Windows 8 and newer no longer use the ACPI video interface, so it often
+ * does not work. So on win8+ systems prefer native brightness control.
+ * Chromebooks should always prefer native backlight control.
+ */
+static bool prefer_native_over_acpi_video(void)
+{
+	return acpi_osi_is_win8() || google_cros_ec_present();
+}
+
 /*
  * Determine which type of backlight interface to use on this system,
  * First check cmdline, then dmi quirks, then do autodetect.
@@ -732,26 +742,14 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
 	if (apple_gmux_present())
 		return acpi_backlight_apple_gmux;
 
-	/* Chromebooks should always prefer native backlight control. */
-	if (google_cros_ec_present() && native_available)
-		return acpi_backlight_native;
+	/* Use ACPI video if available, except when native should be preferred. */
+	if ((video_caps & ACPI_VIDEO_BACKLIGHT) &&
+	     !(native_available && prefer_native_over_acpi_video()))
+		return acpi_backlight_video;
 
-	/* On systems with ACPI video use either native or ACPI video. */
-	if (video_caps & ACPI_VIDEO_BACKLIGHT) {
-		/*
-		 * Windows 8 and newer no longer use the ACPI video interface,
-		 * so it often does not work. If the ACPI tables are written
-		 * for win8 and native brightness ctl is available, use that.
-		 *
-		 * The native check deliberately is inside the if acpi-video
-		 * block on older devices without acpi-video support native
-		 * is usually not the best choice.
-		 */
-		if (acpi_osi_is_win8() && native_available)
-			return acpi_backlight_native;
-		else
-			return acpi_backlight_video;
-	}
+	/* Use native if available */
+	if (native_available && prefer_native_over_acpi_video())
+		return acpi_backlight_native;
 
 	/* No ACPI video (old hw), use vendor specific fw methods. */
 	return acpi_backlight_vendor;
-- 
2.37.3

[RFC 2/2] ACPI: video: Prefer native over vendor

[Hans de Goede]

When available prefer native backlight control over vendor backlight
control.

Testing has shown that there are quite a few laptop models which rely
on native backlight control (they don't have ACPI video bus backlight
control) and on which acpi_osi_is_win8() returns false.

Currently __acpi_video_get_backlight_type() returns vendor on these
laptops, leading to an empty /sys/class/backlight.

As a workaround for this acpi_video_backlight_use_native() has been
temporarily changed to always return true.

This re-introduces the problem of having multiple backlight
devices under /sys/class/backlight for a single panel.

Change __acpi_video_get_backlight_type() to prefer native over vendor
when available. So that it returns native on these models.

And change acpi_video_backlight_use_native() back to only return
true when __acpi_video_get_backlight_type() returns native.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/video_detect.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index ec7421a3986f..19e9bc25225d 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -748,10 +748,10 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native)
 		return acpi_backlight_video;
 
 	/* Use native if available */
-	if (native_available && prefer_native_over_acpi_video())
+	if (native_available)
 		return acpi_backlight_native;
 
-	/* No ACPI video (old hw), use vendor specific fw methods. */
+	/* No ACPI video/native (old hw), use vendor specific fw methods. */
 	return acpi_backlight_vendor;
 }
 
@@ -763,18 +763,6 @@ EXPORT_SYMBOL(acpi_video_get_backlight_type);
 
 bool acpi_video_backlight_use_native(void)
 {
-	/*
-	 * Call __acpi_video_get_backlight_type() to let it know that
-	 * a native backlight is available.
-	 */
-	__acpi_video_get_backlight_type(true);
-
-	/*
-	 * For now just always return true. There is a whole bunch of laptop
-	 * models where (video_caps & ACPI_VIDEO_BACKLIGHT) is false causing
-	 * __acpi_video_get_backlight_type() to return vendor, while these
-	 * models only have a native backlight control.
-	 */
-	return true;
+	return __acpi_video_get_backlight_type(true) == acpi_backlight_native;
 }
 EXPORT_SYMBOL(acpi_video_backlight_use_native);
-- 
2.37.3

Re: [RFC 0/2] ACPI: video: prefer native over vendor

[Hans de Goede]

Hi,

On 11/5/22 15:52, Hans de Goede wrote:
> Hi Rafael, Matthew,
> 
> Here is a second attempt at always registering only a single
> /sys/class/backlight device per panel.
> 
> This first round of testing has shown that native works well even on
> systems so old that the don't have acpi_video backlight control support.
> 
> This patch series makes native be preferred over vendor, which should
> avoid the problems seen with the 6.1 changes before the fixes.
> 
> ATM there is one known model where this will cause a regression,
> the Sony Vaio PCG-FRV3 from 2003. I plan to add a DMI quirk for that
> in the next version of this series, but I'm waiting for some more
> testing (to check that the vendor interface does actually work) first.
> 
> I will also do another blogpost, focussing on asking users to see
> if they have a laptop which provides a combination of vendor + native
> backlight interfaces, which may be impacted by this series. This is
> the main reason why this is a RFC for now.

The blogpost requesting testing of laptops with a combination
of vendor + native backlight interfaces can be found here:

https://hansdegoede.dreamwidth.org/27024.html

Regards,

Hans

Re: [RFC 0/2] ACPI: video: prefer native over vendor

[Rafael J. Wysocki]

On Sat, Nov 5, 2022 at 4:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/5/22 15:52, Hans de Goede wrote:
> > Hi Rafael, Matthew,
> >
> > Here is a second attempt at always registering only a single
> > /sys/class/backlight device per panel.
> >
> > This first round of testing has shown that native works well even on
> > systems so old that the don't have acpi_video backlight control support.
> >
> > This patch series makes native be preferred over vendor, which should
> > avoid the problems seen with the 6.1 changes before the fixes.
> >
> > ATM there is one known model where this will cause a regression,
> > the Sony Vaio PCG-FRV3 from 2003. I plan to add a DMI quirk for that
> > in the next version of this series, but I'm waiting for some more
> > testing (to check that the vendor interface does actually work) first.
> >
> > I will also do another blogpost, focussing on asking users to see
> > if they have a laptop which provides a combination of vendor + native
> > backlight interfaces, which may be impacted by this series. This is
> > the main reason why this is a RFC for now.
>
> The blogpost requesting testing of laptops with a combination
> of vendor + native backlight interfaces can be found here:
>
> https://hansdegoede.dreamwidth.org/27024.html

The patches in this series look reasonable to me, even though I'm not
sure if the assumption that the Windows 8 hardware certification
requirements were always followed is not overly optimistic.

Re: [RFC 0/2] ACPI: video: prefer native over vendor

[Hans de Goede]

Hi,

On 11/9/22 14:51, Rafael J. Wysocki wrote:
> On Sat, Nov 5, 2022 at 4:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 11/5/22 15:52, Hans de Goede wrote:
>>> Hi Rafael, Matthew,
>>>
>>> Here is a second attempt at always registering only a single
>>> /sys/class/backlight device per panel.
>>>
>>> This first round of testing has shown that native works well even on
>>> systems so old that the don't have acpi_video backlight control support.
>>>
>>> This patch series makes native be preferred over vendor, which should
>>> avoid the problems seen with the 6.1 changes before the fixes.
>>>
>>> ATM there is one known model where this will cause a regression,
>>> the Sony Vaio PCG-FRV3 from 2003. I plan to add a DMI quirk for that
>>> in the next version of this series, but I'm waiting for some more
>>> testing (to check that the vendor interface does actually work) first.
>>>
>>> I will also do another blogpost, focussing on asking users to see
>>> if they have a laptop which provides a combination of vendor + native
>>> backlight interfaces, which may be impacted by this series. This is
>>> the main reason why this is a RFC for now.
>>
>> The blogpost requesting testing of laptops with a combination
>> of vendor + native backlight interfaces can be found here:
>>
>> https://hansdegoede.dreamwidth.org/27024.html
> 
> The patches in this series look reasonable to me,

Thanks.

> even though I'm not
> sure if the assumption that the Windows 8 hardware certification
> requirements were always followed is not overly optimistic.

After the second patch in the series the only remaining
prefer_native_over_acpi_video() and thus the only acpi_osi_is_win8()
call is guarded by a (video_caps & ACPI_VIDEO_BACKLIGHT) check as before:

        if ((video_caps & ACPI_VIDEO_BACKLIGHT) &&
             !(native_available && prefer_native_over_acpi_video()))
                return acpi_backlight_video;

So even if the assumption is wrong, there is no behavior change
other then the intended behavior change already caused by the second
patch.

The last part of __acpi_video_get_backlight_type() which contains
the basic (non special case) heuristics looks like this after
this series:

        /* Use ACPI video if available, except when native should be preferred. */
        if ((video_caps & ACPI_VIDEO_BACKLIGHT) &&
             !(native_available && prefer_native_over_acpi_video()))
                return acpi_backlight_video;

        /* Use native if available */
        if (native_available)
                return acpi_backlight_native;

        /* No ACPI video/native (old hw), use vendor specific fw methods. */
        return acpi_backlight_vendor;

Which is also a bit more KISS / easier to follow then before and
I hope that this will work well.

Regards,

Hans

Re: [RFC 0/2] ACPI: video: prefer native over vendor

[Rafael J. Wysocki]

On Wed, Nov 9, 2022 at 3:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/9/22 14:51, Rafael J. Wysocki wrote:
> > On Sat, Nov 5, 2022 at 4:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 11/5/22 15:52, Hans de Goede wrote:
> >>> Hi Rafael, Matthew,
> >>>
> >>> Here is a second attempt at always registering only a single
> >>> /sys/class/backlight device per panel.
> >>>
> >>> This first round of testing has shown that native works well even on
> >>> systems so old that the don't have acpi_video backlight control support.
> >>>
> >>> This patch series makes native be preferred over vendor, which should
> >>> avoid the problems seen with the 6.1 changes before the fixes.
> >>>
> >>> ATM there is one known model where this will cause a regression,
> >>> the Sony Vaio PCG-FRV3 from 2003. I plan to add a DMI quirk for that
> >>> in the next version of this series, but I'm waiting for some more
> >>> testing (to check that the vendor interface does actually work) first.
> >>>
> >>> I will also do another blogpost, focussing on asking users to see
> >>> if they have a laptop which provides a combination of vendor + native
> >>> backlight interfaces, which may be impacted by this series. This is
> >>> the main reason why this is a RFC for now.
> >>
> >> The blogpost requesting testing of laptops with a combination
> >> of vendor + native backlight interfaces can be found here:
> >>
> >> https://hansdegoede.dreamwidth.org/27024.html
> >
> > The patches in this series look reasonable to me,
>
> Thanks.
>
> > even though I'm not
> > sure if the assumption that the Windows 8 hardware certification
> > requirements were always followed is not overly optimistic.
>
> After the second patch in the series the only remaining
> prefer_native_over_acpi_video() and thus the only acpi_osi_is_win8()
> call is guarded by a (video_caps & ACPI_VIDEO_BACKLIGHT) check as before:
>
>         if ((video_caps & ACPI_VIDEO_BACKLIGHT) &&
>              !(native_available && prefer_native_over_acpi_video()))
>                 return acpi_backlight_video;
>
> So even if the assumption is wrong, there is no behavior change
> other then the intended behavior change already caused by the second
> patch.
>
> The last part of __acpi_video_get_backlight_type() which contains
> the basic (non special case) heuristics looks like this after
> this series:
>
>         /* Use ACPI video if available, except when native should be preferred. */
>         if ((video_caps & ACPI_VIDEO_BACKLIGHT) &&
>              !(native_available && prefer_native_over_acpi_video()))
>                 return acpi_backlight_video;
>
>         /* Use native if available */
>         if (native_available)
>                 return acpi_backlight_native;
>
>         /* No ACPI video/native (old hw), use vendor specific fw methods. */
>         return acpi_backlight_vendor;
>
> Which is also a bit more KISS / easier to follow then before and
> I hope that this will work well.

Fair enough.