[PATCH 1/2] ACPI: utils: Add acpi_reduced_hardware() helper

[PATCH 1/2] ACPI: utils: Add acpi_reduced_hardware() helper

[Hans de Goede]

Add a getter for the acpi_gbl_reduced_hardware variable so that modules
can check if they are running on an ACPI reduced-hw platform or not.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/utils.c    | 11 +++++++++++
 include/acpi/acpi_bus.h |  1 +
 include/linux/acpi.h    |  5 +++++
 3 files changed, 17 insertions(+)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 682edd913b3b..4cb061d3169a 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -872,6 +872,17 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
 }
 EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
 
+/**
+ * acpi_reduced_hardware - Return if this is an ACPI-reduced-hw machine
+ *
+ * Return true when running on an ACPI-reduced-hw machine, false otherwise.
+ */
+bool acpi_reduced_hardware(void)
+{
+	return acpi_gbl_reduced_hardware;
+}
+EXPORT_SYMBOL(acpi_reduced_hardware);
+
 /*
  * acpi_backlight= handling, this is done here rather then in video_detect.c
  * because __setup cannot be used in modules.
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index f28b097c658f..d631cb52283e 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -78,6 +78,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const guid_t *guid, u64 rev,
 
 bool acpi_dev_found(const char *hid);
 bool acpi_dev_present(const char *hid, const char *uid, s64 hrv);
+bool acpi_reduced_hardware(void);
 
 #ifdef CONFIG_ACPI
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 3bdcfc4401b7..e2e6db8313c8 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -748,6 +748,11 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
 	return NULL;
 }
 
+static inline bool acpi_reduced_hardware(void)
+{
+	return false;
+}
+
 static inline void acpi_dev_put(struct acpi_device *adev) {}
 
 static inline bool is_acpi_node(const struct fwnode_handle *fwnode)
-- 
2.30.2

[PATCH 2/2] ACPI: video: Check LCD flag on ACPI-reduced-hardware devices

[Hans de Goede]

Starting with Windows 8, Windows no longer uses the ACPI-video interface
for backlight control by default. Instead backlight control is left up
to the GPU drivers and these are typically directly accessing the GPU
for this instead of going through ACPI.

This means that the ACPI video interface is no longer being tested by
many vendors, which leads to false-positive /sys/class/backlight entries
on devices which don't have a backlight at all such as desktops or
top-set boxes. These false-positives causes desktop environments to show
a non functional brightness slider in various places.

Checking the LCD flag greatly reduces the amount of false-positives,
so commit 5928c281524f ("ACPI / video: Default lcd_only to true on
Win8-ready and newer machines") enabled the checking of this flag
by default on all win8 BIOS-es. But this let to regressions on some
models, so the check was made stricter adding a DMI chassis-type check
to only enable the LCD flag checking on desktop/server chassis.

Unfortunately the chassis-type reported in the DMI strings is not always
reliable. One class of devices where this is a problem is Intel Bay Trail-T
based top-set boxes / mini PCs / HDMI sticks. These are based on reference
designs which were targetets and the reference design BIOS code
is often used without changing the chassis-type to something more
appropriate.

There are many, many Bay Trail-T based devices affected by this, so DMI
quirking our way out of this is a bad idea. This patch takes a different
approach, Bay Trail-T (unlike regular Bay Trail) is an ACPI-reduced-hw
platform and ACPI-reduced-hw platforms generally don't have
an embedded-controller and thus will use a native (GPU specific) backlight
interface. This patch enables Checking the LCD flag by default on
ACPI-reduced-hw platforms with a win8 BIOS independent of the reported
chassis-type, fixing the false positive /sys/class/backlight entries
on these devices.

Note in hindsight I should have never added the DMI chassis-type check
when the enabling of LCD flag checking on Windows 8 BIOS-es let to some
regressions. Instead I should have added DMI quirks for the (presumably
few) models where the LCD flag check let to issues. But I'm afraid that
it is too late to change this now, changing this now will likely lead to
a bunch of regressions.

This patch was tested on a Mele PCG03 mini PC.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_video.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 2ea1781290cc..ade02152bb06 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -2182,6 +2182,30 @@ static bool dmi_is_desktop(void)
 	return false;
 }
 
+/*
+ * We're seeing a lot of bogus backlight interfaces on newer machines
+ * without a LCD such as desktops, servers and HDMI sticks. Checking the
+ * lcd flag fixes this, enable this by default on any machines which are:
+ * 1.  Win8 ready (where we also prefer the native backlight driver, so
+ *     normally the acpi_video code should not register there anyways); *and*
+ * 2.1 Report a desktop/server DMI chassis-type, or
+ * 2.2 Are an ACPI-reduced-hardware platform (and thus won't use the EC for
+       backlight control)
+ */
+static bool should_check_lcd_flag(void)
+{
+	if (!acpi_osi_is_win8())
+		return false;
+
+	if (dmi_is_desktop())
+		return true;
+
+	if (acpi_reduced_hardware())
+		return true;
+
+	return false;
+}
+
 int acpi_video_register(void)
 {
 	int ret = 0;
@@ -2195,19 +2219,8 @@ int acpi_video_register(void)
 		goto leave;
 	}
 
-	/*
-	 * We're seeing a lot of bogus backlight interfaces on newer machines
-	 * without a LCD such as desktops, servers and HDMI sticks. Checking
-	 * the lcd flag fixes this, so enable this on any machines which are
-	 * win8 ready (where we also prefer the native backlight driver, so
-	 * normally the acpi_video code should not register there anyways).
-	 */
-	if (only_lcd == -1) {
-		if (dmi_is_desktop() && acpi_osi_is_win8())
-			only_lcd = true;
-		else
-			only_lcd = false;
-	}
+	if (only_lcd == -1)
+		only_lcd = should_check_lcd_flag();
 
 	dmi_check_system(video_dmi_table);
 
-- 
2.30.2

Re: [PATCH 1/2] ACPI: utils: Add acpi_reduced_hardware() helper

[Rafael J. Wysocki]

On Tue, Apr 6, 2021 at 11:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add a getter for the acpi_gbl_reduced_hardware variable so that modules
> can check if they are running on an ACPI reduced-hw platform or not.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/utils.c    | 11 +++++++++++
>  include/acpi/acpi_bus.h |  1 +
>  include/linux/acpi.h    |  5 +++++
>  3 files changed, 17 insertions(+)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 682edd913b3b..4cb061d3169a 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -872,6 +872,17 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
>  }
>  EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
>
> +/**
> + * acpi_reduced_hardware - Return if this is an ACPI-reduced-hw machine
> + *
> + * Return true when running on an ACPI-reduced-hw machine, false otherwise.
> + */
> +bool acpi_reduced_hardware(void)
> +{
> +       return acpi_gbl_reduced_hardware;
> +}
> +EXPORT_SYMBOL(acpi_reduced_hardware);

EXPORT_SYMBOL_GPL()?

> +
>  /*
>   * acpi_backlight= handling, this is done here rather then in video_detect.c
>   * because __setup cannot be used in modules.
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index f28b097c658f..d631cb52283e 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -78,6 +78,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const guid_t *guid, u64 rev,
>
>  bool acpi_dev_found(const char *hid);
>  bool acpi_dev_present(const char *hid, const char *uid, s64 hrv);
> +bool acpi_reduced_hardware(void);
>
>  #ifdef CONFIG_ACPI
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 3bdcfc4401b7..e2e6db8313c8 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -748,6 +748,11 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
>         return NULL;
>  }
>
> +static inline bool acpi_reduced_hardware(void)
> +{
> +       return false;
> +}
> +
>  static inline void acpi_dev_put(struct acpi_device *adev) {}
>
>  static inline bool is_acpi_node(const struct fwnode_handle *fwnode)
> --
> 2.30.2
>

Re: [PATCH 1/2] ACPI: utils: Add acpi_reduced_hardware() helper

[Hans de Goede]

Hi,

On 4/7/21 7:13 PM, Rafael J. Wysocki wrote:
> On Tue, Apr 6, 2021 at 11:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Add a getter for the acpi_gbl_reduced_hardware variable so that modules
>> can check if they are running on an ACPI reduced-hw platform or not.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/acpi/utils.c    | 11 +++++++++++
>>  include/acpi/acpi_bus.h |  1 +
>>  include/linux/acpi.h    |  5 +++++
>>  3 files changed, 17 insertions(+)
>>
>> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
>> index 682edd913b3b..4cb061d3169a 100644
>> --- a/drivers/acpi/utils.c
>> +++ b/drivers/acpi/utils.c
>> @@ -872,6 +872,17 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
>>  }
>>  EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
>>
>> +/**
>> + * acpi_reduced_hardware - Return if this is an ACPI-reduced-hw machine
>> + *
>> + * Return true when running on an ACPI-reduced-hw machine, false otherwise.
>> + */
>> +bool acpi_reduced_hardware(void)
>> +{
>> +       return acpi_gbl_reduced_hardware;
>> +}
>> +EXPORT_SYMBOL(acpi_reduced_hardware);
> 
> EXPORT_SYMBOL_GPL()?

Yes, that was my intention, no idea what happened here.

Before I send a v2, do you have any remarks on patch 2/2 (which is actually
the more interesting patch) ?

Regards,

Hans







> 
>> +
>>  /*
>>   * acpi_backlight= handling, this is done here rather then in video_detect.c
>>   * because __setup cannot be used in modules.
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index f28b097c658f..d631cb52283e 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -78,6 +78,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const guid_t *guid, u64 rev,
>>
>>  bool acpi_dev_found(const char *hid);
>>  bool acpi_dev_present(const char *hid, const char *uid, s64 hrv);
>> +bool acpi_reduced_hardware(void);
>>
>>  #ifdef CONFIG_ACPI
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 3bdcfc4401b7..e2e6db8313c8 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -748,6 +748,11 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
>>         return NULL;
>>  }
>>
>> +static inline bool acpi_reduced_hardware(void)
>> +{
>> +       return false;
>> +}
>> +
>>  static inline void acpi_dev_put(struct acpi_device *adev) {}
>>
>>  static inline bool is_acpi_node(const struct fwnode_handle *fwnode)
>> --
>> 2.30.2
>>
> 

Re: [PATCH 1/2] ACPI: utils: Add acpi_reduced_hardware() helper

[Rafael J. Wysocki]

On Wed, Apr 7, 2021 at 7:43 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 4/7/21 7:13 PM, Rafael J. Wysocki wrote:
> > On Tue, Apr 6, 2021 at 11:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Add a getter for the acpi_gbl_reduced_hardware variable so that modules
> >> can check if they are running on an ACPI reduced-hw platform or not.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/acpi/utils.c    | 11 +++++++++++
> >>  include/acpi/acpi_bus.h |  1 +
> >>  include/linux/acpi.h    |  5 +++++
> >>  3 files changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> >> index 682edd913b3b..4cb061d3169a 100644
> >> --- a/drivers/acpi/utils.c
> >> +++ b/drivers/acpi/utils.c
> >> @@ -872,6 +872,17 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
> >>  }
> >>  EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
> >>
> >> +/**
> >> + * acpi_reduced_hardware - Return if this is an ACPI-reduced-hw machine
> >> + *
> >> + * Return true when running on an ACPI-reduced-hw machine, false otherwise.
> >> + */
> >> +bool acpi_reduced_hardware(void)
> >> +{
> >> +       return acpi_gbl_reduced_hardware;
> >> +}
> >> +EXPORT_SYMBOL(acpi_reduced_hardware);
> >
> > EXPORT_SYMBOL_GPL()?
>
> Yes, that was my intention, no idea what happened here.
>
> Before I send a v2, do you have any remarks on patch 2/2 (which is actually
> the more interesting patch) ?

I thought that basing that check on the ACPICA's global variable may
be too coarse grained for some cases, but then I've decided to do it
as is now and we'll see.

No need to resend that one.

Re: [PATCH 1/2] ACPI: utils: Add acpi_reduced_hardware() helper

[Hans de Goede]

Hi,

On 4/7/21 7:50 PM, Rafael J. Wysocki wrote:
> On Wed, Apr 7, 2021 at 7:43 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 4/7/21 7:13 PM, Rafael J. Wysocki wrote:
>>> On Tue, Apr 6, 2021 at 11:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Add a getter for the acpi_gbl_reduced_hardware variable so that modules
>>>> can check if they are running on an ACPI reduced-hw platform or not.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/acpi/utils.c    | 11 +++++++++++
>>>>  include/acpi/acpi_bus.h |  1 +
>>>>  include/linux/acpi.h    |  5 +++++
>>>>  3 files changed, 17 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
>>>> index 682edd913b3b..4cb061d3169a 100644
>>>> --- a/drivers/acpi/utils.c
>>>> +++ b/drivers/acpi/utils.c
>>>> @@ -872,6 +872,17 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
>>>>  }
>>>>  EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
>>>>
>>>> +/**
>>>> + * acpi_reduced_hardware - Return if this is an ACPI-reduced-hw machine
>>>> + *
>>>> + * Return true when running on an ACPI-reduced-hw machine, false otherwise.
>>>> + */
>>>> +bool acpi_reduced_hardware(void)
>>>> +{
>>>> +       return acpi_gbl_reduced_hardware;
>>>> +}
>>>> +EXPORT_SYMBOL(acpi_reduced_hardware);
>>>
>>> EXPORT_SYMBOL_GPL()?
>>
>> Yes, that was my intention, no idea what happened here.

I just prepped and send out v2 and I think I know what happened, all
the other functions in drivers/acpi/utils.c are EXPORT_SYMBOL, so I probably
just copy-and-pasted this without too much thinking.

It might be worthwhile to see if we should also mark some other functions
as EXPORT_SYMBOL_GPL() here.

>> Before I send a v2, do you have any remarks on patch 2/2 (which is actually
>> the more interesting patch) ?
> 
> I thought that basing that check on the ACPICA's global variable may
> be too coarse grained for some cases, but then I've decided to do it
> as is now and we'll see.

Yes, the whole code for selecting which backlight driver to use is mostly
heuristics, so "we'll see" indeed . With that said I'm pretty confident
that this change should not cause problems. Platforms which actually
set acpi_gbl_reduced_hardware=true seem to be quite rare. I'm actually
only aware of Bay Trail-T based devices doing this.

Regards,

Hans