Re: [PATCH 1/1] ACPI: video: Add Toshiba Satellite/Portege Z830 quirk

[Hans de Goede <hdegoede@redhat.com>]

Hi All,

On 8/24/22 20:49, Arvid Norlander wrote:
> Toshiba Satellite Z830 needs the quirk video_disable_backlight_sysfs_if
> for proper backlight control after suspend/resume cycles.
> 
> Toshiba Portege Z830 is simply the same laptop rebranded for certain
> markets (I looked through the manual to other language sections to confirm
> this) and thus also needs this quirk.
> 
> Thanks to Hans de Goede for suggesting this fix.
> 
> Link: https://www.spinics.net/lists/platform-driver-x86/msg34394.html
> Signed-off-by: Arvid Norlander <lkml@vorpal.se>

So I've been thinking a bit about this and this is going to
be a problem with my pending backlight refactor work.

*** Note this patch should still be merged as a fix for 6.0
and older ***

On these models acpi_video_get_backlight_type() returns
acpi_backlight_video, otherwise setting disable_backlight_sysfs_if
would not do anything.

After my "drm/kms: Stop registering multiple /sys/class/backlight
devs for a single display" series (1), the intel_backlight will no
longer gets registered, that now only happens when
acpi_video_get_backlight_type() returns acpi_backlight_native.

This will break the disable_backlight_sysfs_if workaround
because after this there then won't be any devices under
/sys/class/backlight at all.

I have been thinking about how to fix this, here are my notes
I have written on this.

-toshiba r830 / z830 problem after series:
 -duplicate DMI match in video_detect, force native
 -make disable_backlight_sysfs_if still work even when
  acpi_video_register_backlight() does not get called, just do
  the backlight_init directly from acpi_video_register()
  when disable_backlight_sysfs_if is set, as before
  my refactor series

This will work, but it makes the code (even more) ugly /
convoluted.

Arvid, I wonder if instead of using disable_backlight_sysfs_if
you can try:

0. Remove disable_backlight_sysfs_if from cmdline / quirk
1. Adding acpi_backlight=native to the kernel commandline
2. In toshiba_acpi_resume() add a HCI_PANEL_POWER_ON PANEL_ON

and see if that also fixes things ?

If that is the case then we can:

1. Move the DMI quirks for disable_backlight_sysfs_if
   from acpi_video.c to video_detect.c to force native
   mode by quirk
2. Add the DMI table with the models needing this to
   toshiba_acpi.c and then based on that call
   HCI_PANEL_POWER_ON PANEL_ON on resume from there
3. Since there are no more quirks using it, remove the
   disable_backlight_sysfs_if hack / workaround from
   acpi_video.c

This will give a nice-cleanup of the generic acpi_video.c
code moving the toshiba specific fixup to toshiba_acpi
where it really belongs.

Regards,

Hans



1) https://lore.kernel.org/platform-driver-x86/20220825143726.269890-1-hdegoede@redhat.com/T/


> ---
>  drivers/acpi/acpi_video.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 5cbe2196176d..2a4990733cf0 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -496,6 +496,22 @@ static const struct dmi_system_id video_dmi_table[] = {
>  		DMI_MATCH(DMI_PRODUCT_NAME, "SATELLITE R830"),
>  		},
>  	},
> +	{
> +	 .callback = video_disable_backlight_sysfs_if,
> +	 .ident = "Toshiba Satellite Z830",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "SATELLITE Z830"),
> +		},
> +	},
> +	{
> +	 .callback = video_disable_backlight_sysfs_if,
> +	 .ident = "Toshiba Portege Z830",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "PORTEGE Z830"),
> +		},
> +	},
>  	/*
>  	 * Some machine's _DOD IDs don't have bit 31(Device ID Scheme) set
>  	 * but the IDs actually follow the Device ID Scheme.