| Message ID | 20220824121523.1291269-6-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/kms: Stop registering multiple /sys/class/backlight devs for a single display | expand |
Just one tiny nitpick below: On Wed, 2022-08-24 at 14:14 +0200, Hans de Goede wrote: > Before this commit when we want userspace to use the acpi_video backlight > device we 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. > > Registering 2 backlight devices for a single display really is > undesirable, don't register the GPU's native backlight device when > another backlight device should be used. > > Changes in v2: > - Add nouveau_acpi_video_backlight_use_native() wrapper to avoid unresolved > symbol errors on non X86 > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/gpu/drm/nouveau/nouveau_acpi.c | 5 +++++ > drivers/gpu/drm/nouveau/nouveau_acpi.h | 2 ++ > drivers/gpu/drm/nouveau/nouveau_backlight.c | 6 ++++++ > 3 files changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c > index 6140db756d06..1592c9cd7750 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > @@ -386,3 +386,8 @@ nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector) > > return kmemdup(edid, EDID_LENGTH, GFP_KERNEL); > } > + > +bool nouveau_acpi_video_backlight_use_native(void) > +{ > + return acpi_video_backlight_use_native(); > +} > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.h b/drivers/gpu/drm/nouveau/nouveau_acpi.h > index 330f9b837066..3c666c30dfca 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.h > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.h > @@ -11,6 +11,7 @@ void nouveau_register_dsm_handler(void); > void nouveau_unregister_dsm_handler(void); > void nouveau_switcheroo_optimus_dsm(void); > void *nouveau_acpi_edid(struct drm_device *, struct drm_connector *); > +bool nouveau_acpi_video_backlight_use_native(void); > #else > static inline bool nouveau_is_optimus(void) { return false; }; > static inline bool nouveau_is_v1_dsm(void) { return false; }; > @@ -18,6 +19,7 @@ static inline void nouveau_register_dsm_handler(void) {} > static inline void nouveau_unregister_dsm_handler(void) {} > static inline void nouveau_switcheroo_optimus_dsm(void) {} > static inline void *nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector) { return NULL; } > +static inline bool nouveau_acpi_video_backlight_use_native(void) { return true; } > #endif > > #endif > diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c > index a2141d3d9b1d..d2b8f8c13db4 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c > +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c > @@ -38,6 +38,7 @@ > #include "nouveau_reg.h" > #include "nouveau_encoder.h" > #include "nouveau_connector.h" > +#include "nouveau_acpi.h" > > static struct ida bl_ida; > #define BL_NAME_SIZE 15 // 12 for name + 2 for digits + 1 for '\0' > @@ -405,6 +406,11 @@ nouveau_backlight_init(struct drm_connector *connector) > goto fail_alloc; > } > > + if (!nouveau_acpi_video_backlight_use_native()) { > + NV_INFO(drm, "Skipping nv_backlight registration\n"); > + goto fail_alloc; > + } We should probably make this say something like "No native backlight interface, using ACPI instead" instead. With that fixed Reviewed-by: Lyude Paul <lyude@redhat.com> > + > if (!nouveau_get_backlight_name(backlight_name, bl)) { > NV_ERROR(drm, "Failed to retrieve a unique name for the backlight interface\n"); > goto fail_alloc;
Hi Lyude, Thank you for the review. On 8/24/22 19:41, Lyude Paul wrote: > Just one tiny nitpick below: > > On Wed, 2022-08-24 at 14:14 +0200, Hans de Goede wrote: >> Before this commit when we want userspace to use the acpi_video backlight >> device we 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. >> >> Registering 2 backlight devices for a single display really is >> undesirable, don't register the GPU's native backlight device when >> another backlight device should be used. >> >> Changes in v2: >> - Add nouveau_acpi_video_backlight_use_native() wrapper to avoid unresolved >> symbol errors on non X86 >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/gpu/drm/nouveau/nouveau_acpi.c | 5 +++++ >> drivers/gpu/drm/nouveau/nouveau_acpi.h | 2 ++ >> drivers/gpu/drm/nouveau/nouveau_backlight.c | 6 ++++++ >> 3 files changed, 13 insertions(+) >> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c >> index 6140db756d06..1592c9cd7750 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c >> @@ -386,3 +386,8 @@ nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector) >> >> return kmemdup(edid, EDID_LENGTH, GFP_KERNEL); >> } >> + >> +bool nouveau_acpi_video_backlight_use_native(void) >> +{ >> + return acpi_video_backlight_use_native(); >> +} >> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.h b/drivers/gpu/drm/nouveau/nouveau_acpi.h >> index 330f9b837066..3c666c30dfca 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.h >> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.h >> @@ -11,6 +11,7 @@ void nouveau_register_dsm_handler(void); >> void nouveau_unregister_dsm_handler(void); >> void nouveau_switcheroo_optimus_dsm(void); >> void *nouveau_acpi_edid(struct drm_device *, struct drm_connector *); >> +bool nouveau_acpi_video_backlight_use_native(void); >> #else >> static inline bool nouveau_is_optimus(void) { return false; }; >> static inline bool nouveau_is_v1_dsm(void) { return false; }; >> @@ -18,6 +19,7 @@ static inline void nouveau_register_dsm_handler(void) {} >> static inline void nouveau_unregister_dsm_handler(void) {} >> static inline void nouveau_switcheroo_optimus_dsm(void) {} >> static inline void *nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector) { return NULL; } >> +static inline bool nouveau_acpi_video_backlight_use_native(void) { return true; } >> #endif >> >> #endif >> diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c >> index a2141d3d9b1d..d2b8f8c13db4 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c >> @@ -38,6 +38,7 @@ >> #include "nouveau_reg.h" >> #include "nouveau_encoder.h" >> #include "nouveau_connector.h" >> +#include "nouveau_acpi.h" >> >> static struct ida bl_ida; >> #define BL_NAME_SIZE 15 // 12 for name + 2 for digits + 1 for '\0' >> @@ -405,6 +406,11 @@ nouveau_backlight_init(struct drm_connector *connector) >> goto fail_alloc; >> } >> >> + if (!nouveau_acpi_video_backlight_use_native()) { >> + NV_INFO(drm, "Skipping nv_backlight registration\n"); >> + goto fail_alloc; >> + } > > We should probably make this say something like "No native backlight > interface, using ACPI instead" instead. With that fixed But that would not be correct. If we get to this point then before the change we would continue with registering the native backlight interface. In other words, the native backlight interface is known to be available at this point so saying "No native backlight interface" would not be correct. The reason the registration is being skipped is because the drivers/acpi/video_detect.c heuristics (or DMI quirk or cmdline override) say that another method to control the backlight is preferred and we want to stop registering the native backlight alltogether in that case so that there is only 1 /sys/class/backlight entry (on a 1 GPU 1 panel system). Also "using ACPI instead" is not correct, on older systems it might e.g. by a vendor specific control method such as the one from dell-laptop. And on newer systems it might e.g. be the new nvidia-wmi-ec-backlight driver instead. So you could say the log message is a bit vague on purpose because making it detailed would make it very long. The idea behind the log message is to have something to check for in dmesg if users start complaining about /sys/class/backlight/nouveau_bl disappearing. Normally users should not notice this, because indeed typically they will then also have an /sys/class/backlight/acpi_video0 which is already preferred over the native one by userspace, so nothing should change for them. But they could e.g. have scripts pointing directly at the native one, or some other special case. In those special cases having the log message to tell us why /sys/class/backlight/nv_backlight disappeared will help greatly with debugging. After writing all this I have not been able to come up with a better log message then the one from the original patch, so my proposal here is to just keep the log message as is. Is that ok? Regards, Hans > > Reviewed-by: Lyude Paul <lyude@redhat.com> > >> + >> if (!nouveau_get_backlight_name(backlight_name, bl)) { >> NV_ERROR(drm, "Failed to retrieve a unique name for the backlight interface\n"); >> goto fail_alloc; >
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c index 6140db756d06..1592c9cd7750 100644 --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c @@ -386,3 +386,8 @@ nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector) return kmemdup(edid, EDID_LENGTH, GFP_KERNEL); } + +bool nouveau_acpi_video_backlight_use_native(void) +{ + return acpi_video_backlight_use_native(); +} diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.h b/drivers/gpu/drm/nouveau/nouveau_acpi.h index 330f9b837066..3c666c30dfca 100644 --- a/drivers/gpu/drm/nouveau/nouveau_acpi.h +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.h @@ -11,6 +11,7 @@ void nouveau_register_dsm_handler(void); void nouveau_unregister_dsm_handler(void); void nouveau_switcheroo_optimus_dsm(void); void *nouveau_acpi_edid(struct drm_device *, struct drm_connector *); +bool nouveau_acpi_video_backlight_use_native(void); #else static inline bool nouveau_is_optimus(void) { return false; }; static inline bool nouveau_is_v1_dsm(void) { return false; }; @@ -18,6 +19,7 @@ static inline void nouveau_register_dsm_handler(void) {} static inline void nouveau_unregister_dsm_handler(void) {} static inline void nouveau_switcheroo_optimus_dsm(void) {} static inline void *nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector) { return NULL; } +static inline bool nouveau_acpi_video_backlight_use_native(void) { return true; } #endif #endif diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c index a2141d3d9b1d..d2b8f8c13db4 100644 --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c @@ -38,6 +38,7 @@ #include "nouveau_reg.h" #include "nouveau_encoder.h" #include "nouveau_connector.h" +#include "nouveau_acpi.h" static struct ida bl_ida; #define BL_NAME_SIZE 15 // 12 for name + 2 for digits + 1 for '\0' @@ -405,6 +406,11 @@ nouveau_backlight_init(struct drm_connector *connector) goto fail_alloc; } + if (!nouveau_acpi_video_backlight_use_native()) { + NV_INFO(drm, "Skipping nv_backlight registration\n"); + goto fail_alloc; + } + if (!nouveau_get_backlight_name(backlight_name, bl)) { NV_ERROR(drm, "Failed to retrieve a unique name for the backlight interface\n"); goto fail_alloc;
Before this commit when we want userspace to use the acpi_video backlight device we 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. Registering 2 backlight devices for a single display really is undesirable, don't register the GPU's native backlight device when another backlight device should be used. Changes in v2: - Add nouveau_acpi_video_backlight_use_native() wrapper to avoid unresolved symbol errors on non X86 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/nouveau/nouveau_acpi.c | 5 +++++ drivers/gpu/drm/nouveau/nouveau_acpi.h | 2 ++ drivers/gpu/drm/nouveau/nouveau_backlight.c | 6 ++++++ 3 files changed, 13 insertions(+)