Hi,

On 5/18/22 10:55, Jani Nikula wrote:

On Tue, 17 May 2022, Hans de Goede hdegoede@redhat.com wrote:

...

ATM on x86 laptops where we want userspace to use the acpi_video backlight device we often register both the GPU's native backlight device and acpi_video's firmware acpi_video# backlight device. This relies on userspace preferring firmware type backlight devices over native ones, but registering 2 backlight devices for a single display really is undesirable.

On x86 laptops where the native GPU backlight device should be used, the registering of other backlight devices is avoided by their drivers using acpi_video_get_backlight_type() and only registering their backlight if the return value matches their type.

acpi_video_get_backlight_type() uses backlight_device_get_by_type(BACKLIGHT_RAW) to determine if a native driver is available and will never return native if this returns false. This means that the GPU's native backlight registering code cannot just call acpi_video_get_backlight_type() to determine if it should register its backlight, since acpi_video_get_backlight_type() will never return native until the native backlight has already registered.

To fix this add a native function parameter to acpi_video_get_backlight_type(), which when set to true will make acpi_video_get_backlight_type() behave as if a native backlight has already been registered.

Note that all current callers are updated to pass false for the new parameter, so this change in itself causes no functional changes.

...

diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index becc198e4c22..0a06f0edd298 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -17,12 +17,14 @@

• Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop,
• sony_acpi,... can take care about backlight brightness.

• • Backlight drivers can use acpi_video_get_backlight_type() to determine
• • which driver should handle the backlight.

• • Backlight drivers can use acpi_video_get_backlight_type() to determine which
• • driver should handle the backlight. RAW/GPU-driver backlight drivers must
• • pass true for the native function argument, other drivers must pass false.
  • If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m)
  • this file will not be compiled and acpi_video_get_backlight_type() will

• • always return acpi_backlight_vendor.

• • return acpi_backlight_native when its native argument is true and
• • acpi_backlight_vendor when it is false.
  */

Frankly, I think the boolean native parameter here, and at the call sites, is confusing, and the slightly different explanations in the commit message and comment here aren't helping.

Can you elaborate the "slightly different explanations in the commit message and comment" part a bit (so that I can fix this) ?

I suggest adding a separate function that the native backlight drivers should use. I think it's more obvious all around, and easier to document too.

Code wise I think this would mean renaming the original and then adding 2 wrappers, but that is fine with me. I've no real preference either way and I'm happy with adding a new variant of acpi_video_get_backlight_type() for the native backlight drivers any suggestion for a name ?

Regards,

Hans

Hi,

On 5/19/22 11:02, Jani Nikula wrote:

On Wed, 18 May 2022, Hans de Goede hdegoede@redhat.com wrote:

...

Hi,

On 5/18/22 10:55, Jani Nikula wrote:

...

On Tue, 17 May 2022, Hans de Goede hdegoede@redhat.com wrote:

...

ATM on x86 laptops where we want userspace to use the acpi_video backlight device we often register both the GPU's native backlight device and acpi_video's firmware acpi_video# backlight device. This relies on userspace preferring firmware type backlight devices over native ones, but registering 2 backlight devices for a single display really is undesirable.

On x86 laptops where the native GPU backlight device should be used, the registering of other backlight devices is avoided by their drivers using acpi_video_get_backlight_type() and only registering their backlight if the return value matches their type.

acpi_video_get_backlight_type() uses backlight_device_get_by_type(BACKLIGHT_RAW) to determine if a native driver is available and will never return native if this returns false. This means that the GPU's native backlight registering code cannot just call acpi_video_get_backlight_type() to determine if it should register its backlight, since acpi_video_get_backlight_type() will never return native until the native backlight has already registered.

To fix this add a native function parameter to acpi_video_get_backlight_type(), which when set to true will make acpi_video_get_backlight_type() behave as if a native backlight has already been registered.

Regarding the question below, this is the part that throws me off.

...

...

...

Note that all current callers are updated to pass false for the new parameter, so this change in itself causes no functional changes.

...

diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index becc198e4c22..0a06f0edd298 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -17,12 +17,14 @@

• Otherwise vendor specific drivers like thinkpad_acpi, asus-laptop,
• sony_acpi,... can take care about backlight brightness.

• • Backlight drivers can use acpi_video_get_backlight_type() to determine
• • which driver should handle the backlight.

• • Backlight drivers can use acpi_video_get_backlight_type() to determine which
• • driver should handle the backlight. RAW/GPU-driver backlight drivers must
• • pass true for the native function argument, other drivers must pass false.
  • If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m)
  • this file will not be compiled and acpi_video_get_backlight_type() will

• • always return acpi_backlight_vendor.

• • return acpi_backlight_native when its native argument is true and
• • acpi_backlight_vendor when it is false.
  */

Frankly, I think the boolean native parameter here, and at the call sites, is confusing, and the slightly different explanations in the commit message and comment here aren't helping.

Can you elaborate the "slightly different explanations in the commit message and comment" part a bit (so that I can fix this) ?

...

I suggest adding a separate function that the native backlight drivers should use. I think it's more obvious all around, and easier to document too.

Code wise I think this would mean renaming the original and then adding 2 wrappers, but that is fine with me. I've no real preference either way and I'm happy with adding a new variant of acpi_video_get_backlight_type() for the native backlight drivers any suggestion for a name ?

Alternatively, do the native backlight drivers have any need for the actual backlight type information from acpi? They only need to be able to ask if they should register themselves, right?

I understand this sounds like bikeshedding, but I'm trying to avoid duplicating the conditions in the drivers where a single predicate function call could be sufficient, and the complexity could be hidden in acpi.

if (!acpi_video_backlight_use_native()) return;

acpi_video_backlight_use_native() sounds good, I like I will change this for v2. This also removes churn in all the other acpi_video_get_backlight_type() callers.

Perhaps all the drivers/platform/x86/* backlight drivers could use:

if (acpi_video_backlight_use_vendor()) ...

Hmm, as part of the ractoring there also will be new apple_gmux and nvidia_wmi_ec types. I'm not sure about adding seperate functions for all of those vs get_type() != foo. I like get_type != foo because it makes clear that there will also be another caller somewhere where get_type == foo and that that one will rbe the one which actually gets to register its backlight.

You can still use the native parameter etc. internally, but just hide the details from everyone else, and, hopefully, make it harder for them to do silly things?

Ack.

Regards,

Hans

On 5/17/22 10:23, Hans de Goede wrote:

On x86/ACPI boards the acpi_video driver will usually initializing before the kms driver (except i915). This causes /sys/class/backlight/acpi_video0 to show up and then the kms driver registers its own native backlight device after which the drivers/acpi/video_detect.c code unregisters the acpi_video0 device (when acpi_video_get_backlight_type()==native).

This means that userspace briefly sees 2 devices and the disappearing of acpi_video0 after a brief time confuses the systemd backlight level save/restore code, see e.g.: https://bbs.archlinux.org/viewtopic.php?id=269920

To fix this make backlight class device registration a separate step done by a new acpi_video_register_backlight() function. The intend is for this to be called by the drm/kms driver *after* it is done setting up its own native backlight device. So that acpi_video_get_backlight_type() knows if a native backlight will be available or not at acpi_video backlight registration time, avoiding the add + remove dance.

If I'm understanding this correctly, it seems we will want to call acpi_video_register_backlight() from the NVIDIA proprietary driver in a fallback path in case the driver's own GPU-controlled backlight handler either should not be used, or fails to register. That sounds reasonable enough, but I'm not sure what should be done about drivers like nvidia-wmi-ec-backlight, which are independent of the GPU hardware, and wouldn't be part of the acpi_video driver, either. There are a number of other similar vendor-y/platform-y type backlight drivers in drivers/video/backlight and drivers/platform/x86 that I think would be in a similar situation.

From a quick skim of the ACPI video driver, it seems that perhaps nvidia-wmi-ec-backlight is missing a call to acpi_video_set_dmi_backlight_type(), perhaps with the acpi_backlight_vendor value? But I'm not familiar enough with this code to be sure that nobody will be checking acpi_video_get_backlight_type() before nvidia-wmi-ec-backlight loads. I'll take a closer look to try to convince myself that it makes sense.

Note the new acpi_video_register_backlight() function is also called from a delayed work to ensure that the acpi_video backlight devices does get registered if necessary even if there is no drm/kms driver or when it is disabled.

It sounds like maybe everything should be fine as long as nvidia-wmi-ec-backlight (and other vendor-y/platform-y type drivers) gets everything set up before the delayed work which calls acpi_video_register_backlight()? But then is it really necessary to explicitly call acpi_video_register_backlight() from the DRM drivers if it's going to be called later if no GPU driver registered a backlight handler, anyway? Then we'd just need to make sure that the iGPU and dGPU drivers won't attempt to register a backlight handler on systems where a vendor-y/platform-y driver is supposed to handle the backlight instead, which sounds like it has the potential to be quite messy.

Recall that on at least one system, both amdgpu and the NVIDIA proprietary driver registered a handler even when it shouldn't: https://patchwork.kernel.org/project/platform-driver-x86/patch/2022031620332... - I didn't have direct access to this system, but the fact that the NVIDIA driver registered a handler was almost certainly a bug in either the driver or the system's firmware (on other systems with the same type of backlight hardware, NVIDIA does not register a handler), and I imagine the same is true of the amdgpu driver. In all likelihood nouveau would have probably tried to register one too; I am not certain whether the person who reported the issue to me had tested with nouveau. I'm not convinced that the GPU drivers can reliably determine whether or not they are supposed to register, but maybe cases where they aren't, such as the system mentioned above, are supposed to be handled in a quirks table somewhere.

Signed-off-by: Hans de Goede hdegoede@redhat.com

drivers/acpi/acpi_video.c | 45 ++++++++++++++++++++++++++++++++++++--- include/acpi/video.h | 2 ++ 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c index 95d4868f6a8c..79e75dc86243 100644 --- a/drivers/acpi/acpi_video.c +++ b/drivers/acpi/acpi_video.c @@ -31,6 +31,12 @@ #define ACPI_VIDEO_BUS_NAME "Video Bus" #define ACPI_VIDEO_DEVICE_NAME "Video Device"

+/*

• • Display probing is known to take up to 5 seconds, so delay the fallback
• • backlight registration by 5 seconds + 3 seconds for some extra margin.
• */

+#define ACPI_VIDEO_REGISTER_BACKLIGHT_DELAY (8 * HZ)

• #define MAX_NAME_LEN 20

  MODULE_AUTHOR("Bruno Ducrot");

@@ -80,6 +86,9 @@ static LIST_HEAD(video_bus_head); static int acpi_video_bus_add(struct acpi_device *device); static int acpi_video_bus_remove(struct acpi_device *device); static void acpi_video_bus_notify(struct acpi_device *device, u32 event); +static void acpi_video_bus_register_backlight_work(struct work_struct *ignored); +static DECLARE_DELAYED_WORK(video_bus_register_backlight_work,

• 	    acpi_video_bus_register_backlight_work);
  

  void acpi_video_detect_exit(void);

  /*

@@ -1862,8 +1871,6 @@ static int acpi_video_bus_register_backlight(struct acpi_video_bus *video) if (video->backlight_registered) return 0;

• acpi_video_run_bcl_for_osi(video);
• if (acpi_video_get_backlight_type(false) != acpi_backlight_video) return 0;

@@ -2089,7 +2096,11 @@ static int acpi_video_bus_add(struct acpi_device *device) list_add_tail(&video->entry, &video_bus_head); mutex_unlock(&video_list_lock);

• acpi_video_bus_register_backlight(video);

• /*

• * The userspace visible backlight_device gets registered separately
  

• * from acpi_video_register_backlight().
  

• */
  

• acpi_video_run_bcl_for_osi(video); acpi_video_bus_add_notify_handler(video);

  return 0;

@@ -2128,6 +2139,11 @@ static int acpi_video_bus_remove(struct acpi_device *device) return 0; }

+static void acpi_video_bus_register_backlight_work(struct work_struct *ignored) +{

• acpi_video_register_backlight();

+}

• static int __init is_i740(struct pci_dev *dev) { if (dev->device == 0x00D1)

@@ -2238,6 +2254,17 @@ int acpi_video_register(void) */ register_count = 1;

• /*

• * acpi_video_bus_add() skips registering the userspace visible
  

• * backlight_device. The intend is for this to be registered by the
  

• * drm/kms driver calling acpi_video_register_backlight() *after* it is
  

• * done setting up its own native backlight device. The delayed work
  

• * ensures that acpi_video_register_backlight() always gets called
  

• * eventually, in case there is no drm/kms driver or it is disabled.
  

• */
  

• schedule_delayed_work(&video_bus_register_backlight_work,

• 	      ACPI_VIDEO_REGISTER_BACKLIGHT_DELAY);
  

• leave: mutex_unlock(&register_count_mutex); return ret;

@@ -2248,6 +2275,7 @@ void acpi_video_unregister(void) { mutex_lock(&register_count_mutex); if (register_count) {

• cancel_delayed_work_sync(&video_bus_register_backlight_work);
  

  acpi_bus_unregister_driver(&acpi_video_bus); register_count = 0; }

@@ -2255,6 +2283,17 @@ void acpi_video_unregister(void) } EXPORT_SYMBOL(acpi_video_unregister);

+void acpi_video_register_backlight(void) +{

• struct acpi_video_bus *video;
• mutex_lock(&video_list_lock);
• list_for_each_entry(video, &video_bus_head, entry)

• acpi_video_bus_register_backlight(video);
  

• mutex_unlock(&video_list_lock);

+} +EXPORT_SYMBOL(acpi_video_register_backlight);

• void acpi_video_unregister_backlight(void) { struct acpi_video_bus *video;

diff --git a/include/acpi/video.h b/include/acpi/video.h index e31afb93379a..b2f7dc1f354a 100644 --- a/include/acpi/video.h +++ b/include/acpi/video.h @@ -53,6 +53,7 @@ enum acpi_backlight_type { #if IS_ENABLED(CONFIG_ACPI_VIDEO) extern int acpi_video_register(void); extern void acpi_video_unregister(void); +extern void acpi_video_register_backlight(void); extern int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, void **edid); extern enum acpi_backlight_type acpi_video_get_backlight_type(bool native); @@ -68,6 +69,7 @@ extern int acpi_video_get_levels(struct acpi_device *device, #else static inline int acpi_video_register(void) { return -ENODEV; } static inline void acpi_video_unregister(void) { return; } +static inline void acpi_video_register_backlight(void) { return; } static inline int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, void **edid) {

Hi,

On 5/24/22 01:25, Daniel Dadap wrote:

On 5/20/22 16:41, Daniel Dadap wrote:

...

On 5/17/22 10:23, Hans de Goede wrote:

...

On x86/ACPI boards the acpi_video driver will usually initializing before the kms driver (except i915). This causes /sys/class/backlight/acpi_video0 to show up and then the kms driver registers its own native backlight device after which the drivers/acpi/video_detect.c code unregisters the acpi_video0 device (when acpi_video_get_backlight_type()==native).

This means that userspace briefly sees 2 devices and the disappearing of acpi_video0 after a brief time confuses the systemd backlight level save/restore code, see e.g.: https://bbs.archlinux.org/viewtopic.php?id=269920

To fix this make backlight class device registration a separate step done by a new acpi_video_register_backlight() function. The intend is for this to be called by the drm/kms driver *after* it is done setting up its own native backlight device. So that acpi_video_get_backlight_type() knows if a native backlight will be available or not at acpi_video backlight registration time, avoiding the add + remove dance.

If I'm understanding this correctly, it seems we will want to call acpi_video_register_backlight() from the NVIDIA proprietary driver in a fallback path in case the driver's own GPU-controlled backlight handler either should not be used, or fails to register. That sounds reasonable enough, but I'm not sure what should be done about drivers like nvidia-wmi-ec-backlight, which are independent of the GPU hardware, and wouldn't be part of the acpi_video driver, either. There are a number of other similar vendor-y/platform-y type backlight drivers in drivers/video/backlight and drivers/platform/x86 that I think would be in a similar situation.

From a quick skim of the ACPI video driver, it seems that perhaps nvidia-wmi-ec-backlight is missing a call to acpi_video_set_dmi_backlight_type(), perhaps with the acpi_backlight_vendor value? But I'm not familiar enough with this code to be sure that nobody will be checking acpi_video_get_backlight_type() before nvidia-wmi-ec-backlight loads. I'll take a closer look to try to convince myself that it makes sense.

...

Note the new acpi_video_register_backlight() function is also called from a delayed work to ensure that the acpi_video backlight devices does get registered if necessary even if there is no drm/kms driver or when it is disabled.

It sounds like maybe everything should be fine as long as nvidia-wmi-ec-backlight (and other vendor-y/platform-y type drivers) gets everything set up before the delayed work which calls acpi_video_register_backlight()? But then is it really necessary to explicitly call acpi_video_register_backlight() from the DRM drivers if it's going to be called later if no GPU driver registered a backlight handler, anyway? Then we'd just need to make sure that the iGPU and dGPU drivers won't attempt to register a backlight handler on systems where a vendor-y/platform-y driver is supposed to handle the backlight instead, which sounds like it has the potential to be quite messy.

Ah, I see you addressed this concern in your RFC (sorry for missing that, earlier):

...

The above only takes native vs acpi_video backlight issues into account, there are also a couple of other scenarios which may lead to multiple backlight-devices getting registered:

1. Apple laptops using the apple_gmux driver
2. Nvidia laptops using the (new) nvidia-wmi-ec-backlight driver
3. drivers/platform/x86 drivers calling acpi_video_set_dmi_backlight_type() to override the normal acpi_video_get_backlight_type() heuristics after the native/acpi_video drivers have already loaded

The plan for 1) + 2) is to extend the acpi_backlight_type enum with acpi_backlight_gmux and acpi_backlight_nvidia_wmi_ec values and to add detection of these 2 to acpi_video_get_backlight_type().

Is there a reason these shouldn't have the same, generic, type, rather than separate, driver-specific ones?

In case it is not clear, this needs to be separate from "vendor" because vendor is meant for the old (often pre windows XP) vendor specific BIOS interfaces used by the likes of dell-laptopo, thinkpad_acpi, etc. And acpi_video_get_backlight_type() returns vendor when it cannot find any other types, iow it is the type of last resort.

So vendor is the fallback where as nvidia_wmi_ec and apple_gmux both must take precedence over anything else if detected.

As for why not have a single type for nvidia_wmi_ec and apple_gmux, these will have 2 separate detection helper functions, so it seems cleaner to me to use 2 separate types to match this. Most drivers check for type != my-type, so an extra type does not hurt.

I don't foresee any situation where a system would need to use these two particular drivers simultaneously.

Agreed.

Multiple DRM drivers can coexist on the same system, and even though the goal here is to remove the existing "multiple backlight interfaces for the same panel" situation, there shouldn't be any reason why more than one DRM driver couldn't register backlight handlers at the same time, so long as they are driving distinct panels. If we don't need separate backlight types for individual DRM drivers, why should we have them for apple_gmux and nvidia_wmi_ec_backlight?

I don't think we need them, but as said since they use separate detection methods, it just feels cleaner.

The drivers/acpi/video_detect.c code at the moment has the following detection methods:

1. Check if any GPU drivers have *told* it that the GPU driver will register a GPU native backlight device for the panel (either direct PWM driving or over DP aux, etc.)

2. Check if the ACPI tables have the ACPI video backlight control bits

3. None of the above, assume vendor.

For nvidia-wmi-ec and apple-gmux 2 separate detection helper functions + steps will get added and as said it just feels cleaner to have 2 separate types to match.

I still think that relying on the GPU drivers to correctly know whether they should register their backlight handlers when a platform-level device is supposed to register one instead might be easier said than done, especially on systems where the same panel could potentially be driven by more than one GPU (but not at the same time).

ATM the GPU drivers unconditionally register their native backlight device if they believe (e.g. the video bios tables say so) they can control the backlight.

And then in the case of e.g. windows XP era laptops, where often the EC was still used, the GPU drivers atm rely on acpi_video also registering a backlight device and userspace then preferring that one.

IOW atm the native GPU drivers rely on userspace ignoring their backlight device if another one is present.

The adding of something like e.g.:

if (acpi_video_get_backlight_type(true) != acpi_backlight_native) { NV_INFO(drm, "Skipping nv_backlight registration\n"); goto fail_alloc; }

To the code-paths doing the backlight-device registration, skipping tthe registration just replaces the "lets hope userspace ignores this if necessary" with outright skipping the registration of the sysfs backlight class device in the cases where before we would hope userspace behaves as expected.

Also note that this just skips the registration of the sysfs class device, any prep work is still done, so as to not cause any behavior changes from the pov of which GPU registers get poked.

...

Recall that on at least one system, both amdgpu and the NVIDIA proprietary driver registered a handler even when it shouldn't: https://patchwork.kernel.org/project/platform-driver-x86/patch/2022031620332... - I didn't have direct access to this system, but the fact that the NVIDIA driver registered a handler was almost certainly a bug in either the driver or the system's firmware (on other systems with the same type of backlight hardware, NVIDIA does not register a handler), and I imagine the same is true of the amdgpu driver. In all likelihood nouveau would have probably tried to register one too; I am not certain whether the person who reported the issue to me had tested with nouveau. I'm not convinced that the GPU drivers can reliably determine whether or not they are supposed to register, but maybe cases where they aren't, such as the system mentioned above, are supposed to be handled in a quirks table somewhere.

Right video_detect.c already has a DMI table for this which overrides the value returned by acpi_video_get_backlight_type().

Although in this specific case it seems we may want acpi_video_get_backlight_type() to return native when called from the amdgpu driver and none when called from the nvidia/nouveau driver ? That is not supported atm, but if necessary it seems reasonable to:

1. Add a "const char *driver_name" to acpi_video_get_backlight_type() and maybe agree on nouveau and nvidia to pass the same value.

2. Extend the quirk mechanism to allow using the driver_name to return different results to different drivers.

Regards,

Hans

...

...

Signed-off-by: Hans de Goede hdegoede@redhat.com

drivers/acpi/acpi_video.c | 45 ++++++++++++++++++++++++++++++++++++---   include/acpi/video.h      |  2 ++   2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c index 95d4868f6a8c..79e75dc86243 100644 --- a/drivers/acpi/acpi_video.c +++ b/drivers/acpi/acpi_video.c @@ -31,6 +31,12 @@   #define ACPI_VIDEO_BUS_NAME        "Video Bus"   #define ACPI_VIDEO_DEVICE_NAME        "Video Device"   +/*

• • Display probing is known to take up to 5 seconds, so delay the fallback
• • backlight registration by 5 seconds + 3 seconds for some extra margin.
• */

+#define ACPI_VIDEO_REGISTER_BACKLIGHT_DELAY    (8 * HZ)

#define MAX_NAME_LEN    20     MODULE_AUTHOR("Bruno Ducrot"); @@ -80,6 +86,9 @@ static LIST_HEAD(video_bus_head);   static int acpi_video_bus_add(struct acpi_device *device);   static int acpi_video_bus_remove(struct acpi_device *device);   static void acpi_video_bus_notify(struct acpi_device *device, u32 event); +static void acpi_video_bus_register_backlight_work(struct work_struct *ignored); +static DECLARE_DELAYED_WORK(video_bus_register_backlight_work, +                acpi_video_bus_register_backlight_work);   void acpi_video_detect_exit(void);     /* @@ -1862,8 +1871,6 @@ static int acpi_video_bus_register_backlight(struct acpi_video_bus *video)       if (video->backlight_registered)           return 0;   -    acpi_video_run_bcl_for_osi(video);

if (acpi_video_get_backlight_type(false) != acpi_backlight_video)           return 0;   @@ -2089,7 +2096,11 @@ static int acpi_video_bus_add(struct acpi_device *device)       list_add_tail(&video->entry, &video_bus_head);       mutex_unlock(&video_list_lock);   -    acpi_video_bus_register_backlight(video); +    /* +     * The userspace visible backlight_device gets registered separately +     * from acpi_video_register_backlight(). +     */ +    acpi_video_run_bcl_for_osi(video);       acpi_video_bus_add_notify_handler(video);         return 0; @@ -2128,6 +2139,11 @@ static int acpi_video_bus_remove(struct acpi_device *device)       return 0;   }   +static void acpi_video_bus_register_backlight_work(struct work_struct *ignored) +{ +    acpi_video_register_backlight(); +}

static int __init is_i740(struct pci_dev *dev)   {       if (dev->device == 0x00D1) @@ -2238,6 +2254,17 @@ int acpi_video_register(void)        */       register_count = 1;   +    /* +     * acpi_video_bus_add() skips registering the userspace visible +     * backlight_device. The intend is for this to be registered by the +     * drm/kms driver calling acpi_video_register_backlight() *after* it is +     * done setting up its own native backlight device. The delayed work +     * ensures that acpi_video_register_backlight() always gets called +     * eventually, in case there is no drm/kms driver or it is disabled. +     */ +    schedule_delayed_work(&video_bus_register_backlight_work, +                  ACPI_VIDEO_REGISTER_BACKLIGHT_DELAY);

leave:       mutex_unlock(&register_count_mutex);       return ret; @@ -2248,6 +2275,7 @@ void acpi_video_unregister(void)   {       mutex_lock(&register_count_mutex);       if (register_count) { +        cancel_delayed_work_sync(&video_bus_register_backlight_work);           acpi_bus_unregister_driver(&acpi_video_bus);           register_count = 0;       } @@ -2255,6 +2283,17 @@ void acpi_video_unregister(void)   }   EXPORT_SYMBOL(acpi_video_unregister);   +void acpi_video_register_backlight(void) +{ +    struct acpi_video_bus *video;

+    mutex_lock(&video_list_lock); +    list_for_each_entry(video, &video_bus_head, entry) +        acpi_video_bus_register_backlight(video); +    mutex_unlock(&video_list_lock); +} +EXPORT_SYMBOL(acpi_video_register_backlight);

void acpi_video_unregister_backlight(void)   {       struct acpi_video_bus *video; diff --git a/include/acpi/video.h b/include/acpi/video.h index e31afb93379a..b2f7dc1f354a 100644 --- a/include/acpi/video.h +++ b/include/acpi/video.h @@ -53,6 +53,7 @@ enum acpi_backlight_type {   #if IS_ENABLED(CONFIG_ACPI_VIDEO)   extern int acpi_video_register(void);   extern void acpi_video_unregister(void); +extern void acpi_video_register_backlight(void);   extern int acpi_video_get_edid(struct acpi_device *device, int type,                      int device_id, void **edid);   extern enum acpi_backlight_type acpi_video_get_backlight_type(bool native); @@ -68,6 +69,7 @@ extern int acpi_video_get_levels(struct acpi_device *device,   #else   static inline int acpi_video_register(void) { return -ENODEV; }   static inline void acpi_video_unregister(void) { return; } +static inline void acpi_video_register_backlight(void) { return; }   static inline int acpi_video_get_edid(struct acpi_device *device, int type,                         int device_id, void **edid)   {

On Tue, 2022-05-17 at 17:23 +0200, Hans de Goede wrote:

Typically the acpi_video driver will initialize before nouveau, which used to cause /sys/class/backlight/acpi_video0 to get registered and then nouveau would register its own nv_backlight device later. After which the drivers/acpi/video_detect.c code unregistered the acpi_video0 device to avoid there being 2 backlight devices.

This means that userspace used to briefly see 2 devices and the disappearing of acpi_video0 after a brief time confuses the systemd backlight level save/restore code, see e.g.: https://bbs.archlinux.org/viewtopic.php?id=269920

To fix this the ACPI video code has been modified to make backlight class device registration a separate step, relying on the drm/kms driver to ask for the acpi_video backlight registration after it is done setting up its native backlight device.

Add a call to the new acpi_video_register_backlight() when native backlight device registration has failed / was skipped to ensure that there is a backlight device available before the drm_device gets registered with userspace.

Signed-off-by: Hans de Goede hdegoede@redhat.com

drivers/gpu/drm/nouveau/nouveau_backlight.c | 7 +++++++  1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c index f56ff797c78c..0ae8793357a4 100644 --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c @@ -436,6 +436,13 @@ nouveau_backlight_init(struct drm_connector *connector)    fail_alloc:         kfree(bl); +       /* +        * If we get here we have an internal panel, but no nv_backlight, +        * try registering an ACPI video backlight device instead. +        */ +       if (ret == 0) +               acpi_video_register_backlight();

Assuming we don't need to return the value of acpi_video_register_backlight() here:

Reviewed-by: Lyude Paul lyude@redhat.com

return ret;  }