diff mbox series

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

Message ID 20220909153239.34606-1-hdegoede@redhat.com (mailing list archive)
State Accepted, archived
Headers show
Series [v2,1/2] drivers/platform: toshiba_acpi: Call HCI_PANEL_POWER_ON on resume on some models | expand

Commit Message

Hans de Goede Sept. 9, 2022, 3:32 p.m. UTC
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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Add a turn_on_panel_on_resume module parameter to allow overriding
  the DMI quirk based setting
---
 drivers/platform/x86/toshiba_acpi.c | 50 +++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Arvid Norlander Sept. 9, 2022, 5:29 p.m. UTC | #1
Hi,

Given the changes, do you want me to test this again? If so, on what branch?

Best regards,
Arvid Norlander

On 2022-09-09 17:32, 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Add a turn_on_panel_on_resume module parameter to allow overriding
>   the DMI quirk based setting
> ---
>  drivers/platform/x86/toshiba_acpi.c | 50 +++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 900ee68a4c0b..aa55ed0d029d 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>
> @@ -50,6 +51,11 @@ MODULE_AUTHOR("John Belmonte");
>  MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
>  MODULE_LICENSE("GPL");
>  
> +int turn_on_panel_on_resume = -1;
> +module_param(turn_on_panel_on_resume, int, 0644);
> +MODULE_PARM_DESC(turn_on_panel_on_resume,
> +	"Call HCI_PANEL_POWER_ON on resume (-1 = auto, 0 = no, 1 = yes");
> +
>  #define TOSHIBA_WMI_EVENT_GUID "59142400-C6A3-40FA-BADB-8A2652834100"
>  
>  /* Scan code for Fn key on TOS1900 models */
> @@ -100,6 +106,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
> @@ -3002,6 +3009,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;
> @@ -3144,6 +3188,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  	ret = get_fan_status(dev, &dummy);
>  	dev->fan_supported = !ret;
>  
> +	if (turn_on_panel_on_resume == -1)
> +		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);
> @@ -3260,6 +3307,9 @@ static int toshiba_acpi_resume(struct device *device)
>  			rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch);
>  	}
>  
> +	if (turn_on_panel_on_resume)
> +		hci_write(dev, HCI_PANEL_POWER_ON, 1);
> +
>  	return 0;
>  }
>  #endif
Hans de Goede Sept. 9, 2022, 5:41 p.m. UTC | #2
Hi,

On 9/9/22 17:32, 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Add a turn_on_panel_on_resume module parameter to allow overriding
>   the DMI quirk based setting

I have added these 2 patches to my review-hans (soon to be for-next)
branch now.

Regards,

Hans




> ---
>  drivers/platform/x86/toshiba_acpi.c | 50 +++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 900ee68a4c0b..aa55ed0d029d 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>
> @@ -50,6 +51,11 @@ MODULE_AUTHOR("John Belmonte");
>  MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
>  MODULE_LICENSE("GPL");
>  
> +int turn_on_panel_on_resume = -1;
> +module_param(turn_on_panel_on_resume, int, 0644);
> +MODULE_PARM_DESC(turn_on_panel_on_resume,
> +	"Call HCI_PANEL_POWER_ON on resume (-1 = auto, 0 = no, 1 = yes");
> +
>  #define TOSHIBA_WMI_EVENT_GUID "59142400-C6A3-40FA-BADB-8A2652834100"
>  
>  /* Scan code for Fn key on TOS1900 models */
> @@ -100,6 +106,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
> @@ -3002,6 +3009,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;
> @@ -3144,6 +3188,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  	ret = get_fan_status(dev, &dummy);
>  	dev->fan_supported = !ret;
>  
> +	if (turn_on_panel_on_resume == -1)
> +		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);
> @@ -3260,6 +3307,9 @@ static int toshiba_acpi_resume(struct device *device)
>  			rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch);
>  	}
>  
> +	if (turn_on_panel_on_resume)
> +		hci_write(dev, HCI_PANEL_POWER_ON, 1);
> +
>  	return 0;
>  }
>  #endif
Hans de Goede Sept. 9, 2022, 5:42 p.m. UTC | #3
Hi,

On 9/9/22 19:29, Arvid Norlander wrote:
> Hi,
> 
> Given the changes, do you want me to test this again? If so, on what branch?

I have just pushed this new version + all your pending toshiba_acpi patches
to my review-hans branch:

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans\

If you can give this branch a quick test and let me know if everything works
as expected that would be great.

Thanks,

Hans



> 
> Best regards,
> Arvid Norlander
> 
> On 2022-09-09 17:32, 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.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> - Add a turn_on_panel_on_resume module parameter to allow overriding
>>   the DMI quirk based setting
>> ---
>>  drivers/platform/x86/toshiba_acpi.c | 50 +++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index 900ee68a4c0b..aa55ed0d029d 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>
>> @@ -50,6 +51,11 @@ MODULE_AUTHOR("John Belmonte");
>>  MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
>>  MODULE_LICENSE("GPL");
>>  
>> +int turn_on_panel_on_resume = -1;
>> +module_param(turn_on_panel_on_resume, int, 0644);
>> +MODULE_PARM_DESC(turn_on_panel_on_resume,
>> +	"Call HCI_PANEL_POWER_ON on resume (-1 = auto, 0 = no, 1 = yes");
>> +
>>  #define TOSHIBA_WMI_EVENT_GUID "59142400-C6A3-40FA-BADB-8A2652834100"
>>  
>>  /* Scan code for Fn key on TOS1900 models */
>> @@ -100,6 +106,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
>> @@ -3002,6 +3009,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;
>> @@ -3144,6 +3188,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>>  	ret = get_fan_status(dev, &dummy);
>>  	dev->fan_supported = !ret;
>>  
>> +	if (turn_on_panel_on_resume == -1)
>> +		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);
>> @@ -3260,6 +3307,9 @@ static int toshiba_acpi_resume(struct device *device)
>>  			rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch);
>>  	}
>>  
>> +	if (turn_on_panel_on_resume)
>> +		hci_write(dev, HCI_PANEL_POWER_ON, 1);
>> +
>>  	return 0;
>>  }
>>  #endif
>
Arvid Norlander Sept. 9, 2022, 6:32 p.m. UTC | #4
Hi,

On 2022-09-09 19:42, Hans de Goede wrote:
> Hi,
> 
> On 9/9/22 19:29, Arvid Norlander wrote:
>> Hi,
>>
>> Given the changes, do you want me to test this again? If so, on what branch?
> 
> I have just pushed this new version + all your pending toshiba_acpi patches
> to my review-hans branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans\
> 
> If you can give this branch a quick test and let me know if everything works
> as expected that would be great.

My changes (battery charge threshold, fan HWMON support, ECO LED work as
expected.

Display does come on after resume. Unfortunately the computer also
completely locks up (I can't even switch from X to a VT). SSH is dead.
When I suspend with a VT active the cursor is no longer blinking on resume.
So something else is severly broken in this version and I cannot fully test
this.

I notice your tree is based on rc1. I was previously testing with 5.19 as
well as rc2 and newer. Is it possible this is a bug from mainline? Or is
it something introduced by your tree? I'm wondering it it would be easier
to first rebase your tree on the latest RC rather than trying to bisect it
as is.

Or the easiest test is perhaps: Does suspend and resume with this tree work
on your computer? I remember hearing that there were SATA issues in early
6.0-rcs (don't remember the details), the Toshiba uses mSATA.

> 
> Thanks,
> 
> Hans
> 
<snip>

Best regards,
Arvid Norlander
Hans de Goede Sept. 9, 2022, 7:20 p.m. UTC | #5
Hi,

On 9/9/22 20:32, Arvid Norlander wrote:
> Hi,
> 
> On 2022-09-09 19:42, Hans de Goede wrote:
>> Hi,
>>
>> On 9/9/22 19:29, Arvid Norlander wrote:
>>> Hi,
>>>
>>> Given the changes, do you want me to test this again? If so, on what branch?
>>
>> I have just pushed this new version + all your pending toshiba_acpi patches
>> to my review-hans branch:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans\
>>
>> If you can give this branch a quick test and let me know if everything works
>> as expected that would be great.
> 
> My changes (battery charge threshold, fan HWMON support, ECO LED work as
> expected.
> 
> Display does come on after resume. Unfortunately the computer also
> completely locks up (I can't even switch from X to a VT). SSH is dead.
> When I suspend with a VT active the cursor is no longer blinking on resume.
> So something else is severly broken in this version and I cannot fully test
> this.
> 
> I notice your tree is based on rc1. I was previously testing with 5.19 as
> well as rc2 and newer. Is it possible this is a bug from mainline?

Ah yes, sorry about that. There is a scsi bug which causes any sata
disks to become inaccessible after suspend/resume in rc1.

If you cherry pick this commit on top of my tree the problem should be gone:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=785538bfdd682c8e962341d585f9b88262a0475e

> Or is
> it something introduced by your tree? I'm wondering it it would be easier
> to first rebase your tree on the latest RC rather than trying to bisect it
> as is.
> 
> Or the easiest test is perhaps: Does suspend and resume with this tree work
> on your computer? I remember hearing that there were SATA issues in early
> 6.0-rcs (don't remember the details), the Toshiba uses mSATA.

Right, see above.

Regards,

Hans
Arvid Norlander Sept. 9, 2022, 7:44 p.m. UTC | #6
Hi,

On 2022-09-09 21:20, Hans de Goede wrote:
> Hi,
> 
> On 9/9/22 20:32, Arvid Norlander wrote:
>> Hi,
>>
>> On 2022-09-09 19:42, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 9/9/22 19:29, Arvid Norlander wrote:
>>>> Hi,
>>>>
>>>> Given the changes, do you want me to test this again? If so, on what branch?
>>>
>>> I have just pushed this new version + all your pending toshiba_acpi patches
>>> to my review-hans branch:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans\
>>>
>>> If you can give this branch a quick test and let me know if everything works
>>> as expected that would be great.
>>
>> My changes (battery charge threshold, fan HWMON support, ECO LED work as
>> expected.
>>
>> Display does come on after resume. Unfortunately the computer also
>> completely locks up (I can't even switch from X to a VT). SSH is dead.
>> When I suspend with a VT active the cursor is no longer blinking on resume.
>> So something else is severly broken in this version and I cannot fully test
>> this.
>>
>> I notice your tree is based on rc1. I was previously testing with 5.19 as
>> well as rc2 and newer. Is it possible this is a bug from mainline?
> 
> Ah yes, sorry about that. There is a scsi bug which causes any sata
> disks to become inaccessible after suspend/resume in rc1.
> 
> If you cherry pick this commit on top of my tree the problem should be gone:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=785538bfdd682c8e962341d585f9b88262a0475e

This fixed it. Resume works properly, as does backlight controls after resume.

Tested-by: Arvid Norlander <lkml@vorpal.se>
(For the relevant toshiba_acpi patches)

However I notice that the laptop is hung for about a second after resume. I
had not noticed this before. But I went back and checked on 5.19 and it
happens there too, so presumably I just ended up being hyper-aware about
resume issues after this...

> 
>> Or is
>> it something introduced by your tree? I'm wondering it it would be easier
>> to first rebase your tree on the latest RC rather than trying to bisect it
>> as is.
>>
>> Or the easiest test is perhaps: Does suspend and resume with this tree work
>> on your computer? I remember hearing that there were SATA issues in early
>> 6.0-rcs (don't remember the details), the Toshiba uses mSATA.
> 
> Right, see above.
> 
> Regards,
> 
> Hans
> 

Best regards,
Arvid Norlander
diff mbox series

Patch

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 900ee68a4c0b..aa55ed0d029d 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>
@@ -50,6 +51,11 @@  MODULE_AUTHOR("John Belmonte");
 MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
 MODULE_LICENSE("GPL");
 
+int turn_on_panel_on_resume = -1;
+module_param(turn_on_panel_on_resume, int, 0644);
+MODULE_PARM_DESC(turn_on_panel_on_resume,
+	"Call HCI_PANEL_POWER_ON on resume (-1 = auto, 0 = no, 1 = yes");
+
 #define TOSHIBA_WMI_EVENT_GUID "59142400-C6A3-40FA-BADB-8A2652834100"
 
 /* Scan code for Fn key on TOS1900 models */
@@ -100,6 +106,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
@@ -3002,6 +3009,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;
@@ -3144,6 +3188,9 @@  static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 	ret = get_fan_status(dev, &dummy);
 	dev->fan_supported = !ret;
 
+	if (turn_on_panel_on_resume == -1)
+		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);
@@ -3260,6 +3307,9 @@  static int toshiba_acpi_resume(struct device *device)
 			rfkill_set_hw_state(dev->wwan_rfk, !dev->killswitch);
 	}
 
+	if (turn_on_panel_on_resume)
+		hci_write(dev, HCI_PANEL_POWER_ON, 1);
+
 	return 0;
 }
 #endif