[PATCH v3 0/5] drm/gma500: Backlight handling changes

Sun Sep 18 18:54:16 UTC 2022

Hi Patrik,

On 9/18/22 20:22, Patrik Jakobsson wrote:
> On Sat, Sep 17, 2022 at 10:59 PM Hans de Goede <hdegoede at redhat.com> wrote:
>>
>> Hi All,
>>
>> Here is a patch-series changing gma500's backlight handling to match
>> the changes done to the other major x86 GPU drivers in the just landed
>> backlight detection refactor patch series:
>> https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/
>>
>> The main goal is here is to only register one backlight class device instead
>> of registering both "acpi_video0" and "psb-bl" backlight class devices;
>> in preparation for implementing the new backlight userspace-API from:
>> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/
> 
> Hi Hans,
> 
> Quite some time ago I wrote a backlight driver [1] for a MacBook to
> work around an issue in the i915 driver. My driver spoke directly to
> an external backlight driver chip. By doing so I could turn off the
> signal coming from the GPU and instead program my own PWM value
> directly. I remember it being a bit tricky to make my driver get
> priority over the i915 driver. Not sure what the actual issue was but
> I did get it to work properly in the end (perhaps with an xorg.conf
> change).
> 
> I understand that this is a corner case but I'm just curious how/if
> this can be handled with the new API. Is it possible to kick out an
> existing non-acpi backlight driver if you know yours is better?
> 
> [1] https://github.com/patjak/mba6x_bl

After the main refactoring series which is in linux-next now:
https://lore.kernel.org/all/20220825143726.269890-1-hdegoede@redhat.com/
this should be possible, it should be easy even.

On x86/ACPI platforms the idea is that all backlight-drivers there
call acpi_video_get_backlight_type() which returns one of:

enum acpi_backlight_type {
        acpi_backlight_none = 0,
        acpi_backlight_video,
        acpi_backlight_vendor,
        acpi_backlight_native,
        acpi_backlight_nvidia_wmi_ec,
        acpi_backlight_apple_gmux,
};

(defined in include/acpi/video.h)

And then when acpi_video_get_backlight_type() returns a type which
does not match the driver which calls it, then that driver will
return without registering its backlight sysfs interface.

E.g. drivers/acpi/acpi_video.c does:

        if (acpi_video_get_backlight_type() != acpi_backlight_video)
                return 0;

before registering the /sys/class/backlight/acpi_video# interface(s).

And likewise the i915 driver now does:

        if (!acpi_video_backlight_use_native())
                return 0;

(Note native (GPU) backlight drivers use a special helper since that
helper also lets the video-detect code now that the GPU driver is
capable of doing native backlight control which is part of the heuristics
to pick which backlight control method to use).

So getting i915 out of the way now is as simple as making
acpi_video_get_backlight_type() return something else then 
acpi_backlight_native which can be done through e.g. a DMI quirk.

The acpi_backlight_vendor type here is typically used by backlight code
in drivers like dell-laptop acer-wmi, asus-wmi, thinkpad_acpi, etc.
None of which I expect to load on your macbook.

So you could make your mba6x_bl have a:

        if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
                return 0;

Check to avoid registering. All backlight drivers used on x86/acpi
platforms should have such a check going forward so that
acpi_video_get_backlight_type() is the sole place in the kernel
which decides which backlight-control method actually gets registered.

And then pass: "acpi_backlight=vendor" on the kernel commandline to
switch from the default to your driver (and users can also use this
to switch back once you have made vendor the default on affected
macbooks).

If this works (which I expect it will) and once you have your driver
merged into the mainline kernel you can then add DMI based quirks to
drivers/acpi/video_detect.c to default to acpi_backlight=vendor on
these macbooks (I see that you already use DMI based auto-loading
in your driver).

So as you can hopefully see in linux-next with my refactoring in
place having another driver take over from the i915 driver should
be simple since there is a single point now
(acpi_video_get_backlight_type()) which controls which driver will
load and which ones will not load.

Regards,

Hans

p.s.

Note the above has nothing to do with the new userspace API for
backlight control. But sorting out there being multiple drivers
loaded/registered at the same time for a single panel/backlight
was a prerequisite for the new userspace API.
For the proposed new userspace API see:
https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/

> 
>>
>> Changes in v2:
>> - Add "Use backlight_get_brightness() to get the brightness" patch
>>
>> Changes in v3:
>> - Fix unused variable warnings when CONFIG_BACKLIGHT is not selected by
>>   marking the 2 variables as  __maybe_unused.
> 
> This looks good to me. I don't have access to my DIM setup in a couple
> of days so please push these yourself if possible.
> 
> For the entire series:
> Acked-by: Patrik Jakobsson <patrik.r.jakobsson at gmail.com>
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>> Hans de Goede (5):
>>   drm/gma500: Refactor backlight support (v2)
>>   drm/gma500: Change registered backlight device type to raw/native
>>   drm/gma500: Use backlight_get_brightness() to get the brightness
>>   drm/gma500: Don't register backlight when another backlight should be
>>     used
>>   drm/gma500: Call acpi_video_register_backlight()
>>
>>  drivers/gpu/drm/gma500/backlight.c       | 102 +++++++++++++++--------
>>  drivers/gpu/drm/gma500/cdv_device.c      |  50 ++---------
>>  drivers/gpu/drm/gma500/oaktrail_device.c |  65 ++-------------
>>  drivers/gpu/drm/gma500/opregion.c        |   6 +-
>>  drivers/gpu/drm/gma500/psb_device.c      |  73 +---------------
>>  drivers/gpu/drm/gma500/psb_drv.c         |  15 +---
>>  drivers/gpu/drm/gma500/psb_drv.h         |  13 +--
>>  7 files changed, 97 insertions(+), 227 deletions(-)
>>
>> --
>> 2.37.3
>>
> 