| Message ID | 20221024113513.5205-1-akihiko.odaki@daynix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fallback to native backlight | expand |
Hi, On 10/24/22 15:14, Pali Rohár wrote: > On Monday 24 October 2022 21:58:57 Akihiko Odaki wrote: >> Regarding the second limitation, I don't even understand the difference >> between vendor and native. My guess is that a vendor backlight device uses >> vendor-specific ACPI interface, and a native one directly uses hardware >> registers. If my guess is correct, the difference between vendor and native >> does not imply that both of them are likely to exist at the same time. As >> the conclusion, there is no more motivation to try to de-duplicate the >> vendor/native combination than to try to de-duplicate combination of devices >> with a single type. > > Hello! I just want to point one thing. On some Dell laptops there are > 3 different ways (= 3 different APIs) how to control display backlight. > There is ACPI driver (uses ACPI), GPU/DRM driver (i915.ko; uses directly > HW) and platform vendor driver (dell-laptop.ko; uses vendor BIOS or > firmware API). Right and that is just one example of laptops which can register both vendor + native backlight devices, which is why this whole series is a bad idea. Regards, Hans > Just every driver has different pre-calculated scaling > values. So sometimes user wants to choose different driver just because > it allows to set backlight level with "better" granularity. Registering > all 3 device drivers is bad as user does not want to see 3 display > panels and forcing registration of specific one without runtime option > is also bad (some of those drivers do not have to be suitable or has > worse granularity as other).
On 2022/10/24 22:21, Hans de Goede wrote: > Hi, > > On 10/24/22 14:58, Akihiko Odaki wrote: >> On 2022/10/24 20:53, Hans de Goede wrote: >>> Hi Akihiko, >>> >>> On 10/24/22 13:34, Akihiko Odaki wrote: >>>> Commit 2600bfa3df99 ("ACPI: video: Add acpi_video_backlight_use_native() >>>> helper") and following commits made native backlight unavailable if >>>> CONFIG_ACPI_VIDEO is set and the backlight feature of ACPI video is >>>> unavailable, which broke the backlight functionality on Lenovo ThinkPad >>>> C13 Yoga Chromebook. Allow to fall back to native backlight in such >>>> cases. >>> >>> I appreciate your work on this, but what this in essence does is >>> it allows 2 backlight drivers (vendor + native) to get registered >>> for the same panel again. While the whole goal of the backlight refactor >>> series landing in 6.1 was to make it so that there always is only >>> *1* backlight device registered instead of (possibly) registering >>> multiple and letting userspace figure it out. It is also important >>> to only always have 1 backlight device per panel for further >>> upcoming changes. >>> >>> So nack for this solution, sorry. >>> >>> I am aware that this breaks backlight control on some Chromebooks, >>> this was already reported and I wrote a long reply explaining why >>> things are done the way they are done now and also suggesting >>> 2 possible (much simpler) fixes, see: >>> https://lore.kernel.org/linux-acpi/42a5f2c9-a1dc-8fc0-7334-fe6c390ecfbb@redhat.com/ >>> >>> Unfortunately the reported has not followed-up on this and >>> I don't have the hardware to test this myself. >>> >>> Can you please try implementing 1 of the fixes suggested there >>> and then submit that upstream ? >>> >>> Regards, >>> >>> Hans >>> >> >> Hi Hans, >> >> Thanks for reviewing and letting me know the prior attempt. >> >> In this case, there is only a native backlight device and no vendor backlight device so the duplication of backlight devices does not happen. I think it is better to handle such a case without quirks. > > Adding a single heuristic for all chromebooks is something completely different > then adding per model quirks which indeed ideally should be avoided (but this > is not always possible). > >> I understand it is still questionable to cover the case by allowing duplication when both of a vendor backlight device and native one. To explain my understanding and reasoning for *not* trying to apply the de-duplication rule to the vendor/native combination, let me first describe that the de-duplication which happens in acpi_video_get_backlight_type() is a heuristics and limited. >> >> As the background of acpi_video_get_backlight_type(), there is an assumption that it should be common that both of the firmware, implementing ACPI, and the kernel have code to drive backlight. In the case, the more reliable one should be picked instead of using both, and that is what the statements in `if (video_caps & ACPI_VIDEO_BACKLIGHT)` does. >> >> However, the method has two limitations: >> 1. It does not cover the case where two backlight devices with the same type exist. > > This only happens when there are 2 panels; or 2 gpus driving a single panel > which are both special cases where we actually want 2 backlight devices. > >> 2. The underlying assumption does not apply to vendor/native combination. >> >> Regarding the second limitation, I don't even understand the difference between vendor and native. My guess is that a vendor backlight device uses vendor-specific ACPI interface, and a native one directly uses hardware registers. If my guess is correct, the difference between vendor and native does not imply that both of them are likely to exist at the same time. As the conclusion, there is no more motivation to try to de-duplicate the vendor/native combination than to try to de-duplicate combination of devices with a single type. >> >> Of course, it is better if we could also avoid registering two devices with one type for one physical device. Possibly we can do so by providing a parameter to indicate that it is for the same "internal" backlight to devm_backlight_device_register(), and let the function check for the duplication. However, this rule may be too restrict, or may have problems I missed. >> >> Based on the discussion above, we can deduce three possibilities: >> a. There is no reason to distinguish vendor and native in this case, and we can stick to my current proposal. >> b. There is a valid reason to distinguish vendor and native, and we can adopt the same strategy that already adopted for ACPI video/vendor combination. >> c. We can implement de-duplication in devm_backlight_device_register(). >> d. The other possible options are not worth, and we can just implement quirks specific to Chromebook/coreboot. >> >> In case b, it should be noted that vendor and native backlight device do not require ACPI video, and CONFIG_ACPI_VIDEO may not be enabled. In the case, the de-duplication needs to be implemented in backlight class device. > > I have been working on the ACPI/x86 backlight detection code since 2015, please trust > me when I say that allowing both vendor + native backlight devices at the same time > is a bad idea. > > I'm currently in direct contact with the ChromeOS team about fixing the Chromebook > backlight issue introduced in 6.1-rc1. > > If you wan to help, please read: > > https://lore.kernel.org/linux-acpi/42a5f2c9-a1dc-8fc0-7334-fe6c390ecfbb@redhat.com/ > > And try implementing 1 if the 2 solutions suggested there. > > Regards, > > Hans Hi, I just wanted to confirm your intention that we should distinguish vendor and native. In the case I think it is better to modify __acpi_video_get_backlight_type() and add "native_available" check in case of no ACPI video as already done for the ACPI video/native combination. Unfortunately this has one pitfall though: it does not work if CONFIG_ACPI_VIDEO is disabled. If it is, acpi_video_get_backlight_type() always return acpi_backlight_vendor, and acpi_video_backlight_use_native() always returns true. It is not a regression but the current behavior. Fixing it requires also refactoring touching both of ACPI video and backlight class driver so I guess I'm not an appropriate person to do that, and I should just add "native_available" check to __acpi_video_get_backlight_type(). Does that sound good? Regards, Akihiko Odaki > > > >>>> Akihiko Odaki (22): >>>> drm/i915/opregion: Improve backlight request condition >>>> ACPI: video: Introduce acpi_video_get_backlight_types() >>>> LoongArch: Use acpi_video_get_backlight_types() >>>> platform/x86: acer-wmi: Use acpi_video_get_backlight_types() >>>> platform/x86: asus-laptop: Use acpi_video_get_backlight_types() >>>> platform/x86: asus-wmi: Use acpi_video_get_backlight_types() >>>> platform/x86: compal-laptop: Use acpi_video_get_backlight_types() >>>> platform/x86: eeepc-laptop: Use acpi_video_get_backlight_types() >>>> platform/x86: fujitsu-laptop: Use acpi_video_get_backlight_types() >>>> platform/x86: ideapad-laptop: Use acpi_video_get_backlight_types() >>>> platform/x86: msi-laptop: Use acpi_video_get_backlight_types() >>>> platform/x86: msi-wmi: Use acpi_video_get_backlight_types() >>>> platform/x86: nvidia-wmi-ec-backlight: Use >>>> acpi_video_get_backlight_types() >>>> platform/x86: panasonic-laptop: Use acpi_video_get_backlight_types() >>>> platform/x86: samsung-laptop: Use acpi_video_get_backlight_types() >>>> platform/x86: sony-laptop: Use acpi_video_get_backlight_types() >>>> platform/x86: thinkpad_acpi: Use acpi_video_get_backlight_types() >>>> platform/x86: toshiba_acpi: Use acpi_video_get_backlight_types() >>>> platform/x86: dell-laptop: Use acpi_video_get_backlight_types() >>>> platform/x86: intel_oaktrail: Use acpi_video_get_backlight_types() >>>> ACPI: video: Remove acpi_video_get_backlight_type() >>>> ACPI: video: Fallback to native backlight >>>> >>>> Documentation/gpu/todo.rst | 8 +-- >>>> drivers/acpi/acpi_video.c | 2 +- >>>> drivers/acpi/video_detect.c | 54 ++++++++++--------- >>>> drivers/gpu/drm/i915/display/intel_opregion.c | 3 +- >>>> drivers/platform/loongarch/loongson-laptop.c | 4 +- >>>> drivers/platform/x86/acer-wmi.c | 2 +- >>>> drivers/platform/x86/asus-laptop.c | 2 +- >>>> drivers/platform/x86/asus-wmi.c | 4 +- >>>> drivers/platform/x86/compal-laptop.c | 2 +- >>>> drivers/platform/x86/dell/dell-laptop.c | 2 +- >>>> drivers/platform/x86/eeepc-laptop.c | 2 +- >>>> drivers/platform/x86/fujitsu-laptop.c | 4 +- >>>> drivers/platform/x86/ideapad-laptop.c | 2 +- >>>> drivers/platform/x86/intel/oaktrail.c | 2 +- >>>> drivers/platform/x86/msi-laptop.c | 2 +- >>>> drivers/platform/x86/msi-wmi.c | 2 +- >>>> .../platform/x86/nvidia-wmi-ec-backlight.c | 2 +- >>>> drivers/platform/x86/panasonic-laptop.c | 2 +- >>>> drivers/platform/x86/samsung-laptop.c | 2 +- >>>> drivers/platform/x86/sony-laptop.c | 2 +- >>>> drivers/platform/x86/thinkpad_acpi.c | 4 +- >>>> drivers/platform/x86/toshiba_acpi.c | 2 +- >>>> drivers/video/backlight/backlight.c | 18 +++++++ >>>> include/acpi/video.h | 21 ++++---- >>>> include/linux/backlight.h | 1 + >>>> 25 files changed, 85 insertions(+), 66 deletions(-) >>>> >>> >> >
On 2022/10/24 23:06, Akihiko Odaki wrote: > On 2022/10/24 22:21, Hans de Goede wrote: >> Hi, >> >> On 10/24/22 14:58, Akihiko Odaki wrote: >>> On 2022/10/24 20:53, Hans de Goede wrote: >>>> Hi Akihiko, >>>> >>>> On 10/24/22 13:34, Akihiko Odaki wrote: >>>>> Commit 2600bfa3df99 ("ACPI: video: Add >>>>> acpi_video_backlight_use_native() >>>>> helper") and following commits made native backlight unavailable if >>>>> CONFIG_ACPI_VIDEO is set and the backlight feature of ACPI video is >>>>> unavailable, which broke the backlight functionality on Lenovo >>>>> ThinkPad >>>>> C13 Yoga Chromebook. Allow to fall back to native backlight in such >>>>> cases. >>>> >>>> I appreciate your work on this, but what this in essence does is >>>> it allows 2 backlight drivers (vendor + native) to get registered >>>> for the same panel again. While the whole goal of the backlight >>>> refactor >>>> series landing in 6.1 was to make it so that there always is only >>>> *1* backlight device registered instead of (possibly) registering >>>> multiple and letting userspace figure it out. It is also important >>>> to only always have 1 backlight device per panel for further >>>> upcoming changes. >>>> >>>> So nack for this solution, sorry. >>>> >>>> I am aware that this breaks backlight control on some Chromebooks, >>>> this was already reported and I wrote a long reply explaining why >>>> things are done the way they are done now and also suggesting >>>> 2 possible (much simpler) fixes, see: >>>> https://lore.kernel.org/linux-acpi/42a5f2c9-a1dc-8fc0-7334-fe6c390ecfbb@redhat.com/ >>>> >>>> Unfortunately the reported has not followed-up on this and >>>> I don't have the hardware to test this myself. >>>> >>>> Can you please try implementing 1 of the fixes suggested there >>>> and then submit that upstream ? >>>> >>>> Regards, >>>> >>>> Hans >>>> >>> >>> Hi Hans, >>> >>> Thanks for reviewing and letting me know the prior attempt. >>> >>> In this case, there is only a native backlight device and no vendor >>> backlight device so the duplication of backlight devices does not >>> happen. I think it is better to handle such a case without quirks. >> >> Adding a single heuristic for all chromebooks is something completely >> different >> then adding per model quirks which indeed ideally should be avoided >> (but this >> is not always possible). >> >>> I understand it is still questionable to cover the case by allowing >>> duplication when both of a vendor backlight device and native one. To >>> explain my understanding and reasoning for *not* trying to apply the >>> de-duplication rule to the vendor/native combination, let me first >>> describe that the de-duplication which happens in >>> acpi_video_get_backlight_type() is a heuristics and limited. >>> >>> As the background of acpi_video_get_backlight_type(), there is an >>> assumption that it should be common that both of the firmware, >>> implementing ACPI, and the kernel have code to drive backlight. In >>> the case, the more reliable one should be picked instead of using >>> both, and that is what the statements in `if (video_caps & >>> ACPI_VIDEO_BACKLIGHT)` does. >>> >>> However, the method has two limitations: >>> 1. It does not cover the case where two backlight devices with the >>> same type exist. >> >> This only happens when there are 2 panels; or 2 gpus driving a single >> panel >> which are both special cases where we actually want 2 backlight devices. >> >>> 2. The underlying assumption does not apply to vendor/native >>> combination. >>> >>> Regarding the second limitation, I don't even understand the >>> difference between vendor and native. My guess is that a vendor >>> backlight device uses vendor-specific ACPI interface, and a native >>> one directly uses hardware registers. If my guess is correct, the >>> difference between vendor and native does not imply that both of them >>> are likely to exist at the same time. As the conclusion, there is no >>> more motivation to try to de-duplicate the vendor/native combination >>> than to try to de-duplicate combination of devices with a single type. >>> >>> Of course, it is better if we could also avoid registering two >>> devices with one type for one physical device. Possibly we can do so >>> by providing a parameter to indicate that it is for the same >>> "internal" backlight to devm_backlight_device_register(), and let the >>> function check for the duplication. However, this rule may be too >>> restrict, or may have problems I missed. >>> >>> Based on the discussion above, we can deduce three possibilities: >>> a. There is no reason to distinguish vendor and native in this case, >>> and we can stick to my current proposal. >>> b. There is a valid reason to distinguish vendor and native, and we >>> can adopt the same strategy that already adopted for ACPI >>> video/vendor combination. >>> c. We can implement de-duplication in devm_backlight_device_register(). >>> d. The other possible options are not worth, and we can just >>> implement quirks specific to Chromebook/coreboot. >>> >>> In case b, it should be noted that vendor and native backlight device >>> do not require ACPI video, and CONFIG_ACPI_VIDEO may not be enabled. >>> In the case, the de-duplication needs to be implemented in backlight >>> class device. >> >> I have been working on the ACPI/x86 backlight detection code since >> 2015, please trust >> me when I say that allowing both vendor + native backlight devices at >> the same time >> is a bad idea. >> >> I'm currently in direct contact with the ChromeOS team about fixing >> the Chromebook >> backlight issue introduced in 6.1-rc1. >> >> If you wan to help, please read: >> >> https://lore.kernel.org/linux-acpi/42a5f2c9-a1dc-8fc0-7334-fe6c390ecfbb@redhat.com/ >> >> And try implementing 1 if the 2 solutions suggested there. >> >> Regards, >> >> Hans > > Hi, > > I just wanted to confirm your intention that we should distinguish > vendor and native. In the case I think it is better to modify > __acpi_video_get_backlight_type() and add "native_available" check in > case of no ACPI video as already done for the ACPI video/native > combination. > > Unfortunately this has one pitfall though: it does not work if > CONFIG_ACPI_VIDEO is disabled. If it is, acpi_video_get_backlight_type() > always return acpi_backlight_vendor, and > acpi_video_backlight_use_native() always returns true. It is not a > regression but the current behavior. Fixing it requires also refactoring > touching both of ACPI video and backlight class driver so I guess I'm > not an appropriate person to do that, and I should just add > "native_available" check to __acpi_video_get_backlight_type(). > > Does that sound good? Well, it would not be that easy since just adding native_available cannot handle the case where the vendor driver gets registered first. Checking with "native_available" was possible for ACPI video/vendor combination because ACPI video registers its backlight after some delay. I still think it does not overcomplicate things to modify __acpi_video_get_backlight_type() so that it can use both of vendor and native as fallback while preventing duplicate registration. Regards, Akihiko Odaki > > Regards, > Akihiko Odaki > >> > >> >>>>> Akihiko Odaki (22): >>>>> drm/i915/opregion: Improve backlight request condition >>>>> ACPI: video: Introduce acpi_video_get_backlight_types() >>>>> LoongArch: Use acpi_video_get_backlight_types() >>>>> platform/x86: acer-wmi: Use acpi_video_get_backlight_types() >>>>> platform/x86: asus-laptop: Use acpi_video_get_backlight_types() >>>>> platform/x86: asus-wmi: Use acpi_video_get_backlight_types() >>>>> platform/x86: compal-laptop: Use acpi_video_get_backlight_types() >>>>> platform/x86: eeepc-laptop: Use acpi_video_get_backlight_types() >>>>> platform/x86: fujitsu-laptop: Use acpi_video_get_backlight_types() >>>>> platform/x86: ideapad-laptop: Use acpi_video_get_backlight_types() >>>>> platform/x86: msi-laptop: Use acpi_video_get_backlight_types() >>>>> platform/x86: msi-wmi: Use acpi_video_get_backlight_types() >>>>> platform/x86: nvidia-wmi-ec-backlight: Use >>>>> acpi_video_get_backlight_types() >>>>> platform/x86: panasonic-laptop: Use >>>>> acpi_video_get_backlight_types() >>>>> platform/x86: samsung-laptop: Use acpi_video_get_backlight_types() >>>>> platform/x86: sony-laptop: Use acpi_video_get_backlight_types() >>>>> platform/x86: thinkpad_acpi: Use acpi_video_get_backlight_types() >>>>> platform/x86: toshiba_acpi: Use acpi_video_get_backlight_types() >>>>> platform/x86: dell-laptop: Use acpi_video_get_backlight_types() >>>>> platform/x86: intel_oaktrail: Use acpi_video_get_backlight_types() >>>>> ACPI: video: Remove acpi_video_get_backlight_type() >>>>> ACPI: video: Fallback to native backlight >>>>> >>>>> Documentation/gpu/todo.rst | 8 +-- >>>>> drivers/acpi/acpi_video.c | 2 +- >>>>> drivers/acpi/video_detect.c | 54 >>>>> ++++++++++--------- >>>>> drivers/gpu/drm/i915/display/intel_opregion.c | 3 +- >>>>> drivers/platform/loongarch/loongson-laptop.c | 4 +- >>>>> drivers/platform/x86/acer-wmi.c | 2 +- >>>>> drivers/platform/x86/asus-laptop.c | 2 +- >>>>> drivers/platform/x86/asus-wmi.c | 4 +- >>>>> drivers/platform/x86/compal-laptop.c | 2 +- >>>>> drivers/platform/x86/dell/dell-laptop.c | 2 +- >>>>> drivers/platform/x86/eeepc-laptop.c | 2 +- >>>>> drivers/platform/x86/fujitsu-laptop.c | 4 +- >>>>> drivers/platform/x86/ideapad-laptop.c | 2 +- >>>>> drivers/platform/x86/intel/oaktrail.c | 2 +- >>>>> drivers/platform/x86/msi-laptop.c | 2 +- >>>>> drivers/platform/x86/msi-wmi.c | 2 +- >>>>> .../platform/x86/nvidia-wmi-ec-backlight.c | 2 +- >>>>> drivers/platform/x86/panasonic-laptop.c | 2 +- >>>>> drivers/platform/x86/samsung-laptop.c | 2 +- >>>>> drivers/platform/x86/sony-laptop.c | 2 +- >>>>> drivers/platform/x86/thinkpad_acpi.c | 4 +- >>>>> drivers/platform/x86/toshiba_acpi.c | 2 +- >>>>> drivers/video/backlight/backlight.c | 18 +++++++ >>>>> include/acpi/video.h | 21 ++++---- >>>>> include/linux/backlight.h | 1 + >>>>> 25 files changed, 85 insertions(+), 66 deletions(-) >>>>> >>>> >>> >>
Hi, On 10/24/22 16:31, Akihiko Odaki wrote: > On 2022/10/24 23:06, Akihiko Odaki wrote: >> On 2022/10/24 22:21, Hans de Goede wrote: >>> Hi, >>> >>> On 10/24/22 14:58, Akihiko Odaki wrote: >>>> On 2022/10/24 20:53, Hans de Goede wrote: >>>>> Hi Akihiko, >>>>> >>>>> On 10/24/22 13:34, Akihiko Odaki wrote: >>>>>> Commit 2600bfa3df99 ("ACPI: video: Add acpi_video_backlight_use_native() >>>>>> helper") and following commits made native backlight unavailable if >>>>>> CONFIG_ACPI_VIDEO is set and the backlight feature of ACPI video is >>>>>> unavailable, which broke the backlight functionality on Lenovo ThinkPad >>>>>> C13 Yoga Chromebook. Allow to fall back to native backlight in such >>>>>> cases. >>>>> >>>>> I appreciate your work on this, but what this in essence does is >>>>> it allows 2 backlight drivers (vendor + native) to get registered >>>>> for the same panel again. While the whole goal of the backlight refactor >>>>> series landing in 6.1 was to make it so that there always is only >>>>> *1* backlight device registered instead of (possibly) registering >>>>> multiple and letting userspace figure it out. It is also important >>>>> to only always have 1 backlight device per panel for further >>>>> upcoming changes. >>>>> >>>>> So nack for this solution, sorry. >>>>> >>>>> I am aware that this breaks backlight control on some Chromebooks, >>>>> this was already reported and I wrote a long reply explaining why >>>>> things are done the way they are done now and also suggesting >>>>> 2 possible (much simpler) fixes, see: >>>>> https://lore.kernel.org/linux-acpi/42a5f2c9-a1dc-8fc0-7334-fe6c390ecfbb@redhat.com/ >>>>> >>>>> Unfortunately the reported has not followed-up on this and >>>>> I don't have the hardware to test this myself. >>>>> >>>>> Can you please try implementing 1 of the fixes suggested there >>>>> and then submit that upstream ? >>>>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >>>> >>>> Hi Hans, >>>> >>>> Thanks for reviewing and letting me know the prior attempt. >>>> >>>> In this case, there is only a native backlight device and no vendor backlight device so the duplication of backlight devices does not happen. I think it is better to handle such a case without quirks. >>> >>> Adding a single heuristic for all chromebooks is something completely different >>> then adding per model quirks which indeed ideally should be avoided (but this >>> is not always possible). >>> >>>> I understand it is still questionable to cover the case by allowing duplication when both of a vendor backlight device and native one. To explain my understanding and reasoning for *not* trying to apply the de-duplication rule to the vendor/native combination, let me first describe that the de-duplication which happens in acpi_video_get_backlight_type() is a heuristics and limited. >>>> >>>> As the background of acpi_video_get_backlight_type(), there is an assumption that it should be common that both of the firmware, implementing ACPI, and the kernel have code to drive backlight. In the case, the more reliable one should be picked instead of using both, and that is what the statements in `if (video_caps & ACPI_VIDEO_BACKLIGHT)` does. >>>> >>>> However, the method has two limitations: >>>> 1. It does not cover the case where two backlight devices with the same type exist. >>> >>> This only happens when there are 2 panels; or 2 gpus driving a single panel >>> which are both special cases where we actually want 2 backlight devices. >>> >>>> 2. The underlying assumption does not apply to vendor/native combination. >>>> >>>> Regarding the second limitation, I don't even understand the difference between vendor and native. My guess is that a vendor backlight device uses vendor-specific ACPI interface, and a native one directly uses hardware registers. If my guess is correct, the difference between vendor and native does not imply that both of them are likely to exist at the same time. As the conclusion, there is no more motivation to try to de-duplicate the vendor/native combination than to try to de-duplicate combination of devices with a single type. >>>> >>>> Of course, it is better if we could also avoid registering two devices with one type for one physical device. Possibly we can do so by providing a parameter to indicate that it is for the same "internal" backlight to devm_backlight_device_register(), and let the function check for the duplication. However, this rule may be too restrict, or may have problems I missed. >>>> >>>> Based on the discussion above, we can deduce three possibilities: >>>> a. There is no reason to distinguish vendor and native in this case, and we can stick to my current proposal. >>>> b. There is a valid reason to distinguish vendor and native, and we can adopt the same strategy that already adopted for ACPI video/vendor combination. >>>> c. We can implement de-duplication in devm_backlight_device_register(). >>>> d. The other possible options are not worth, and we can just implement quirks specific to Chromebook/coreboot. >>>> >>>> In case b, it should be noted that vendor and native backlight device do not require ACPI video, and CONFIG_ACPI_VIDEO may not be enabled. In the case, the de-duplication needs to be implemented in backlight class device. >>> >>> I have been working on the ACPI/x86 backlight detection code since 2015, please trust >>> me when I say that allowing both vendor + native backlight devices at the same time >>> is a bad idea. >>> >>> I'm currently in direct contact with the ChromeOS team about fixing the Chromebook >>> backlight issue introduced in 6.1-rc1. >>> >>> If you wan to help, please read: >>> >>> https://lore.kernel.org/linux-acpi/42a5f2c9-a1dc-8fc0-7334-fe6c390ecfbb@redhat.com/ >>> >>> And try implementing 1 if the 2 solutions suggested there. >>> >>> Regards, >>> >>> Hans >> >> Hi, >> >> I just wanted to confirm your intention that we should distinguish vendor and native. In the case I think it is better to modify __acpi_video_get_backlight_type() and add "native_available" check in case of no ACPI video as already done for the ACPI video/native combination. >> >> Unfortunately this has one pitfall though: it does not work if CONFIG_ACPI_VIDEO is disabled. If it is, acpi_video_get_backlight_type() always return acpi_backlight_vendor, and acpi_video_backlight_use_native() always returns true. It is not a regression but the current behavior. Fixing it requires also refactoring touching both of ACPI video and backlight class driver so I guess I'm not an appropriate person to do that, and I should just add "native_available" check to __acpi_video_get_backlight_type(). >> >> Does that sound good? > > Well, it would not be that easy since just adding native_available cannot handle the case where the vendor driver gets registered first. Checking with "native_available" was possible for ACPI video/vendor combination because ACPI video registers its backlight after some delay. I still think it does not overcomplicate things to modify __acpi_video_get_backlight_type() so that it can use both of vendor and native as fallback while preventing duplicate registration. In the mean time this has already been fixed by a single patch of just a few lines, rather then requiring 22 patches, see: https://lore.kernel.org/dri-devel/20221024141210.67784-1-dmitry.osipenko@collabora.com/ Regards, Hans >>>>>> Akihiko Odaki (22): >>>>>> drm/i915/opregion: Improve backlight request condition >>>>>> ACPI: video: Introduce acpi_video_get_backlight_types() >>>>>> LoongArch: Use acpi_video_get_backlight_types() >>>>>> platform/x86: acer-wmi: Use acpi_video_get_backlight_types() >>>>>> platform/x86: asus-laptop: Use acpi_video_get_backlight_types() >>>>>> platform/x86: asus-wmi: Use acpi_video_get_backlight_types() >>>>>> platform/x86: compal-laptop: Use acpi_video_get_backlight_types() >>>>>> platform/x86: eeepc-laptop: Use acpi_video_get_backlight_types() >>>>>> platform/x86: fujitsu-laptop: Use acpi_video_get_backlight_types() >>>>>> platform/x86: ideapad-laptop: Use acpi_video_get_backlight_types() >>>>>> platform/x86: msi-laptop: Use acpi_video_get_backlight_types() >>>>>> platform/x86: msi-wmi: Use acpi_video_get_backlight_types() >>>>>> platform/x86: nvidia-wmi-ec-backlight: Use >>>>>> acpi_video_get_backlight_types() >>>>>> platform/x86: panasonic-laptop: Use acpi_video_get_backlight_types() >>>>>> platform/x86: samsung-laptop: Use acpi_video_get_backlight_types() >>>>>> platform/x86: sony-laptop: Use acpi_video_get_backlight_types() >>>>>> platform/x86: thinkpad_acpi: Use acpi_video_get_backlight_types() >>>>>> platform/x86: toshiba_acpi: Use acpi_video_get_backlight_types() >>>>>> platform/x86: dell-laptop: Use acpi_video_get_backlight_types() >>>>>> platform/x86: intel_oaktrail: Use acpi_video_get_backlight_types() >>>>>> ACPI: video: Remove acpi_video_get_backlight_type() >>>>>> ACPI: video: Fallback to native backlight >>>>>> >>>>>> Documentation/gpu/todo.rst | 8 +-- >>>>>> drivers/acpi/acpi_video.c | 2 +- >>>>>> drivers/acpi/video_detect.c | 54 ++++++++++--------- >>>>>> drivers/gpu/drm/i915/display/intel_opregion.c | 3 +- >>>>>> drivers/platform/loongarch/loongson-laptop.c | 4 +- >>>>>> drivers/platform/x86/acer-wmi.c | 2 +- >>>>>> drivers/platform/x86/asus-laptop.c | 2 +- >>>>>> drivers/platform/x86/asus-wmi.c | 4 +- >>>>>> drivers/platform/x86/compal-laptop.c | 2 +- >>>>>> drivers/platform/x86/dell/dell-laptop.c | 2 +- >>>>>> drivers/platform/x86/eeepc-laptop.c | 2 +- >>>>>> drivers/platform/x86/fujitsu-laptop.c | 4 +- >>>>>> drivers/platform/x86/ideapad-laptop.c | 2 +- >>>>>> drivers/platform/x86/intel/oaktrail.c | 2 +- >>>>>> drivers/platform/x86/msi-laptop.c | 2 +- >>>>>> drivers/platform/x86/msi-wmi.c | 2 +- >>>>>> .../platform/x86/nvidia-wmi-ec-backlight.c | 2 +- >>>>>> drivers/platform/x86/panasonic-laptop.c | 2 +- >>>>>> drivers/platform/x86/samsung-laptop.c | 2 +- >>>>>> drivers/platform/x86/sony-laptop.c | 2 +- >>>>>> drivers/platform/x86/thinkpad_acpi.c | 4 +- >>>>>> drivers/platform/x86/toshiba_acpi.c | 2 +- >>>>>> drivers/video/backlight/backlight.c | 18 +++++++ >>>>>> include/acpi/video.h | 21 ++++---- >>>>>> include/linux/backlight.h | 1 + >>>>>> 25 files changed, 85 insertions(+), 66 deletions(-) >>>>>> >>>>> >>>> >>> >
On 2022/10/24 23:37, Hans de Goede wrote: > Hi, > > On 10/24/22 16:31, Akihiko Odaki wrote: >> On 2022/10/24 23:06, Akihiko Odaki wrote: >>> On 2022/10/24 22:21, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 10/24/22 14:58, Akihiko Odaki wrote: >>>>> On 2022/10/24 20:53, Hans de Goede wrote: >>>>>> Hi Akihiko, >>>>>> >>>>>> On 10/24/22 13:34, Akihiko Odaki wrote: >>>>>>> Commit 2600bfa3df99 ("ACPI: video: Add acpi_video_backlight_use_native() >>>>>>> helper") and following commits made native backlight unavailable if >>>>>>> CONFIG_ACPI_VIDEO is set and the backlight feature of ACPI video is >>>>>>> unavailable, which broke the backlight functionality on Lenovo ThinkPad >>>>>>> C13 Yoga Chromebook. Allow to fall back to native backlight in such >>>>>>> cases. >>>>>> >>>>>> I appreciate your work on this, but what this in essence does is >>>>>> it allows 2 backlight drivers (vendor + native) to get registered >>>>>> for the same panel again. While the whole goal of the backlight refactor >>>>>> series landing in 6.1 was to make it so that there always is only >>>>>> *1* backlight device registered instead of (possibly) registering >>>>>> multiple and letting userspace figure it out. It is also important >>>>>> to only always have 1 backlight device per panel for further >>>>>> upcoming changes. >>>>>> >>>>>> So nack for this solution, sorry. >>>>>> >>>>>> I am aware that this breaks backlight control on some Chromebooks, >>>>>> this was already reported and I wrote a long reply explaining why >>>>>> things are done the way they are done now and also suggesting >>>>>> 2 possible (much simpler) fixes, see: >>>>>> https://lore.kernel.org/linux-acpi/42a5f2c9-a1dc-8fc0-7334-fe6c390ecfbb@redhat.com/ >>>>>> >>>>>> Unfortunately the reported has not followed-up on this and >>>>>> I don't have the hardware to test this myself. >>>>>> >>>>>> Can you please try implementing 1 of the fixes suggested there >>>>>> and then submit that upstream ? >>>>>> >>>>>> Regards, >>>>>> >>>>>> Hans >>>>>> >>>>> >>>>> Hi Hans, >>>>> >>>>> Thanks for reviewing and letting me know the prior attempt. >>>>> >>>>> In this case, there is only a native backlight device and no vendor backlight device so the duplication of backlight devices does not happen. I think it is better to handle such a case without quirks. >>>> >>>> Adding a single heuristic for all chromebooks is something completely different >>>> then adding per model quirks which indeed ideally should be avoided (but this >>>> is not always possible). >>>> >>>>> I understand it is still questionable to cover the case by allowing duplication when both of a vendor backlight device and native one. To explain my understanding and reasoning for *not* trying to apply the de-duplication rule to the vendor/native combination, let me first describe that the de-duplication which happens in acpi_video_get_backlight_type() is a heuristics and limited. >>>>> >>>>> As the background of acpi_video_get_backlight_type(), there is an assumption that it should be common that both of the firmware, implementing ACPI, and the kernel have code to drive backlight. In the case, the more reliable one should be picked instead of using both, and that is what the statements in `if (video_caps & ACPI_VIDEO_BACKLIGHT)` does. >>>>> >>>>> However, the method has two limitations: >>>>> 1. It does not cover the case where two backlight devices with the same type exist. >>>> >>>> This only happens when there are 2 panels; or 2 gpus driving a single panel >>>> which are both special cases where we actually want 2 backlight devices. >>>> >>>>> 2. The underlying assumption does not apply to vendor/native combination. >>>>> >>>>> Regarding the second limitation, I don't even understand the difference between vendor and native. My guess is that a vendor backlight device uses vendor-specific ACPI interface, and a native one directly uses hardware registers. If my guess is correct, the difference between vendor and native does not imply that both of them are likely to exist at the same time. As the conclusion, there is no more motivation to try to de-duplicate the vendor/native combination than to try to de-duplicate combination of devices with a single type. >>>>> >>>>> Of course, it is better if we could also avoid registering two devices with one type for one physical device. Possibly we can do so by providing a parameter to indicate that it is for the same "internal" backlight to devm_backlight_device_register(), and let the function check for the duplication. However, this rule may be too restrict, or may have problems I missed. >>>>> >>>>> Based on the discussion above, we can deduce three possibilities: >>>>> a. There is no reason to distinguish vendor and native in this case, and we can stick to my current proposal. >>>>> b. There is a valid reason to distinguish vendor and native, and we can adopt the same strategy that already adopted for ACPI video/vendor combination. >>>>> c. We can implement de-duplication in devm_backlight_device_register(). >>>>> d. The other possible options are not worth, and we can just implement quirks specific to Chromebook/coreboot. >>>>> >>>>> In case b, it should be noted that vendor and native backlight device do not require ACPI video, and CONFIG_ACPI_VIDEO may not be enabled. In the case, the de-duplication needs to be implemented in backlight class device. >>>> >>>> I have been working on the ACPI/x86 backlight detection code since 2015, please trust >>>> me when I say that allowing both vendor + native backlight devices at the same time >>>> is a bad idea. >>>> >>>> I'm currently in direct contact with the ChromeOS team about fixing the Chromebook >>>> backlight issue introduced in 6.1-rc1. >>>> >>>> If you wan to help, please read: >>>> >>>> https://lore.kernel.org/linux-acpi/42a5f2c9-a1dc-8fc0-7334-fe6c390ecfbb@redhat.com/ >>>> >>>> And try implementing 1 if the 2 solutions suggested there. >>>> >>>> Regards, >>>> >>>> Hans >>> >>> Hi, >>> >>> I just wanted to confirm your intention that we should distinguish vendor and native. In the case I think it is better to modify __acpi_video_get_backlight_type() and add "native_available" check in case of no ACPI video as already done for the ACPI video/native combination. >>> >>> Unfortunately this has one pitfall though: it does not work if CONFIG_ACPI_VIDEO is disabled. If it is, acpi_video_get_backlight_type() always return acpi_backlight_vendor, and acpi_video_backlight_use_native() always returns true. It is not a regression but the current behavior. Fixing it requires also refactoring touching both of ACPI video and backlight class driver so I guess I'm not an appropriate person to do that, and I should just add "native_available" check to __acpi_video_get_backlight_type(). >>> >>> Does that sound good? >> >> Well, it would not be that easy since just adding native_available cannot handle the case where the vendor driver gets registered first. Checking with "native_available" was possible for ACPI video/vendor combination because ACPI video registers its backlight after some delay. I still think it does not overcomplicate things to modify __acpi_video_get_backlight_type() so that it can use both of vendor and native as fallback while preventing duplicate registration. > > In the mean time this has already been fixed by a single patch of just a few > lines, rather then requiring 22 patches, see: > > https://lore.kernel.org/dri-devel/20221024141210.67784-1-dmitry.osipenko@collabora.com/ > > Regards, > > Hans It should be the smaller indeed. Modifying __acpi_video_get_backlight_type() so that it can fall back to either of vendor and native requires all of the vendor drivers to use something like acpi_video_backlight_use_native() but for vendor. It certainly requires 22 patches. That aside, the first patch in this series can be applied without the later patches so you may have a look at it. It's fine if you don't merge it though since it does not fix really a pragmatic bug as its message says. Regards, Akihiko Odaki > > > > >>>>>>> Akihiko Odaki (22): >>>>>>> drm/i915/opregion: Improve backlight request condition >>>>>>> ACPI: video: Introduce acpi_video_get_backlight_types() >>>>>>> LoongArch: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: acer-wmi: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: asus-laptop: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: asus-wmi: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: compal-laptop: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: eeepc-laptop: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: fujitsu-laptop: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: ideapad-laptop: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: msi-laptop: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: msi-wmi: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: nvidia-wmi-ec-backlight: Use >>>>>>> acpi_video_get_backlight_types() >>>>>>> platform/x86: panasonic-laptop: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: samsung-laptop: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: sony-laptop: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: thinkpad_acpi: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: toshiba_acpi: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: dell-laptop: Use acpi_video_get_backlight_types() >>>>>>> platform/x86: intel_oaktrail: Use acpi_video_get_backlight_types() >>>>>>> ACPI: video: Remove acpi_video_get_backlight_type() >>>>>>> ACPI: video: Fallback to native backlight >>>>>>> >>>>>>> Documentation/gpu/todo.rst | 8 +-- >>>>>>> drivers/acpi/acpi_video.c | 2 +- >>>>>>> drivers/acpi/video_detect.c | 54 ++++++++++--------- >>>>>>> drivers/gpu/drm/i915/display/intel_opregion.c | 3 +- >>>>>>> drivers/platform/loongarch/loongson-laptop.c | 4 +- >>>>>>> drivers/platform/x86/acer-wmi.c | 2 +- >>>>>>> drivers/platform/x86/asus-laptop.c | 2 +- >>>>>>> drivers/platform/x86/asus-wmi.c | 4 +- >>>>>>> drivers/platform/x86/compal-laptop.c | 2 +- >>>>>>> drivers/platform/x86/dell/dell-laptop.c | 2 +- >>>>>>> drivers/platform/x86/eeepc-laptop.c | 2 +- >>>>>>> drivers/platform/x86/fujitsu-laptop.c | 4 +- >>>>>>> drivers/platform/x86/ideapad-laptop.c | 2 +- >>>>>>> drivers/platform/x86/intel/oaktrail.c | 2 +- >>>>>>> drivers/platform/x86/msi-laptop.c | 2 +- >>>>>>> drivers/platform/x86/msi-wmi.c | 2 +- >>>>>>> .../platform/x86/nvidia-wmi-ec-backlight.c | 2 +- >>>>>>> drivers/platform/x86/panasonic-laptop.c | 2 +- >>>>>>> drivers/platform/x86/samsung-laptop.c | 2 +- >>>>>>> drivers/platform/x86/sony-laptop.c | 2 +- >>>>>>> drivers/platform/x86/thinkpad_acpi.c | 4 +- >>>>>>> drivers/platform/x86/toshiba_acpi.c | 2 +- >>>>>>> drivers/video/backlight/backlight.c | 18 +++++++ >>>>>>> include/acpi/video.h | 21 ++++---- >>>>>>> include/linux/backlight.h | 1 + >>>>>>> 25 files changed, 85 insertions(+), 66 deletions(-) >>>>>>> >>>>>> >>>>> >>>> >> >
On Tue, 25 Oct 2022, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > That aside, the first patch in this series can be applied without the > later patches so you may have a look at it. It's fine if you don't merge > it though since it does not fix really a pragmatic bug as its message says. I think it's problematic because it needlessly ties i915 backlight operation to existence of backlight devices that may not be related to Intel GPU at all. The direction should be multiple supported backlight devices, across GPUs and connectors, but only one per display. BR, Jani.
On 2022/10/25 3:11, Jani Nikula wrote: > On Tue, 25 Oct 2022, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> That aside, the first patch in this series can be applied without the >> later patches so you may have a look at it. It's fine if you don't merge >> it though since it does not fix really a pragmatic bug as its message says. > > I think it's problematic because it needlessly ties i915 backlight > operation to existence of backlight devices that may not be related to > Intel GPU at all. The direction should be multiple supported backlight > devices, across GPUs and connectors, but only one per display. > > BR, > Jani. > > Unfortunately it is the current situation (even without this patch), and this patch is not meant to fix the particular issue. This patch replaces the following expression: acpi_video_get_backlight_type() == acpi_backlight_native As you can see, acpi_video_get_backlight_type() doesn't take a parameter which represents the backlight currently being operated. The problem is known and documented in "Brightness handling on devices with multiple internal panels" section of Documentation/gpu/todo.rst. The exiting solution is based on the assumption that no device with i915 and multiple internal backlights. Regards, Akihiko Odaki
Commit 2600bfa3df99 ("ACPI: video: Add acpi_video_backlight_use_native() helper") and following commits made native backlight unavailable if CONFIG_ACPI_VIDEO is set and the backlight feature of ACPI video is unavailable, which broke the backlight functionality on Lenovo ThinkPad C13 Yoga Chromebook. Allow to fall back to native backlight in such cases. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Akihiko Odaki (22): drm/i915/opregion: Improve backlight request condition ACPI: video: Introduce acpi_video_get_backlight_types() LoongArch: Use acpi_video_get_backlight_types() platform/x86: acer-wmi: Use acpi_video_get_backlight_types() platform/x86: asus-laptop: Use acpi_video_get_backlight_types() platform/x86: asus-wmi: Use acpi_video_get_backlight_types() platform/x86: compal-laptop: Use acpi_video_get_backlight_types() platform/x86: eeepc-laptop: Use acpi_video_get_backlight_types() platform/x86: fujitsu-laptop: Use acpi_video_get_backlight_types() platform/x86: ideapad-laptop: Use acpi_video_get_backlight_types() platform/x86: msi-laptop: Use acpi_video_get_backlight_types() platform/x86: msi-wmi: Use acpi_video_get_backlight_types() platform/x86: nvidia-wmi-ec-backlight: Use acpi_video_get_backlight_types() platform/x86: panasonic-laptop: Use acpi_video_get_backlight_types() platform/x86: samsung-laptop: Use acpi_video_get_backlight_types() platform/x86: sony-laptop: Use acpi_video_get_backlight_types() platform/x86: thinkpad_acpi: Use acpi_video_get_backlight_types() platform/x86: toshiba_acpi: Use acpi_video_get_backlight_types() platform/x86: dell-laptop: Use acpi_video_get_backlight_types() platform/x86: intel_oaktrail: Use acpi_video_get_backlight_types() ACPI: video: Remove acpi_video_get_backlight_type() ACPI: video: Fallback to native backlight Documentation/gpu/todo.rst | 8 +-- drivers/acpi/acpi_video.c | 2 +- drivers/acpi/video_detect.c | 54 ++++++++++--------- drivers/gpu/drm/i915/display/intel_opregion.c | 3 +- drivers/platform/loongarch/loongson-laptop.c | 4 +- drivers/platform/x86/acer-wmi.c | 2 +- drivers/platform/x86/asus-laptop.c | 2 +- drivers/platform/x86/asus-wmi.c | 4 +- drivers/platform/x86/compal-laptop.c | 2 +- drivers/platform/x86/dell/dell-laptop.c | 2 +- drivers/platform/x86/eeepc-laptop.c | 2 +- drivers/platform/x86/fujitsu-laptop.c | 4 +- drivers/platform/x86/ideapad-laptop.c | 2 +- drivers/platform/x86/intel/oaktrail.c | 2 +- drivers/platform/x86/msi-laptop.c | 2 +- drivers/platform/x86/msi-wmi.c | 2 +- .../platform/x86/nvidia-wmi-ec-backlight.c | 2 +- drivers/platform/x86/panasonic-laptop.c | 2 +- drivers/platform/x86/samsung-laptop.c | 2 +- drivers/platform/x86/sony-laptop.c | 2 +- drivers/platform/x86/thinkpad_acpi.c | 4 +- drivers/platform/x86/toshiba_acpi.c | 2 +- drivers/video/backlight/backlight.c | 18 +++++++ include/acpi/video.h | 21 ++++---- include/linux/backlight.h | 1 + 25 files changed, 85 insertions(+), 66 deletions(-)