| Message ID | 20220824184950.631520-2-lkml@vorpal.se |
|---|---|
| State | Accepted |
| Commit | 574160b8548deff8b80b174f03201e94ab8431e2 |
| Headers | show |
| Series | Fix backlight control on Toshiba Satellite Z830 | expand |
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.
Hi, On 2022-08-26 13:46, Hans de Goede wrote: > Hi All, > > [...] > > 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 ? > Yes, this works. I do not have a patch for this (I assume it would involve creating quirk tables, checking for support for HCI_PANEL_POWER_ON, etc). I simply hard coded the call in for the test. I very much doubt I will have time to code this in the near future as well. However, do we know what the other Toshiba's that need this quirk also supports HCI_PANEL_POWER_ON? I obviously can only test the Z830 that I own. > 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 > > Best regards, Arvid Norlander
hI, On 8/27/22 13:23, Arvid Norlander wrote: > Hi, > > On 2022-08-26 13:46, Hans de Goede wrote: >> Hi All, >> >> [...] >> >> 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 ? >> > Yes, this works. Great, thank you for testing this! In hindsight the disable_backlight_sysfs_if flag was a mistake and I should have fixed this differently (I wrote the code adding that flag). And now it is sorta getting in the way of cleaning up the backlight handling. So IMHO removing disable_backlight_sysfs_if is the best thing to do here. > I do not have a patch for this (I assume it > would involve creating quirk tables, checking for support for > HCI_PANEL_POWER_ON, etc). I simply hard coded the call in for > the test. I very much doubt I will have time to code this in > the near future as well. No problem I will prepare a patch series for you to test. Note this will be on top of my other backlight cleanups, so I will just send you an email pointing to a git branch to tes, I hope this will be ok? > However, do we know what the other Toshiba's that need this > quirk also supports HCI_PANEL_POWER_ON? I obviously can only > test the Z830 that I own. It seems that all models which need this are all from the same generation so I would expect the same fix to work. If I get regression reports from users after my cleanup series lands I can then take a closer look at the DSDT tables of the other models if necessary. Regards, Hans > >> 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 >> >> > > Best regards, > Arvid Norlander >
Hi, On 2022-08-29 16:12, Hans de Goede wrote: > Hi, > <snip> > > Arvid, here is a git branch with my backlight-refactor for you > to test: > > https://github.com/jwrdegoede/linux-sunxi/commits/backlight-refactor-for-arvid > > If you can give this a test spin (without any special kernel > commandline options) then that would be great. I'll set up a PKGBUILD and get this built (I'm building on other computers). It may take a couple of days before I get around to that however. I hope this is okay with you. <snip> > Regards, > > Hans > Best regards, Arvid Norlander
Hi, On 2022-08-29 20:58, Hans de Goede wrote: > Hi, > > On 8/29/22 20:30, Arvid Norlander wrote: >> Hi, >> >> On 2022-08-29 16:12, Hans de Goede wrote: >>> Hi, >>> >> >> <snip> >> >>> >>> Arvid, here is a git branch with my backlight-refactor for you >>> to test: >>> >>> https://github.com/jwrdegoede/linux-sunxi/commits/backlight-refactor-for-arvid >>> >>> If you can give this a test spin (without any special kernel >>> commandline options) then that would be great. >> >> I'll set up a PKGBUILD and get this built (I'm building on other computers). >> It may take a couple of days before I get around to that however. I hope >> this is okay with you. > > Yes that is fine, thank you. Just and update to let you know that your tree works, at least for suspend. I'm not set up to test hibernation (using swap file on btrfs). Nor do I know if it even works on this laptop. It has some sort of auto hibernate feature in BIOS called Intel Rapid Start. It supposedly auto transitions to hibernation after being asleep for a while. I have not looked into if this is supported on Linux, and what setup would be required to support it in that case. > > Regards, > > Hans > Best regards, Arvid Norlander
Hi, On 8/31/22 15:44, Arvid Norlander wrote: > Hi, > > > On 2022-08-29 20:58, Hans de Goede wrote: >> Hi, >> >> On 8/29/22 20:30, Arvid Norlander wrote: >>> Hi, >>> >>> On 2022-08-29 16:12, Hans de Goede wrote: >>>> Hi, >>>> >>> >>> <snip> >>> >>>> >>>> Arvid, here is a git branch with my backlight-refactor for you >>>> to test: >>>> >>>> https://github.com/jwrdegoede/linux-sunxi/commits/backlight-refactor-for-arvid >>>> >>>> If you can give this a test spin (without any special kernel >>>> commandline options) then that would be great. >>> >>> I'll set up a PKGBUILD and get this built (I'm building on other computers). >>> It may take a couple of days before I get around to that however. I hope >>> this is okay with you. >> >> Yes that is fine, thank you. > > Just and update to let you know that your tree works, at least for suspend. Great, thank you so much for testing this! Is it ok if I add a: Tested-by: Arvid Norlander <lkml@vorpal.se> to the 2 patches for fixing this to give you credit for your testing ? > I'm not set up to test hibernation (using swap file on btrfs). Nor do I > know if it even works on this laptop. It has some sort of auto hibernate > feature in BIOS called Intel Rapid Start. It supposedly auto transitions to > hibernation after being asleep for a while. I have not looked into if this > is supported on Linux, and what setup would be required to support it in > that case. Regular suspend/resume testing is what I was looking for. On restore from hibernation the backlight is already on when restoring the state so I don't expect any problems there. And as you indicate getting hibernation to work is tricky in general, IMHO there is no need to go through all the trouble necessary to (maybe) get that to work. Regards, Hans
Hi, On 2022-09-01 12:42, Hans de Goede wrote: > Hi, > <snip> > > Great, thank you so much for testing this! > > Is it ok if I add a: > > Tested-by: Arvid Norlander <lkml@vorpal.se> > > to the 2 patches for fixing this to give you credit for your testing ? Yes, of course. As a newcommer it is hard to learn and remember all these little rules you have in kernel development. It is quite different from the forge-based workflow I'm used to. > >> I'm not set up to test hibernation (using swap file on btrfs). Nor do I >> know if it even works on this laptop. It has some sort of auto hibernate >> feature in BIOS called Intel Rapid Start. It supposedly auto transitions to >> hibernation after being asleep for a while. I have not looked into if this >> is supported on Linux, and what setup would be required to support it in >> that case. > > Regular suspend/resume testing is what I was looking for. On restore > from hibernation the backlight is already on when restoring the state so > I don't expect any problems there. And as you indicate getting hibernation > to work is tricky in general, IMHO there is no need to go through all > the trouble necessary to (maybe) get that to work. Hm, I have not found it tricky when I have had swap partitions. At least on my Thinkpads as long as I avoid nvidia drivers. > > Regards, > > Hans > Best regards, Arvid Norlander
Hi, On 9/1/22 16:15, Arvid Norlander wrote: > Hi, > > On 2022-09-01 12:42, Hans de Goede wrote: >> Hi, >> > > <snip> > >> >> Great, thank you so much for testing this! >> >> Is it ok if I add a: >> >> Tested-by: Arvid Norlander <lkml@vorpal.se> >> >> to the 2 patches for fixing this to give you credit for your testing ? > > Yes, of course. As a newcommer it is hard to learn and remember all these > little rules you have in kernel development. It is quite different from the > forge-based workflow I'm used to. No worries, I think you are doing great. Actually I could just have added your Tested-by tags if I wanted too. Only the Signed-off-by tag is one which you must explicitly give. But asking seemed like the polite thing to do :) >>> I'm not set up to test hibernation (using swap file on btrfs). Nor do I >>> know if it even works on this laptop. It has some sort of auto hibernate >>> feature in BIOS called Intel Rapid Start. It supposedly auto transitions to >>> hibernation after being asleep for a while. I have not looked into if this >>> is supported on Linux, and what setup would be required to support it in >>> that case. >> >> Regular suspend/resume testing is what I was looking for. On restore >> from hibernation the backlight is already on when restoring the state so >> I don't expect any problems there. And as you indicate getting hibernation >> to work is tricky in general, IMHO there is no need to go through all >> the trouble necessary to (maybe) get that to work. > > Hm, I have not found it tricky when I have had swap partitions. At least on > my Thinkpads as long as I avoid nvidia drivers. As long as you have large enough swap partitions, yes. Sometimes figuring out what large enough is is tricky though. Anyway we are getting offtopic. Regards, Hans
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.
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> --- drivers/acpi/acpi_video.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)