Hi Patrik, On 9/18/22 20:22, Patrik Jakobsson wrote: > On Sat, Sep 17, 2022 at 10:59 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx/ >> >> 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@xxxxxxxxxx/ > > 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@xxxxxxxxxx/ 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@xxxxxxxxxx/ > >> >> 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@xxxxxxxxx> > >> >> 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 >> >
