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-fe6c390ec...@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-fe6c390ec...@redhat.com/
And try implementing 1 if the 2 solutions suggested there.
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(-)
>>>
>>
>
