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-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
>>
>> 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.osipe...@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(-)
>>>>>>
>>>>>
>>>>
>>>
>
