On 2022/10/24 23:06, Akihiko Odaki wrote:

On 2022/10/24 22:21, Hans de Goede wrote:

quoted

Hi,

On 10/24/22 14:58, Akihiko Odaki wrote:

quoted

On 2022/10/24 20:53, Hans de Goede wrote:

quoted

Hi Akihiko,

On 10/24/22 13:34, Akihiko Odaki wrote:

quoted

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/ (local)

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).

quoted

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.

quoted

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/ (local)

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

quoted

 >

quoted

quoted

quoted

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(-)