[PATCH 0/2] ACPI: video: Fix disable_backlight_sysfs_if regression

[PATCH 0/2] ACPI: video: Fix disable_backlight_sysfs_if regression

[Hans de Goede]

Hi Rafael,

Last week I realized that my backlight detect refactor series, which
makes sure we only ever register 1 backlight device, breaks the
video.disable_backlight_sysfs_if module-param / quirk which is necessary
to turn the backlight back on after a suspend / resume cycle on some
Toshiba models.

This workaround for some Toshiba models relied (in a way) on the old
behavior of registering both the acpi and intel backlight class devices
in case where acpi_video_get_backlight_type() returned "video".

Luckily recently Arvid has started contributing some toshiba_acpi driver
improvements and he has an affected Toshiba model. So with the help
of Arvid I have been able to come up with a fix for this by pushing
the magic firmware call these devices need on resume into toshiba_acpi.

After which the whole (no longer functional) disable_backlight_sysfs_if
module-parameter can simply be replaced by "standard" acpi_backlight=native
DMI quirks for the affected models.

I can take care of merging this on top of the backlight-detect-refactor-1
merge already done in the pdx86 tree.

Rafael, may I have your ack for merging patch 2/2 through the pdx86 tree?

Regards,

Hans


Hans de Goede (2):
  drivers/platform: toshiba_acpi: Call HCI_PANEL_POWER_ON on resume on
    some models
  ACPI: video: Change disable_backlight_sysfs_if quirks to
    acpi_backlight=native

 drivers/acpi/acpi_video.c           | 48 -----------------------------
 drivers/acpi/video_detect.c         | 35 +++++++++++++++++++++
 drivers/platform/x86/toshiba_acpi.c | 46 +++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 48 deletions(-)

-- 
2.36.1

[PATCH 1/2] drivers/platform: toshiba_acpi: Call HCI_PANEL_POWER_ON on resume on some models

[Hans de Goede]

Some Toshibas have a broken acpi-video interface for brightness control, so
far these have been using a special workaround in drivers/acpi/acpi_video.c
which gets activated by the disable_backlight_sysfs_if module-param/quirks.

The recent x86/acpi backlight refactoring has broken this workaround:
1. This workaround relies on acpi_video_get_backlight_type() returning
   acpi_video so that the acpi_video code actually runs; and
2. this relies on the actual native GPU driver to offer the sysfs
   backlight interface to userspace.

After the refactor this breaks since the native driver will no
longer register its backlight-device if acpi_video_get_backlight_type()
does not return native and making it return native breaks 1.

Keeping the acpi_video backlight handling on resume active, while not
using it to set the brightness, is necessary because it does a _BCM
call on resume which is necessary to turn the panel back on on resume.

Looking at the DSDT shows that this _BCM call results in a Toshiba
HCI_SET HCI_LCD_BRIGHTNESS call, which turns the panel back on.

This commit makes toshiba_acpi do a HCI_SET HCI_PANEL_POWER_ON call
on resume on the affected models, so that the (now broken)
acpi_video disable_backlight_sysfs_if workaround will no longer
be necessary.

Note this uses HCI_PANEL_POWER_ON instead of HCI_LCD_BRIGHTNESS
to avoid changing the configured brightness level.

Fixes: b1d36e73cc1c ("drm/i915: Don't register backlight when another backlight should be used (v2)")
Tested-by: Arvid Norlander <lkml@vorpal.se>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/toshiba_acpi.c | 46 +++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 030dc37d50b8..826ffac4af1c 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -23,6 +23,7 @@
 #define PROC_INTERFACE_VERSION	1
 
 #include <linux/compiler.h>
+#include <linux/dmi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -100,6 +101,7 @@ MODULE_LICENSE("GPL");
 #define TOS_NOT_INSTALLED		0x8e00
 
 /* Registers */
+#define HCI_PANEL_POWER_ON		0x0002
 #define HCI_FAN				0x0004
 #define HCI_TR_BACKLIGHT		0x0005
 #define HCI_SYSTEM_EVENT		0x0016
@@ -206,6 +208,7 @@ struct toshiba_acpi_dev {
 
 	bool kbd_event_generated;
 	bool killswitch;
+	bool turn_on_panel_on_resume;
 };
 
 static struct toshiba_acpi_dev *toshiba_acpi;
@@ -2999,6 +3002,43 @@ static const char *find_hci_method(acpi_handle handle)
 	return NULL;
 }
 
+/*
+ * Some Toshibas have a broken acpi-video interface for brightness control,
+ * these are quirked in drivers/acpi/video_detect.c to use the GPU native
+ * (/sys/class/backlight/intel_backlight) instead.
+ * But these need a HCI_SET call to actually turn the panel back on at resume,
+ * without this call the screen stays black at resume.
+ * Either HCI_LCD_BRIGHTNESS (used by acpi_video's _BCM) or HCI_PANEL_POWER_ON
+ * works. toshiba_acpi_resume() uses HCI_PANEL_POWER_ON to avoid changing
+ * the configured brightness level.
+ */
+static const struct dmi_system_id turn_on_panel_on_resume_dmi_ids[] = {
+	{
+	 /* Toshiba Portégé R700 */
+	 /* https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE R700"),
+		},
+	},
+	{
+	 /* Toshiba Satellite/Portégé R830 */
+	 /* Portégé: https://bugs.freedesktop.org/show_bug.cgi?id=82634 */
+	 /* Satellite: https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "R830"),
+		},
+	},
+	{
+	 /* Toshiba Satellite/Portégé Z830 */
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Z830"),
+		},
+	},
+};
+
 static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 {
 	struct toshiba_acpi_dev *dev;
@@ -3141,6 +3181,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 	ret = get_fan_status(dev, &dummy);
 	dev->fan_supported = !ret;
 
+	dev->turn_on_panel_on_resume =
+		dmi_check_system(turn_on_panel_on_resume_dmi_ids);
+
 	toshiba_wwan_available(dev);
 	if (dev->wwan_supported)
 		toshiba_acpi_setup_wwan_rfkill(dev);
@@ -3257,6 +3300,9 @@ static int toshiba_acpi_resume(struct device *device)
 			rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch);
 	}
 
+	if (dev->turn_on_panel_on_resume)
+		hci_write(dev, HCI_PANEL_POWER_ON, 1);
+
 	return 0;
 }
 #endif
-- 
2.36.1

[PATCH 2/2] ACPI: video: Change disable_backlight_sysfs_if quirks to acpi_backlight=native

[Hans de Goede]

Some Toshibas have a broken acpi-video interface for brightness control
and need a special firmware call on resume to turn the panel back on.
So far these have been using the disable_backlight_sysfs_if workaround
to deal with this.

The recent x86/acpi backlight refactoring has broken this workaround:
1. This workaround relies on acpi_video_get_backlight_type() returning
   acpi_video so that the acpi_video code actually runs; and
2. this relies on the actual native GPU driver to offer the sysfs
   backlight interface to userspace.

After the refactor this breaks since the native driver will no
longer register its backlight-device if acpi_video_get_backlight_type()
does not return native and making it return native breaks 1.

Keeping the acpi_video backlight handling on resume active, while not
using it to set the brightness, is necessary because it does a _BCM
call on resume which is necessary to turn the panel back on on resume.

Looking at the DSDT shows that this _BCM call results in a Toshiba
HCI_SET HCI_LCD_BRIGHTNESS call, which turns the panel back on.

This kind of special vendor specific handling really belongs in
the vendor specific acpi driver. An earlier patch in this series
modifies toshiba_acpi to make the necessary HCI_SET call on resume
on affected models.

With toshiba_acpi taking care of the HCI_SET call on resume,
the acpi_video code no longer needs to call _BCM on resume.

So instead of using the (now broken) disable_backlight_sysfs_if
workaround, simply setting acpi_backlight=native to disable
the broken apci-video interface is sufficient fix things now.

After this there are no more users of the disable_backlight_sysfs_if
flag and as discussed above the flag also no longer works as intended,
so remove the disable_backlight_sysfs_if flag entirely.

Fixes: b1d36e73cc1c ("drm/i915: Don't register backlight when another backlight should be used (v2)")
Tested-by: Arvid Norlander <lkml@vorpal.se>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_video.c   | 48 -------------------------------------
 drivers/acpi/video_detect.c | 35 +++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 48 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index a7c3d11e0dac..99894fbcf4e3 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -47,9 +47,6 @@ module_param(brightness_switch_enabled, bool, 0644);
 static bool allow_duplicates;
 module_param(allow_duplicates, bool, 0644);
 
-static int disable_backlight_sysfs_if = -1;
-module_param(disable_backlight_sysfs_if, int, 0444);
-
 #define REPORT_OUTPUT_KEY_EVENTS		0x01
 #define REPORT_BRIGHTNESS_KEY_EVENTS		0x02
 static int report_key_events = -1;
@@ -394,14 +391,6 @@ static int video_set_bqc_offset(const struct dmi_system_id *d)
 	return 0;
 }
 
-static int video_disable_backlight_sysfs_if(
-	const struct dmi_system_id *d)
-{
-	if (disable_backlight_sysfs_if == -1)
-		disable_backlight_sysfs_if = 1;
-	return 0;
-}
-
 static int video_set_device_id_scheme(const struct dmi_system_id *d)
 {
 	device_id_scheme = true;
@@ -474,40 +463,6 @@ static const struct dmi_system_id video_dmi_table[] = {
 		},
 	},
 
-	/*
-	 * Some machines have a broken acpi-video interface for brightness
-	 * control, but still need an acpi_video_device_lcd_set_level() call
-	 * on resume to turn the backlight power on.  We Enable backlight
-	 * control on these systems, but do not register a backlight sysfs
-	 * as brightness control does not work.
-	 */
-	{
-	 /* https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
-	 .callback = video_disable_backlight_sysfs_if,
-	 .ident = "Toshiba Portege R700",
-	 .matches = {
-		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
-		DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE R700"),
-		},
-	},
-	{
-	 /* https://bugs.freedesktop.org/show_bug.cgi?id=82634 */
-	 .callback = video_disable_backlight_sysfs_if,
-	 .ident = "Toshiba Portege R830",
-	 .matches = {
-		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
-		DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE R830"),
-		},
-	},
-	{
-	 /* https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
-	 .callback = video_disable_backlight_sysfs_if,
-	 .ident = "Toshiba Satellite R830",
-	 .matches = {
-		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
-		DMI_MATCH(DMI_PRODUCT_NAME, "SATELLITE R830"),
-		},
-	},
 	/*
 	 * Some machine's _DOD IDs don't have bit 31(Device ID Scheme) set
 	 * but the IDs actually follow the Device ID Scheme.
@@ -1770,9 +1725,6 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
 	if (result)
 		return;
 
-	if (disable_backlight_sysfs_if > 0)
-		return;
-
 	name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
 	if (!name)
 		return;
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index db2474fe58ac..0d9064a9804c 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -610,6 +610,41 @@ static const struct dmi_system_id video_detect_dmi_table[] = {
 		},
 	},
 
+	/*
+	 * These Toshibas have a broken acpi-video interface for brightness
+	 * control. They also have an issue where the panel is off after
+	 * suspend until a special firmware call is made to turn it back
+	 * on. This is handled by the toshiba_acpi kernel module, so that
+	 * module must be enabled for these models to work correctly.
+	 */
+	{
+	 /* https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
+	 .callback = video_detect_force_native,
+	 /* Toshiba Portégé R700 */
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE R700"),
+		},
+	},
+	{
+	 /* Portégé: https://bugs.freedesktop.org/show_bug.cgi?id=82634 */
+	 /* Satellite: https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
+	 .callback = video_detect_force_native,
+	 /* Toshiba Satellite/Portégé R830 */
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "R830"),
+		},
+	},
+	{
+	 .callback = video_detect_force_native,
+	 /* Toshiba Satellite/Portégé Z830 */
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Z830"),
+		},
+	},
+
 	/*
 	 * Desktops which falsely report a backlight and which our heuristics
 	 * for this do not catch.
-- 
2.36.1

Re: [PATCH 1/2] drivers/platform: toshiba_acpi: Call HCI_PANEL_POWER_ON on resume on some models

[Arvid Norlander]

Hi,

On 2022-09-05 11:00, Hans de Goede wrote:
> Some Toshibas have a broken acpi-video interface for brightness control, so
> far these have been using a special workaround in drivers/acpi/acpi_video.c
> which gets activated by the disable_backlight_sysfs_if module-param/quirks.
> 
> The recent x86/acpi backlight refactoring has broken this workaround:
> 1. This workaround relies on acpi_video_get_backlight_type() returning
>    acpi_video so that the acpi_video code actually runs; and
> 2. this relies on the actual native GPU driver to offer the sysfs
>    backlight interface to userspace.
> 
> After the refactor this breaks since the native driver will no
> longer register its backlight-device if acpi_video_get_backlight_type()
> does not return native and making it return native breaks 1.
> 
> Keeping the acpi_video backlight handling on resume active, while not
> using it to set the brightness, is necessary because it does a _BCM
> call on resume which is necessary to turn the panel back on on resume.
> 
> Looking at the DSDT shows that this _BCM call results in a Toshiba
> HCI_SET HCI_LCD_BRIGHTNESS call, which turns the panel back on.
> 
> This commit makes toshiba_acpi do a HCI_SET HCI_PANEL_POWER_ON call
> on resume on the affected models, so that the (now broken)
> acpi_video disable_backlight_sysfs_if workaround will no longer
> be necessary.
> 
> Note this uses HCI_PANEL_POWER_ON instead of HCI_LCD_BRIGHTNESS
> to avoid changing the configured brightness level.

With the previous ACPI Video based approach this quirk was possible to
control from the kernel command line. This does not seem to be the case
here. This raises the difficulty for users with as of yet unlisted models
to test if this quirk would help. Would it be worth adding support for the
enabling this from the kernel command line?

> 
> Fixes: b1d36e73cc1c ("drm/i915: Don't register backlight when another backlight should be used (v2)")
> Tested-by: Arvid Norlander <lkml@vorpal.se>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/toshiba_acpi.c | 46 +++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 030dc37d50b8..826ffac4af1c 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -23,6 +23,7 @@
>  #define PROC_INTERFACE_VERSION	1
>  
>  #include <linux/compiler.h>
> +#include <linux/dmi.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> @@ -100,6 +101,7 @@ MODULE_LICENSE("GPL");
>  #define TOS_NOT_INSTALLED		0x8e00
>  
>  /* Registers */
> +#define HCI_PANEL_POWER_ON		0x0002
>  #define HCI_FAN				0x0004
>  #define HCI_TR_BACKLIGHT		0x0005
>  #define HCI_SYSTEM_EVENT		0x0016
> @@ -206,6 +208,7 @@ struct toshiba_acpi_dev {
>  
>  	bool kbd_event_generated;
>  	bool killswitch;
> +	bool turn_on_panel_on_resume;

You added this bool to the section that changes at runtime, rather than
the feature section just above.

Also, many of the bools are bitfields, especially (almost) all the ones
that are about detecting a feature once then setting it. (I belive
"special_functions" is an exception since it can take more values, and
when I add support for the non-working buttons on the 830 this will be
significant).

In summary I thus believe it would make sense to add your new boolean to
bitfield section above this one.

>  };
>  
>  static struct toshiba_acpi_dev *toshiba_acpi;
> @@ -2999,6 +3002,43 @@ static const char *find_hci_method(acpi_handle handle)
>  	return NULL;
>  }
>  
> +/*
> + * Some Toshibas have a broken acpi-video interface for brightness control,
> + * these are quirked in drivers/acpi/video_detect.c to use the GPU native
> + * (/sys/class/backlight/intel_backlight) instead.
> + * But these need a HCI_SET call to actually turn the panel back on at resume,
> + * without this call the screen stays black at resume.
> + * Either HCI_LCD_BRIGHTNESS (used by acpi_video's _BCM) or HCI_PANEL_POWER_ON
> + * works. toshiba_acpi_resume() uses HCI_PANEL_POWER_ON to avoid changing
> + * the configured brightness level.
> + */
> +static const struct dmi_system_id turn_on_panel_on_resume_dmi_ids[] = {
> +	{
> +	 /* Toshiba Portégé R700 */
> +	 /* https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE R700"),
> +		},
> +	},
> +	{
> +	 /* Toshiba Satellite/Portégé R830 */
> +	 /* Portégé: https://bugs.freedesktop.org/show_bug.cgi?id=82634 */
> +	 /* Satellite: https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "R830"),
> +		},
> +	},
> +	{
> +	 /* Toshiba Satellite/Portégé Z830 */
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "Z830"),
> +		},
> +	},
> +};
> +
>  static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  {
>  	struct toshiba_acpi_dev *dev;
> @@ -3141,6 +3181,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  	ret = get_fan_status(dev, &dummy);
>  	dev->fan_supported = !ret;
>  
> +	dev->turn_on_panel_on_resume =
> +		dmi_check_system(turn_on_panel_on_resume_dmi_ids);
> +
>  	toshiba_wwan_available(dev);
>  	if (dev->wwan_supported)
>  		toshiba_acpi_setup_wwan_rfkill(dev);
> @@ -3257,6 +3300,9 @@ static int toshiba_acpi_resume(struct device *device)
>  			rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch);
>  	}
>  
> +	if (dev->turn_on_panel_on_resume)
> +		hci_write(dev, HCI_PANEL_POWER_ON, 1);
> +
>  	return 0;
>  }
>  #endif

Best regards,
Arvid Norlander

Re: [PATCH 2/2] ACPI: video: Change disable_backlight_sysfs_if quirks to acpi_backlight=native

[Rafael J. Wysocki]

On Mon, Sep 5, 2022 at 11:00 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Some Toshibas have a broken acpi-video interface for brightness control
> and need a special firmware call on resume to turn the panel back on.
> So far these have been using the disable_backlight_sysfs_if workaround
> to deal with this.
>
> The recent x86/acpi backlight refactoring has broken this workaround:
> 1. This workaround relies on acpi_video_get_backlight_type() returning
>    acpi_video so that the acpi_video code actually runs; and
> 2. this relies on the actual native GPU driver to offer the sysfs
>    backlight interface to userspace.
>
> After the refactor this breaks since the native driver will no
> longer register its backlight-device if acpi_video_get_backlight_type()
> does not return native and making it return native breaks 1.
>
> Keeping the acpi_video backlight handling on resume active, while not
> using it to set the brightness, is necessary because it does a _BCM
> call on resume which is necessary to turn the panel back on on resume.
>
> Looking at the DSDT shows that this _BCM call results in a Toshiba
> HCI_SET HCI_LCD_BRIGHTNESS call, which turns the panel back on.
>
> This kind of special vendor specific handling really belongs in
> the vendor specific acpi driver. An earlier patch in this series
> modifies toshiba_acpi to make the necessary HCI_SET call on resume
> on affected models.
>
> With toshiba_acpi taking care of the HCI_SET call on resume,
> the acpi_video code no longer needs to call _BCM on resume.
>
> So instead of using the (now broken) disable_backlight_sysfs_if
> workaround, simply setting acpi_backlight=native to disable
> the broken apci-video interface is sufficient fix things now.
>
> After this there are no more users of the disable_backlight_sysfs_if
> flag and as discussed above the flag also no longer works as intended,
> so remove the disable_backlight_sysfs_if flag entirely.
>
> Fixes: b1d36e73cc1c ("drm/i915: Don't register backlight when another backlight should be used (v2)")
> Tested-by: Arvid Norlander <lkml@vorpal.se>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/acpi_video.c   | 48 -------------------------------------
>  drivers/acpi/video_detect.c | 35 +++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index a7c3d11e0dac..99894fbcf4e3 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -47,9 +47,6 @@ module_param(brightness_switch_enabled, bool, 0644);
>  static bool allow_duplicates;
>  module_param(allow_duplicates, bool, 0644);
>
> -static int disable_backlight_sysfs_if = -1;
> -module_param(disable_backlight_sysfs_if, int, 0444);
> -
>  #define REPORT_OUTPUT_KEY_EVENTS               0x01
>  #define REPORT_BRIGHTNESS_KEY_EVENTS           0x02
>  static int report_key_events = -1;
> @@ -394,14 +391,6 @@ static int video_set_bqc_offset(const struct dmi_system_id *d)
>         return 0;
>  }
>
> -static int video_disable_backlight_sysfs_if(
> -       const struct dmi_system_id *d)
> -{
> -       if (disable_backlight_sysfs_if == -1)
> -               disable_backlight_sysfs_if = 1;
> -       return 0;
> -}
> -
>  static int video_set_device_id_scheme(const struct dmi_system_id *d)
>  {
>         device_id_scheme = true;
> @@ -474,40 +463,6 @@ static const struct dmi_system_id video_dmi_table[] = {
>                 },
>         },
>
> -       /*
> -        * Some machines have a broken acpi-video interface for brightness
> -        * control, but still need an acpi_video_device_lcd_set_level() call
> -        * on resume to turn the backlight power on.  We Enable backlight
> -        * control on these systems, but do not register a backlight sysfs
> -        * as brightness control does not work.
> -        */
> -       {
> -        /* https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
> -        .callback = video_disable_backlight_sysfs_if,
> -        .ident = "Toshiba Portege R700",
> -        .matches = {
> -               DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> -               DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE R700"),
> -               },
> -       },
> -       {
> -        /* https://bugs.freedesktop.org/show_bug.cgi?id=82634 */
> -        .callback = video_disable_backlight_sysfs_if,
> -        .ident = "Toshiba Portege R830",
> -        .matches = {
> -               DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> -               DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE R830"),
> -               },
> -       },
> -       {
> -        /* https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
> -        .callback = video_disable_backlight_sysfs_if,
> -        .ident = "Toshiba Satellite R830",
> -        .matches = {
> -               DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> -               DMI_MATCH(DMI_PRODUCT_NAME, "SATELLITE R830"),
> -               },
> -       },
>         /*
>          * Some machine's _DOD IDs don't have bit 31(Device ID Scheme) set
>          * but the IDs actually follow the Device ID Scheme.
> @@ -1770,9 +1725,6 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
>         if (result)
>                 return;
>
> -       if (disable_backlight_sysfs_if > 0)
> -               return;
> -
>         name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
>         if (!name)
>                 return;
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index db2474fe58ac..0d9064a9804c 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -610,6 +610,41 @@ static const struct dmi_system_id video_detect_dmi_table[] = {
>                 },
>         },
>
> +       /*
> +        * These Toshibas have a broken acpi-video interface for brightness
> +        * control. They also have an issue where the panel is off after
> +        * suspend until a special firmware call is made to turn it back
> +        * on. This is handled by the toshiba_acpi kernel module, so that
> +        * module must be enabled for these models to work correctly.
> +        */
> +       {
> +        /* https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
> +        .callback = video_detect_force_native,
> +        /* Toshiba Portégé R700 */
> +        .matches = {
> +               DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> +               DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE R700"),
> +               },
> +       },
> +       {
> +        /* Portégé: https://bugs.freedesktop.org/show_bug.cgi?id=82634 */
> +        /* Satellite: https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
> +        .callback = video_detect_force_native,
> +        /* Toshiba Satellite/Portégé R830 */
> +        .matches = {
> +               DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> +               DMI_MATCH(DMI_PRODUCT_NAME, "R830"),
> +               },
> +       },
> +       {
> +        .callback = video_detect_force_native,
> +        /* Toshiba Satellite/Portégé Z830 */
> +        .matches = {
> +               DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> +               DMI_MATCH(DMI_PRODUCT_NAME, "Z830"),
> +               },
> +       },
> +
>         /*
>          * Desktops which falsely report a backlight and which our heuristics
>          * for this do not catch.
> --
> 2.36.1
>

Re: [PATCH 1/2] drivers/platform: toshiba_acpi: Call HCI_PANEL_POWER_ON on resume on some models

[Hans de Goede]

Hi,

On 9/5/22 11:37, Arvid Norlander wrote:
> Hi,
> 
> On 2022-09-05 11:00, Hans de Goede wrote:
>> Some Toshibas have a broken acpi-video interface for brightness control, so
>> far these have been using a special workaround in drivers/acpi/acpi_video.c
>> which gets activated by the disable_backlight_sysfs_if module-param/quirks.
>>
>> The recent x86/acpi backlight refactoring has broken this workaround:
>> 1. This workaround relies on acpi_video_get_backlight_type() returning
>>    acpi_video so that the acpi_video code actually runs; and
>> 2. this relies on the actual native GPU driver to offer the sysfs
>>    backlight interface to userspace.
>>
>> After the refactor this breaks since the native driver will no
>> longer register its backlight-device if acpi_video_get_backlight_type()
>> does not return native and making it return native breaks 1.
>>
>> Keeping the acpi_video backlight handling on resume active, while not
>> using it to set the brightness, is necessary because it does a _BCM
>> call on resume which is necessary to turn the panel back on on resume.
>>
>> Looking at the DSDT shows that this _BCM call results in a Toshiba
>> HCI_SET HCI_LCD_BRIGHTNESS call, which turns the panel back on.
>>
>> This commit makes toshiba_acpi do a HCI_SET HCI_PANEL_POWER_ON call
>> on resume on the affected models, so that the (now broken)
>> acpi_video disable_backlight_sysfs_if workaround will no longer
>> be necessary.
>>
>> Note this uses HCI_PANEL_POWER_ON instead of HCI_LCD_BRIGHTNESS
>> to avoid changing the configured brightness level.
> 
> With the previous ACPI Video based approach this quirk was possible to
> control from the kernel command line. This does not seem to be the case
> here. This raises the difficulty for users with as of yet unlisted models
> to test if this quirk would help. Would it be worth adding support for the
> enabling this from the kernel command line?

Yes that is a good idea, I will send out a v2 where this becomes a
module parameter for the toshiba_acpi module.

> 
>>
>> Fixes: b1d36e73cc1c ("drm/i915: Don't register backlight when another backlight should be used (v2)")
>> Tested-by: Arvid Norlander <lkml@vorpal.se>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/toshiba_acpi.c | 46 +++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index 030dc37d50b8..826ffac4af1c 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -23,6 +23,7 @@
>>  #define PROC_INTERFACE_VERSION	1
>>  
>>  #include <linux/compiler.h>
>> +#include <linux/dmi.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/moduleparam.h>
>> @@ -100,6 +101,7 @@ MODULE_LICENSE("GPL");
>>  #define TOS_NOT_INSTALLED		0x8e00
>>  
>>  /* Registers */
>> +#define HCI_PANEL_POWER_ON		0x0002
>>  #define HCI_FAN				0x0004
>>  #define HCI_TR_BACKLIGHT		0x0005
>>  #define HCI_SYSTEM_EVENT		0x0016
>> @@ -206,6 +208,7 @@ struct toshiba_acpi_dev {
>>  
>>  	bool kbd_event_generated;
>>  	bool killswitch;
>> +	bool turn_on_panel_on_resume;
> 
> You added this bool to the section that changes at runtime, rather than
> the feature section just above.
> 
> Also, many of the bools are bitfields, especially (almost) all the ones
> that are about detecting a feature once then setting it. (I belive
> "special_functions" is an exception since it can take more values, and
> when I add support for the non-working buttons on the 830 this will be
> significant).
> 
> In summary I thus believe it would make sense to add your new boolean to
> bitfield section above this one.

Making this a module-parameter will make it a global variable,
completely moving it out of the struct. See the v2 which I will
send out shortly.

Regards,

Hans



> 
>>  };
>>  
>>  static struct toshiba_acpi_dev *toshiba_acpi;
>> @@ -2999,6 +3002,43 @@ static const char *find_hci_method(acpi_handle handle)
>>  	return NULL;
>>  }
>>  
>> +/*
>> + * Some Toshibas have a broken acpi-video interface for brightness control,
>> + * these are quirked in drivers/acpi/video_detect.c to use the GPU native
>> + * (/sys/class/backlight/intel_backlight) instead.
>> + * But these need a HCI_SET call to actually turn the panel back on at resume,
>> + * without this call the screen stays black at resume.
>> + * Either HCI_LCD_BRIGHTNESS (used by acpi_video's _BCM) or HCI_PANEL_POWER_ON
>> + * works. toshiba_acpi_resume() uses HCI_PANEL_POWER_ON to avoid changing
>> + * the configured brightness level.
>> + */
>> +static const struct dmi_system_id turn_on_panel_on_resume_dmi_ids[] = {
>> +	{
>> +	 /* Toshiba Portégé R700 */
>> +	 /* https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
>> +	 .matches = {
>> +		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
>> +		DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE R700"),
>> +		},
>> +	},
>> +	{
>> +	 /* Toshiba Satellite/Portégé R830 */
>> +	 /* Portégé: https://bugs.freedesktop.org/show_bug.cgi?id=82634 */
>> +	 /* Satellite: https://bugzilla.kernel.org/show_bug.cgi?id=21012 */
>> +	 .matches = {
>> +		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
>> +		DMI_MATCH(DMI_PRODUCT_NAME, "R830"),
>> +		},
>> +	},
>> +	{
>> +	 /* Toshiba Satellite/Portégé Z830 */
>> +	 .matches = {
>> +		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
>> +		DMI_MATCH(DMI_PRODUCT_NAME, "Z830"),
>> +		},
>> +	},
>> +};
>> +
>>  static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>>  {
>>  	struct toshiba_acpi_dev *dev;
>> @@ -3141,6 +3181,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>>  	ret = get_fan_status(dev, &dummy);
>>  	dev->fan_supported = !ret;
>>  
>> +	dev->turn_on_panel_on_resume =
>> +		dmi_check_system(turn_on_panel_on_resume_dmi_ids);
>> +
>>  	toshiba_wwan_available(dev);
>>  	if (dev->wwan_supported)
>>  		toshiba_acpi_setup_wwan_rfkill(dev);
>> @@ -3257,6 +3300,9 @@ static int toshiba_acpi_resume(struct device *device)
>>  			rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch);
>>  	}
>>  
>> +	if (dev->turn_on_panel_on_resume)
>> +		hci_write(dev, HCI_PANEL_POWER_ON, 1);
>> +
>>  	return 0;
>>  }
>>  #endif
> 
> Best regards,
> Arvid Norlander
>