* [PATCH v10 01/15] drm: add connector LUMINANCE property
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 17:07 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 02/15] drm/amd/display: Pass up errors reading actual brightness Mario Limonciello (AMD)
` (14 subsequent siblings)
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Panel brightness is a property of a display, and thus of a DRM
connector. Add an atomic per-connector LUMINANCE range property so
compositors can manage relative panel luminance together with the rest
of the display state.
Keep the core backend-agnostic. Drivers create the property with their
hardware range and implement connector helper callbacks to set luminance
or disable panel emission. The atomic helpers sequence those callbacks
around pipeline enable and disable, including luminance-only commits.
The stored property value is independent of display power. A commit made
while the connector is inactive is retained and applied on the next
enable; disabling the pipeline does not overwrite it.
LUMINANCE values are relative, driver-defined levels rather than
absolute luminance in cd/m^2. The interface supports emissive panels as
well as panels with a separate backlight.
Keep the interface behind a default-off DRM_LUMINANCE option. Migrated
drivers retain their legacy backlight class ABI unless the new interface
is explicitly selected, allowing the DRM UAPI and legacy compatibility
policy to remain separate.
Acked-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v10:
* Add tag
---
drivers/gpu/drm/Kconfig | 14 ++++++
drivers/gpu/drm/drm_atomic_helper.c | 73 +++++++++++++++++++++++++++++-
drivers/gpu/drm/drm_atomic_state_helper.c | 7 +++
drivers/gpu/drm/drm_atomic_uapi.c | 5 ++
drivers/gpu/drm/drm_connector.c | 57 +++++++++++++++++++++++
drivers/gpu/drm/tests/drm_connector_test.c | 28 ++++++++++++
include/drm/drm_atomic_helper.h | 3 ++
include/drm/drm_connector.h | 16 +++++++
include/drm/drm_modeset_helper_vtables.h | 25 ++++++++++
9 files changed, 227 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 61111d50b1c65..1bbe2c2d7c76f 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -33,6 +33,20 @@ endmenu
if DRM
+config DRM_LUMINANCE
+ bool "DRM connector luminance support"
+ help
+ Expose relative luminance as an atomic DRM connector property. Values
+ are driver-defined levels rather than absolute luminance in cd/m^2.
+ Drivers provide connector helper callbacks to program their hardware;
+ the DRM core does not depend on a particular backend.
+
+ Drivers migrated to this interface will use it instead of registering
+ a legacy backlight class device. Leave this disabled to preserve the
+ de-facto userspace ABI in /sys/class/backlight.
+
+ If in doubt, say N.
+
config DRM_MIPI_DBI
tristate
depends on DRM
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9d006f98413a5..1e9b7b8acccfe 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1375,6 +1375,8 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_encoder_bridge_post_disable);
static void
disable_outputs(struct drm_device *dev, struct drm_atomic_commit *state)
{
+ drm_atomic_helper_commit_luminance_disables(state);
+
drm_atomic_helper_commit_encoder_bridge_disable(dev, state);
drm_atomic_helper_commit_encoder_bridge_post_disable(dev, state);
@@ -1382,6 +1384,45 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_commit *state)
drm_atomic_helper_commit_crtc_disable(dev, state);
}
+/**
+ * drm_atomic_helper_commit_luminance_disables - disable panel emission
+ * @state: atomic state object being committed
+ *
+ * Invokes each connector driver's luminance disable callback before a
+ * display pipeline is disabled. The connector property's stored value is left
+ * untouched so it can be restored when the pipeline is enabled again.
+ */
+void drm_atomic_helper_commit_luminance_disables(struct drm_atomic_commit *state)
+{
+ struct drm_connector_state *old_conn_state, *new_conn_state;
+ struct drm_connector *connector;
+ int i;
+
+ for_each_oldnew_connector_in_state(state, connector, old_conn_state,
+ new_conn_state, i) {
+ const struct drm_connector_helper_funcs *funcs;
+ struct drm_crtc_state *old_crtc_state, *new_crtc_state = NULL;
+
+ if (!connector->luminance_property || !old_conn_state->crtc)
+ continue;
+
+ old_crtc_state =
+ drm_atomic_get_old_crtc_state(state, old_conn_state->crtc);
+ if (new_conn_state->crtc)
+ new_crtc_state =
+ drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
+
+ if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
+ !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
+ continue;
+
+ funcs = connector->helper_private;
+ if (funcs && funcs->disable_luminance)
+ funcs->disable_luminance(old_conn_state);
+ }
+}
+EXPORT_SYMBOL(drm_atomic_helper_commit_luminance_disables);
+
/**
* drm_atomic_helper_update_legacy_modeset_state - update legacy modeset state
* @dev: DRM device
@@ -1771,10 +1812,41 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
drm_atomic_helper_commit_encoder_bridge_enable(dev, state);
+ drm_atomic_helper_commit_luminance_enables(state);
+
drm_atomic_helper_commit_writebacks(dev, state);
}
EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
+/**
+ * drm_atomic_helper_commit_luminance_enables - apply luminance
+ * @state: atomic state object being committed
+ *
+ * Invokes each connector driver's luminance callback after the display
+ * pipeline has been enabled. The callback is also invoked for brightness-only
+ * commits while the connector remains active. Changes committed while the
+ * connector is inactive remain stored in atomic state until a later enable.
+ */
+void drm_atomic_helper_commit_luminance_enables(struct drm_atomic_commit *state)
+{
+ struct drm_connector_state *new_conn_state;
+ struct drm_connector *connector;
+ int i;
+
+ for_each_new_connector_in_state(state, connector, new_conn_state, i) {
+ const struct drm_connector_helper_funcs *funcs;
+
+ if (!connector->luminance_property || !new_conn_state->crtc ||
+ !new_conn_state->crtc->state->active)
+ continue;
+
+ funcs = connector->helper_private;
+ if (funcs && funcs->set_luminance)
+ funcs->set_luminance(new_conn_state);
+ }
+}
+EXPORT_SYMBOL(drm_atomic_helper_commit_luminance_enables);
+
/*
* For atomic updates which touch just a single CRTC, calculate the time of the
* next vblank, and inform all the fences of the deadline.
@@ -4187,4 +4259,3 @@ drm_atomic_helper_bridge_get_hdmi_output_bus_fmts(struct drm_bridge *bridge,
return out_fmts;
}
EXPORT_SYMBOL(drm_atomic_helper_bridge_get_hdmi_output_bus_fmts);
-
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 406157a3660fd..5c31bf2bdbe76 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -408,7 +408,14 @@ void
__drm_atomic_helper_connector_state_init(struct drm_connector_state *conn_state,
struct drm_connector *connector)
{
+ u64 value;
+
conn_state->connector = connector;
+ if (connector->luminance_property &&
+ !drm_object_property_get_default_value(&connector->base,
+ connector->luminance_property,
+ &value))
+ conn_state->luminance = value;
}
EXPORT_SYMBOL(__drm_atomic_helper_connector_state_init);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 1eebcf6f5e05b..dffc51cd8d13c 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -30,6 +30,7 @@
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_atomic_uapi.h>
+#include <drm/drm_connector.h>
#include <drm/drm_framebuffer.h>
#include <drm/drm_print.h>
#include <drm/drm_drv.h>
@@ -969,6 +970,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
state->hdmi.broadcast_rgb = val;
} else if (property == connector->color_format_property) {
state->color_format = val;
+ } else if (property == connector->luminance_property) {
+ state->luminance = val;
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
state, property, val);
@@ -1056,6 +1059,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
*val = state->hdmi.broadcast_rgb;
} else if (property == connector->color_format_property) {
*val = state->color_format;
+ } else if (property == connector->luminance_property) {
+ *val = state->luminance;
} else if (connector->funcs->atomic_get_property) {
return connector->funcs->atomic_get_property(connector,
state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 893716a9e4895..ab75e8bd9991a 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -810,6 +810,47 @@ void drm_connector_attach_edid_property(struct drm_connector *connector)
}
EXPORT_SYMBOL(drm_connector_attach_edid_property);
+/**
+ * drm_connector_attach_luminance_property - attach luminance property
+ * @connector: connector to attach the property to
+ * @min: minimum supported relative brightness
+ * @max: maximum supported relative brightness
+ * @value: initial relative brightness
+ *
+ * Creates the atomic ``LUMINANCE`` range property for @connector.
+ * Drivers should call this during connector initialization. The property's
+ * units are driver-defined; they do not represent absolute luminance.
+ *
+ * Returns:
+ * Zero on success, a negative errno otherwise.
+ */
+int drm_connector_attach_luminance_property(struct drm_connector *connector,
+ unsigned int min,
+ unsigned int max,
+ unsigned int value)
+{
+ struct drm_property *property;
+
+ if (drm_WARN_ON(connector->dev, min > max || value < min || value > max))
+ return -EINVAL;
+
+ if (connector->luminance_property)
+ return 0;
+
+ property = drm_property_create_range(connector->dev, DRM_MODE_PROP_ATOMIC,
+ "LUMINANCE", min, max);
+ if (!property)
+ return -ENOMEM;
+
+ connector->luminance_property = property;
+ drm_object_attach_property(&connector->base, property, value);
+ if (connector->state)
+ connector->state->luminance = value;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_luminance_property);
+
/**
* drm_connector_attach_encoder - attach a connector to an encoder
* @connector: connector to attach
@@ -1681,6 +1722,22 @@ EXPORT_SYMBOL(drm_hdmi_connector_get_output_format_name);
* Summarizing: Only set "DPMS" when the connector is known to be enabled,
* assume that a successful SETCONFIG call also sets "DPMS" to on, and
* never read back the value of "DPMS" because it can be incorrect.
+ * LUMINANCE:
+ * Atomic range property for controlling a panel's relative luminance.
+ * The unit and range are driver-defined and do not represent an absolute
+ * luminance in cd/m^2. This name therefore also applies to emissive panels
+ * that do not contain a separate backlight.
+ *
+ * The property is independent of the connector's display-power state.
+ * Changing it while the connector is inactive stores the new value without
+ * turning the display on. The atomic helpers apply the stored value after
+ * the display pipeline is enabled and invoke a separate driver callback
+ * before disabling the pipeline; neither action changes the property value.
+ *
+ * Drivers create the property with
+ * drm_connector_attach_luminance_property() and implement the
+ * corresponding &drm_connector_helper_funcs callbacks. Connectors that do
+ * not support luminance control do not expose the property.
* panel_type:
* Immutable enum property to indicate the type of connected panel.
* Possible values are "unknown" (default), "OLED", and "LCD".
diff --git a/drivers/gpu/drm/tests/drm_connector_test.c b/drivers/gpu/drm/tests/drm_connector_test.c
index 3368aedd4e669..e43692d38ebb8 100644
--- a/drivers/gpu/drm/tests/drm_connector_test.c
+++ b/drivers/gpu/drm/tests/drm_connector_test.c
@@ -137,6 +137,33 @@ static void drm_test_drmm_connector_init_null_ddc(struct kunit *test)
KUNIT_EXPECT_EQ(test, ret, 0);
}
+static void drm_test_connector_attach_luminance_property(struct kunit *test)
+{
+ struct drm_connector_init_priv *priv = test->priv;
+ struct drm_connector *connector = &priv->connector;
+ struct drm_property *property;
+ u64 value;
+ int ret;
+
+ ret = drmm_connector_init(&priv->drm, connector, &dummy_funcs,
+ DRM_MODE_CONNECTOR_eDP, NULL);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+ KUNIT_ASSERT_NULL(test, connector->state);
+
+ ret = drm_connector_attach_luminance_property(connector, 10, 100, 50);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+ property = connector->luminance_property;
+ KUNIT_ASSERT_NOT_NULL(test, property);
+
+ ret = drm_object_property_get_default_value(&connector->base, property, &value);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+ KUNIT_EXPECT_EQ(test, value, (u64)50);
+
+ drm_atomic_helper_connector_reset(connector);
+ KUNIT_ASSERT_NOT_NULL(test, connector->state);
+ KUNIT_EXPECT_EQ(test, connector->state->luminance, 50u);
+}
+
/*
* Test that the registration of a connector succeeds for all possible
* connector types.
@@ -190,6 +217,7 @@ KUNIT_ARRAY_PARAM(drm_connector_init_type_valid,
static struct kunit_case drmm_connector_init_tests[] = {
KUNIT_CASE(drm_test_drmm_connector_init),
KUNIT_CASE(drm_test_drmm_connector_init_null_ddc),
+ KUNIT_CASE(drm_test_connector_attach_luminance_property),
KUNIT_CASE_PARAM(drm_test_drmm_connector_init_type_valid,
drm_connector_init_type_valid_gen_params),
{ }
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 4cfeec70d648a..3084dd5cd6a1f 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -116,6 +116,9 @@ void drm_atomic_helper_commit_encoder_bridge_enable(struct drm_device *dev,
void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
struct drm_atomic_commit *old_state);
+void drm_atomic_helper_commit_luminance_disables(struct drm_atomic_commit *state);
+void drm_atomic_helper_commit_luminance_enables(struct drm_atomic_commit *state);
+
int drm_atomic_helper_prepare_planes(struct drm_device *dev,
struct drm_atomic_commit *state);
void drm_atomic_helper_unprepare_planes(struct drm_device *dev,
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 2ee7c59329aae..7f7d115b65f9f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1369,6 +1369,11 @@ struct drm_connector_state {
* @drm_atomic_helper_connector_hdmi_check().
*/
struct drm_connector_hdmi_state hdmi;
+
+ /**
+ * @luminance: Relative panel luminance requested by userspace.
+ */
+ unsigned int luminance;
};
struct drm_connector_hdmi_audio_funcs {
@@ -2714,6 +2719,13 @@ struct drm_connector {
* @cec: CEC-related data.
*/
struct drm_connector_cec cec;
+
+ /**
+ * @luminance_property: Optional atomic range property for relative panel
+ * luminance. Drivers create it with
+ * drm_connector_attach_luminance_property().
+ */
+ struct drm_property *luminance_property;
};
#define obj_to_connector(x) container_of(x, struct drm_connector, base)
@@ -2980,6 +2992,10 @@ const char *drm_get_colorspace_name(enum drm_colorspace colorspace);
int drm_connector_attach_color_format_property(struct drm_connector *connector,
unsigned long supported_color_formats);
+int drm_connector_attach_luminance_property(struct drm_connector *connector,
+ unsigned int min,
+ unsigned int max,
+ unsigned int value);
/**
* drm_for_each_connector_iter - connector_list iterator macro
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 60b30cc269404..cecacc1755a36 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1147,6 +1147,31 @@ struct drm_connector_helper_funcs {
void (*atomic_commit)(struct drm_connector *connector,
struct drm_atomic_commit *state);
+ /**
+ * @set_luminance:
+ *
+ * Apply &drm_connector_state.luminance to an active panel. The
+ * atomic helpers call this after enabling the display pipeline and for
+ * brightness-only commits while the connector remains active.
+ *
+ * This callback is optional and is only used by connectors exposing the
+ * ``LUMINANCE`` property.
+ */
+ void (*set_luminance)(const struct drm_connector_state *new_state);
+
+ /**
+ * @disable_luminance:
+ *
+ * Turn panel light emission off before disabling the display pipeline.
+ * This callback does not change the ``LUMINANCE`` property; the
+ * stored value is restored through @set_luminance when the panel
+ * is enabled again.
+ *
+ * This callback is optional and is only used by connectors exposing the
+ * ``LUMINANCE`` property.
+ */
+ void (*disable_luminance)(const struct drm_connector_state *old_state);
+
/**
* @prepare_writeback_job:
*
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 01/15] drm: add connector LUMINANCE property
2026-11-02 16:49 ` [PATCH v10 01/15] drm: add connector LUMINANCE property Mario Limonciello (AMD)
@ 2026-11-02 17:07 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 17:07 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/atomic-helper: panel emission disabled on PSR entry in drm_atomic_helper_commit_luminance_disables()
- [Medium] drm/atomic-helper: redundant set_luminance() calls in drm_atomic_helper_commit_luminance_enables()
--
commit 723bca80ec2bc6672a30925c2cb41a862300dc7b
Author: Mario Limonciello <mario.limonciello@amd.com>
drm: add connector LUMINANCE property
This commit adds an atomic per-connector LUMINANCE range property along with
connector helper callbacks to set luminance or disable panel emission during
atomic commits.
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9d006f98413a5..1e9b7b8acccfe 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1375,6 +1375,8 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_encoder_bridge_post_disable);
> static void
> disable_outputs(struct drm_device *dev, struct drm_atomic_commit *state)
> {
> + drm_atomic_helper_commit_luminance_disables(state);
> +
> drm_atomic_helper_commit_encoder_bridge_disable(dev, state);
>
> drm_atomic_helper_commit_encoder_bridge_post_disable(dev, state);
> @@ -1382,6 +1384,45 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_commit *state)
> drm_atomic_helper_commit_crtc_disable(dev, state);
> }
>
> +/**
> + * drm_atomic_helper_commit_luminance_disables - disable panel emission
> + * @state: atomic state object being committed
> + *
> + * Invokes each connector driver's luminance disable callback before a
> + * display pipeline is disabled. The connector property's stored value is left
> + * untouched so it can be restored when the pipeline is enabled again.
> + */
> +void drm_atomic_helper_commit_luminance_disables(struct drm_atomic_commit *state)
> +{
> + struct drm_connector_state *old_conn_state, *new_conn_state;
> + struct drm_connector *connector;
> + int i;
> +
> + for_each_oldnew_connector_in_state(state, connector, old_conn_state,
> + new_conn_state, i) {
> + const struct drm_connector_helper_funcs *funcs;
> + struct drm_crtc_state *old_crtc_state, *new_crtc_state = NULL;
> +
> + if (!connector->luminance_property || !old_conn_state->crtc)
> + continue;
> +
> + old_crtc_state =
> + drm_atomic_get_old_crtc_state(state, old_conn_state->crtc);
> + if (new_conn_state->crtc)
> + new_crtc_state =
> + drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
> +
> + if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
> + !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
> + continue;
> +
> + funcs = connector->helper_private;
> + if (funcs && funcs->disable_luminance)
> + funcs->disable_luminance(old_conn_state);
> + }
> +}
[Severity: High]
Will drm_atomic_helper_commit_luminance_disables() turn off panel emission
when a display enters Panel Self-Refresh (PSR)?
When a CRTC enters self-refresh via drm_self_refresh_helper_entry_work() in
drivers/gpu/drm/drm_self_refresh_helper.c, affected connectors are added to
the commit with new_crtc_state->active set to false and
new_crtc_state->self_refresh_active set to true. That transition sets
new_crtc_state->active_changed, so drm_atomic_crtc_needs_modeset() returns
true, and crtc_needs_disable() also returns true because old_state->active
and new_state->self_refresh_active are set:
drivers/gpu/drm/drm_atomic_helper.c:crtc_needs_disable() {
...
return old_state->active ||
(old_state->self_refresh_active && !new_state->active) ||
new_state->self_refresh_active;
}
For comparison, panel_bridge_atomic_disable() in drivers/gpu/drm/drm_panel.c
explicitly skips disabling the panel when entering self-refresh:
drivers/gpu/drm/drm_panel.c:panel_bridge_atomic_disable() {
...
new_crtc_state = drm_atomic_get_new_crtc_state(atomic_state, crtc);
if (new_crtc_state && new_crtc_state->self_refresh_active)
return;
drm_panel_disable(panel);
}
Without a similar check for new_crtc_state->self_refresh_active here (and
because drm_atomic_helper_commit_luminance_enables() skips set_luminance()
while new_conn_state->crtc->state->active is false), wouldn't
funcs->disable_luminance(old_conn_state) be called on PSR entry and leave the
panel dark while displaying the static frame?
> +EXPORT_SYMBOL(drm_atomic_helper_commit_luminance_disables);
> +
> /**
> * drm_atomic_helper_update_legacy_modeset_state - update legacy modeset state
> * @dev: DRM device
> @@ -1771,10 +1812,41 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>
> drm_atomic_helper_commit_encoder_bridge_enable(dev, state);
>
> + drm_atomic_helper_commit_luminance_enables(state);
> +
> drm_atomic_helper_commit_writebacks(dev, state);
> }
> EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>
> +/**
> + * drm_atomic_helper_commit_luminance_enables - apply luminance
> + * @state: atomic state object being committed
> + *
> + * Invokes each connector driver's luminance callback after the display
> + * pipeline has been enabled. The callback is also invoked for brightness-only
> + * commits while the connector remains active. Changes committed while the
> + * connector is inactive remain stored in atomic state until a later enable.
> + */
> +void drm_atomic_helper_commit_luminance_enables(struct drm_atomic_commit *state)
> +{
> + struct drm_connector_state *new_conn_state;
> + struct drm_connector *connector;
> + int i;
> +
> + for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> + const struct drm_connector_helper_funcs *funcs;
> +
> + if (!connector->luminance_property || !new_conn_state->crtc ||
> + !new_conn_state->crtc->state->active)
> + continue;
> +
> + funcs = connector->helper_private;
> + if (funcs && funcs->set_luminance)
> + funcs->set_luminance(new_conn_state);
> + }
> +}
[Severity: Medium]
Should drm_atomic_helper_commit_luminance_enables() check whether the CRTC
needs a modeset or old_conn_state->luminance != new_conn_state->luminance
before calling funcs->set_luminance(new_conn_state)?
drm_atomic_helper_commit_modeset_enables() runs on every atomic commit via
drm_atomic_helper_commit_tail() and drm_atomic_helper_commit_tail_rpm(). Any
non-modeset commit on an active CRTC that includes the connector in the
atomic state (such as updates to other connector properties like
hdr_output_metadata or max_bpc, compositors submitting unchanged connector
properties during page flips, or driver atomic_check hooks calling
drm_atomic_add_affected_connectors()) will invoke funcs->set_luminance() and
reprogram the hardware even when the luminance level and power state are
unchanged.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=1
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 02/15] drm/amd/display: Pass up errors reading actual brightness
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
2026-11-02 16:49 ` [PATCH v10 01/15] drm: add connector LUMINANCE property Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 16:49 ` [PATCH v10 03/15] drm/amd: add luminance property support Mario Limonciello (AMD)
` (13 subsequent siblings)
15 siblings, 0 replies; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
[Why]
If the DC API fails to return actual brightness when backlight control
API requests it, then the wrong value may be returned.
[How]
Change return type of amdgpu_dm_backlight_get_level() to an integer
and pass an error code up to the caller.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c | 13 +++++++++----
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h | 2 +-
.../amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c | 4 ++--
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
index cb74b00723fda..0f664c658a127 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
@@ -438,7 +438,7 @@ int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
EXPORT_IF_KUNIT(amdgpu_dm_backlight_update_status);
STATIC_IFN_KUNIT
-u32 amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx)
+int amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx)
{
int ret;
struct amdgpu_dm_backlight_caps caps;
@@ -451,14 +451,14 @@ u32 amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx)
u32 avg, peak;
if (!dc_link_get_backlight_level_nits(link, &avg, &peak))
- return dm->brightness[bl_idx];
+ return -EINVAL;
return convert_brightness_to_user(&caps, avg);
}
ret = dc_link_get_backlight_level(link);
if (ret == DC_ERROR_UNEXPECTED)
- return dm->brightness[bl_idx];
+ return -EINVAL;
return convert_brightness_to_user(&caps, ret);
}
@@ -469,8 +469,13 @@ int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
{
struct amdgpu_display_manager *dm = bl_get_data(bd);
int i = amdgpu_dm_backlight_get_device_index(dm, bd);
+ int ret;
+
+ ret = amdgpu_dm_backlight_get_level(dm, i);
+ if (ret < 0)
+ return dm->brightness[i];
- return amdgpu_dm_backlight_get_level(dm, i);
+ return ret;
}
EXPORT_IF_KUNIT(amdgpu_dm_backlight_get_brightness);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
index 9ad888d3ae7b2..891749322191c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
@@ -57,7 +57,7 @@ void amdgpu_dm_validate_backlight_caps(struct amdgpu_display_manager *dm, int bl
struct dc_stream_state *dm_find_stream_with_link(struct amdgpu_display_manager *dm,
struct dc_link *link);
int amdgpu_dm_backlight_update_status(struct backlight_device *bd);
-u32 amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx);
+int amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm, int bl_idx);
int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd);
ssize_t panel_power_savings_show(struct device *device,
struct device_attribute *attr,
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
index 5221050dec2f1..b21850788f36f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
@@ -382,7 +382,7 @@ static void dm_test_backlight_get_level_pwm_error(struct kunit *test)
dm->brightness[0] = 4321;
dm->backlight_link[0] = link;
- KUNIT_EXPECT_EQ(test, amdgpu_dm_backlight_get_level(dm, 0), 4321U);
+ KUNIT_EXPECT_EQ(test, amdgpu_dm_backlight_get_level(dm, 0), -EINVAL);
}
/**
@@ -424,7 +424,7 @@ static void dm_test_backlight_get_level_aux_error(struct kunit *test)
caps->caps_valid = true;
caps->aux_support = true;
- KUNIT_EXPECT_EQ(test, amdgpu_dm_backlight_get_level(dm, 0), 6789U);
+ KUNIT_EXPECT_EQ(test, amdgpu_dm_backlight_get_level(dm, 0), -EINVAL);
}
/**
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v10 03/15] drm/amd: add luminance property support
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
2026-11-02 16:49 ` [PATCH v10 01/15] drm: add connector LUMINANCE property Mario Limonciello (AMD)
2026-11-02 16:49 ` [PATCH v10 02/15] drm/amd/display: Pass up errors reading actual brightness Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 17:32 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 04/15] drm/amdgpu: Check bios_scratch_reg_offset in backlight level helper Mario Limonciello (AMD)
` (12 subsequent siblings)
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Expose the DRM LUMINANCE connector property for internal panels when
CONFIG_DRM_LUMINANCE is enabled. Keep registering the existing backlight
class device otherwise, preserving its userspace ABI.
Use the existing brightness scaling and programming paths from connector
helper callbacks in both Display Core and legacy display. Apply atomic
luminance after the display pipeline is enabled so changes made while a
connector is inactive take effect on the next enable.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/amd/amdgpu/Kconfig | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 20 +++++++++++
drivers/gpu/drm/amd/amdgpu/atombios_encoders.c | 17 +++++++--
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 22 +++++++-----
.../amd/display/amdgpu_dm/amdgpu_dm_backlight.c | 42 ++++++++++++++++++++--
.../amd/display/amdgpu_dm/amdgpu_dm_backlight.h | 2 ++
.../amd/display/amdgpu_dm/amdgpu_dm_connector.c | 7 ++--
7 files changed, 96 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
index 50305630f5335..7a6a049aa6e90 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -20,7 +20,7 @@ config DRM_AMDGPU
select I2C
select I2C_ALGOBIT
select CRC16
- select BACKLIGHT_CLASS_DEVICE
+ select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
select INTERVAL_TREE
select DRM_BUDDY
select DRM_SUBALLOC_HELPER
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 92c98e999efe9..209a7a6c246db 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -798,11 +798,31 @@ static int amdgpu_connector_set_lcd_property(struct drm_connector *connector,
return 0;
}
+static void amdgpu_connector_set_luminance(const struct drm_connector_state *conn_state)
+{
+ struct drm_encoder *encoder =
+ amdgpu_connector_best_single_encoder(conn_state->connector);
+
+ if (encoder)
+ amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder),
+ conn_state->luminance);
+}
+
+static void amdgpu_connector_disable_luminance(const struct drm_connector_state *conn_state)
+{
+ struct drm_encoder *encoder =
+ amdgpu_connector_best_single_encoder(conn_state->connector);
+
+ if (encoder)
+ amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder), 0);
+}
static const struct drm_connector_helper_funcs amdgpu_connector_lvds_helper_funcs = {
.get_modes = amdgpu_connector_lvds_get_modes,
.mode_valid = amdgpu_connector_lvds_mode_valid,
.best_encoder = amdgpu_connector_best_single_encoder,
+ .set_luminance = amdgpu_connector_set_luminance,
+ .disable_luminance = amdgpu_connector_disable_luminance,
};
static const struct drm_connector_funcs amdgpu_connector_lvds_funcs = {
diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
index a1d05df0ac99b..dbff532ac1bcd 100644
--- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
+++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
@@ -191,6 +191,21 @@ void amdgpu_atombios_encoder_init_backlight(struct amdgpu_encoder *amdgpu_encode
goto register_acpi_backlight;
}
+ dig = amdgpu_encoder->enc_priv;
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ unsigned int brightness;
+
+ brightness = amdgpu_atombios_encoder_get_backlight_level(amdgpu_encoder);
+ if (drm_connector_attach_luminance_property(drm_connector, 0,
+ AMDGPU_MAX_BL_LEVEL,
+ brightness))
+ return;
+
+ dig->backlight_level = brightness;
+ dig->bl_dev = NULL;
+ return;
+ }
+
pdata = kmalloc_obj(struct amdgpu_backlight_privdata);
if (!pdata) {
DRM_ERROR("Memory allocation failed\n");
@@ -211,7 +226,6 @@ void amdgpu_atombios_encoder_init_backlight(struct amdgpu_encoder *amdgpu_encode
pdata->encoder = amdgpu_encoder;
- dig = amdgpu_encoder->enc_priv;
dig->bl_dev = bd;
bd->props.brightness = amdgpu_atombios_encoder_get_backlight_brightness(bd);
@@ -2123,4 +2137,3 @@ amdgpu_atombios_encoder_get_dig_info(struct amdgpu_encoder *amdgpu_encoder)
return dig;
}
-
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 0ea45ab12119c..53ff9a55c3a8e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5191,9 +5191,12 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
}
/* Skip any modesets/resets */
- if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state))
+ if (!acrtc || !new_crtc_state ||
+ drm_atomic_crtc_needs_modeset(new_crtc_state))
continue;
+ drm_connector_update_privacy_screen(new_con_state);
+
dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
@@ -5273,8 +5276,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
&stream_update);
mutex_unlock(&dm->dc_lock);
kfree(dummy_updates);
-
- drm_connector_update_privacy_screen(new_con_state);
}
/**
@@ -5397,11 +5398,16 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
/* Update audio instances for each connector. */
amdgpu_dm_commit_audio(dev, state);
- /* restore the backlight level */
- for (i = 0; i < dm->num_of_edps; i++) {
- if (dm->backlight_dev[i] &&
- (dm->actual_brightness[i] != dm->brightness[i]))
- amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ /* Apply brightness after the display pipeline is enabled. */
+ drm_atomic_helper_commit_luminance_enables(state);
+ } else {
+ /* Restore the legacy backlight level. */
+ for (i = 0; i < dm->num_of_edps; i++) {
+ if (dm->backlight_dev[i] &&
+ dm->actual_brightness[i] != dm->brightness[i])
+ amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
+ }
}
/*
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
index 0f664c658a127..0857354e7327e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
@@ -41,6 +41,7 @@
#include <linux/backlight.h>
#include <linux/minmax.h>
#include <linux/power_supply.h>
+#include <drm/drm_connector.h>
#include <drm/drm_edid.h>
#include <drm/drm_panel_quirks.h>
@@ -296,13 +297,17 @@ struct dc_stream_state *dm_find_stream_with_link(
struct amdgpu_display_manager *dm,
struct dc_link *link)
{
- struct dc_state *cur_dc_state = dm->dc->current_state;
+ struct dc_state *cur_dc_state;
struct dc_stream_state *stream = NULL;
int i;
+ if (!dm || !dm->dc || !dm->dc->current_state || !link)
+ return NULL;
+
+ cur_dc_state = dm->dc->current_state;
for (i = 0; i < cur_dc_state->stream_count; i++) {
stream = cur_dc_state->streams[i];
- if (stream->link == link)
+ if (stream && stream->link == link)
return stream;
}
@@ -356,7 +361,7 @@ void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
dm->brightness[bl_idx] = user_brightness;
/* update scratch register */
- if (bl_idx == 0)
+ if (bl_idx == 0 && dm->adev && dm->adev->rmmio && dm->adev->bios_scratch_reg_offset)
amdgpu_atombios_scratch_regs_set_backlight_level(dm->adev, dm->brightness[bl_idx]);
link = (struct dc_link *)dm->backlight_link[bl_idx];
@@ -657,6 +662,10 @@ void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
{
struct dc_link *link = aconnector->dc_link;
int bl_idx = dm->num_of_edps;
+ struct backlight_properties props = { 0 };
+ struct amdgpu_dm_backlight_caps *caps;
+ struct drm_connector *connector = &aconnector->base;
+ int ret;
if (!(link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) ||
link->type == dc_connection_none)
@@ -675,6 +684,19 @@ void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
amdgpu_dm_update_connector_ext_caps(aconnector);
+ caps = &dm->backlight_caps[bl_idx];
+ amdgpu_dm_backlight_fill_props(caps, power_supply_is_system_supplied() > 0,
+ !(amdgpu_dc_debug_mask &
+ DC_DISABLE_CUSTOM_BRIGHTNESS_CURVE),
+ &props);
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ unsigned int max = props.max_brightness;
+ unsigned int value = props.brightness;
+
+ ret = drm_connector_attach_luminance_property(connector, 0, max, value);
+ drm_WARN_ON(adev_to_drm(dm->adev), ret);
+ }
+
/* Offer ABM property when user didn't turn off by module parameter.
* OLED panels are included to support CACP (Content Adaptive
* Contrast and Power) feature via set_abm_level.
@@ -686,6 +708,20 @@ void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
}
EXPORT_IF_KUNIT(amdgpu_dm_setup_backlight_device);
+void amdgpu_dm_set_luminance(const struct drm_connector_state *state)
+{
+ struct amdgpu_dm_connector *aconnector =
+ to_amdgpu_dm_connector(state->connector);
+ struct amdgpu_display_manager *dm =
+ &drm_to_adev(state->connector->dev)->dm;
+
+ if (aconnector->bl_idx < 0)
+ return;
+
+ amdgpu_dm_backlight_set_level(dm, aconnector->bl_idx,
+ state->luminance);
+}
+
/**
* DOC: panel power savings
*
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
index 891749322191c..859224cb37988 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.h
@@ -33,6 +33,7 @@ struct dc_stream_state;
struct device;
struct device_attribute;
struct drm_connector;
+struct drm_connector_state;
struct attribute_group;
#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12
@@ -47,6 +48,7 @@ void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
void amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector);
void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
struct amdgpu_dm_connector *aconnector);
+void amdgpu_dm_set_luminance(const struct drm_connector_state *state);
void amdgpu_dm_update_connector_ext_caps(struct amdgpu_dm_connector *aconnector);
bool amdgpu_dm_should_create_sysfs(struct amdgpu_dm_connector *aconnector);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
index b498fc5fbb140..f2c2a2e730d3e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
@@ -1944,7 +1944,8 @@ STATIC_IFN_KUNIT void amdgpu_dm_connector_destroy(struct drm_connector *connecto
}
if (aconnector->bl_idx != -1) {
- backlight_device_unregister(dm->backlight_dev[aconnector->bl_idx]);
+ if (dm->backlight_dev[aconnector->bl_idx])
+ backlight_device_unregister(dm->backlight_dev[aconnector->bl_idx]);
dm->backlight_dev[aconnector->bl_idx] = NULL;
}
@@ -2038,7 +2039,8 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector)
return r;
}
- amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
+ if (!IS_ENABLED(CONFIG_DRM_LUMINANCE))
+ amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) ||
(connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
@@ -2701,6 +2703,7 @@ amdgpu_dm_connector_helper_funcs = {
.get_modes = get_modes,
.mode_valid = amdgpu_dm_connector_mode_valid,
.atomic_check = amdgpu_dm_connector_atomic_check,
+ .set_luminance = amdgpu_dm_set_luminance,
};
int amdgpu_dm_convert_dc_color_depth_into_bpc(enum dc_color_depth display_color_depth)
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 03/15] drm/amd: add luminance property support
2026-11-02 16:49 ` [PATCH v10 03/15] drm/amd: add luminance property support Mario Limonciello (AMD)
@ 2026-11-02 17:32 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 17:32 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/amdgpu: broken atomic LUMINANCE support on non-atomic legacy display
- [High] drm/amd/display: skipped backlight restore and setup when DRM_LUMINANCE=y
- [Medium] drm/amd/display: unconditional backlight programming blocks PSR/Replay
- [Low] drm/amd: Kconfig and link breakage when DRM_LUMINANCE=y without backlight
--
commit de26f6665aed29ccc412fe4be3f0737b06b98759
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/amd: add luminance property support
This commit adds support for the DRM LUMINANCE connector property on internal
panels when CONFIG_DRM_LUMINANCE is enabled, bypassing sysfs backlight device
registration and wiring connector luminance callbacks for Display Core and
legacy display paths.
> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
> index 50305630f5335..7a6a049aa6e90 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
> @@ -20,7 +20,7 @@ config DRM_AMDGPU
> select I2C
> select I2C_ALGOBIT
> select CRC16
> - select BACKLIGHT_CLASS_DEVICE
> + select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
> select INTERVAL_TREE
> select DRM_BUDDY
> select DRM_SUBALLOC_HELPER
[Severity: Low]
Does dropping select BACKLIGHT_CLASS_DEVICE when CONFIG_DRM_LUMINANCE is
enabled cause Kconfig dependency warnings and link errors when
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled (or built as a module with
CONFIG_DRM_AMDGPU=y)?
In drivers/gpu/drm/amd/amdgpu/Kconfig, config DRM_AMDGPU still unconditionally
selects ACPI_VIDEO when ACPI is enabled:
# decidable, for the moment, to just select all of ACPI_VIDEO's
# dependencies, which is fewer than you might think.
select ACPI_VIDEO if ACPI
Because config ACPI_VIDEO in drivers/acpi/Kconfig depends on
BACKLIGHT_CLASS_DEVICE, enabling CONFIG_DRM_LUMINANCE=y with CONFIG_ACPI=y and
CONFIG_BACKLIGHT_CLASS_DEVICE=n triggers an unmet direct dependency warning
for ACPI_VIDEO.
In addition, include/linux/backlight.h declares backlight_device_register(),
backlight_device_unregister(), and backlight_device_set_brightness() without
inline stubs when CONFIG_BACKLIGHT_CLASS_DEVICE is disabled, while amdgpu
still compiles direct references to those symbols:
- amdgpu_dm_register_backlight_device() in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c is a non-static
function compiled unconditionally that calls backlight_device_register().
- amdgpu_dm_connector_destroy() in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c and
amdgpu_atombios_encoder_fini_backlight() in
drivers/gpu/drm/amd/amdgpu/atombios_encoders.c call
backlight_device_unregister() behind runtime pointer checks.
- amdgpu_atif_handler() in drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c calls
backlight_device_set_brightness(atif->bd, req.backlight_level) behind a
runtime check on atif->bd.
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> index 92c98e999efe9..209a7a6c246db 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> @@ -798,11 +798,31 @@ static int amdgpu_connector_set_lcd_property(struct drm_connector *connector,
> return 0;
> }
>
> +static void amdgpu_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct drm_encoder *encoder =
> + amdgpu_connector_best_single_encoder(conn_state->connector);
> +
> + if (encoder)
> + amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder),
> + conn_state->luminance);
> +}
> +
> +static void amdgpu_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct drm_encoder *encoder =
> + amdgpu_connector_best_single_encoder(conn_state->connector);
> +
> + if (encoder)
> + amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder), 0);
> +}
>
> static const struct drm_connector_helper_funcs amdgpu_connector_lvds_helper_funcs = {
> .get_modes = amdgpu_connector_lvds_get_modes,
> .mode_valid = amdgpu_connector_lvds_mode_valid,
> .best_encoder = amdgpu_connector_best_single_encoder,
> + .set_luminance = amdgpu_connector_set_luminance,
> + .disable_luminance = amdgpu_connector_disable_luminance,
> };
>
> static const struct drm_connector_funcs amdgpu_connector_lvds_funcs = {
> diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> index a1d05df0ac99b..dbff532ac1bcd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> +++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> @@ -191,6 +191,21 @@ void amdgpu_atombios_encoder_init_backlight(struct amdgpu_encoder *amdgpu_encode
> goto register_acpi_backlight;
> }
>
> + dig = amdgpu_encoder->enc_priv;
> + if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> + unsigned int brightness;
> +
> + brightness = amdgpu_atombios_encoder_get_backlight_level(amdgpu_encoder);
> + if (drm_connector_attach_luminance_property(drm_connector, 0,
> + AMDGPU_MAX_BL_LEVEL,
> + brightness))
> + return;
> +
> + dig->backlight_level = brightness;
> + dig->bl_dev = NULL;
> + return;
> + }
[Severity: High]
Can atomic LUMINANCE work on the legacy non-DC display path in
drivers/gpu/drm/amd/amdgpu/atombios_encoders.c and amdgpu_connectors.c?
Those files are used only when Display Core is not active, where
amdgpu_pci_probe() in drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c clears
DRIVER_ATOMIC:
if (!supports_atomic)
ddev->driver_features &= ~DRIVER_ATOMIC;
On this legacy path, connectors do not implement atomic state callbacks
(.reset, .atomic_duplicate_state, .atomic_destroy_state), so
drm_connector->state is NULL, and modesets use legacy CRTC helpers
(drm_crtc_helper_set_config() and drm_helper_connector_dpms()).
Because drm_connector_attach_luminance_property() creates LUMINANCE with
DRM_MODE_PROP_ATOMIC, non-atomic drivers hide LUMINANCE from userspace, legacy
property updates go through amdgpu_connector_set_lcd_property() (which ignores
everything except scaling_mode_property), and the legacy helpers never invoke
drm_atomic_helper_commit_luminance_enables() or
drm_atomic_helper_commit_luminance_disables(). Skipping backlight_device_register()
in amdgpu_atombios_encoder_init_backlight() therefore leaves legacy panels
with no working brightness control when CONFIG_DRM_LUMINANCE=y.
Even if atomic callbacks were reachable here, there are two additional issues
in drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c:
1. .set_luminance and .disable_luminance are only added to
amdgpu_connector_lvds_helper_funcs and are missing from
amdgpu_connector_dp_helper_funcs, which is used by DRM_MODE_CONNECTOR_eDP
and DP-bridge LVDS connectors that also call
amdgpu_atombios_encoder_init_backlight().
2. amdgpu_connector_disable_luminance() calls
amdgpu_atombios_encoder_set_backlight_level(to_amdgpu_encoder(encoder), 0),
which overwrites dig->backlight_level = 0 and mmBIOS_SCRATCH_2 = 0,
clobbering the saved brightness level used by
amdgpu_atombios_encoder_setup_dig() on enable and by dce_v*_suspend().
[ ... ]
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 0ea45ab12119c..53ff9a55c3a8e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
[ ... ]
> @@ -5397,11 +5398,16 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
> /* Update audio instances for each connector. */
> amdgpu_dm_commit_audio(dev, state);
>
> - /* restore the backlight level */
> - for (i = 0; i < dm->num_of_edps; i++) {
> - if (dm->backlight_dev[i] &&
> - (dm->actual_brightness[i] != dm->brightness[i]))
> - amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> + if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> + /* Apply brightness after the display pipeline is enabled. */
> + drm_atomic_helper_commit_luminance_enables(state);
> + } else {
> + /* Restore the legacy backlight level. */
> + for (i = 0; i < dm->num_of_edps; i++) {
> + if (dm->backlight_dev[i] &&
> + dm->actual_brightness[i] != dm->brightness[i])
> + amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> + }
> }
[Severity: Medium]
Unlike the !CONFIG_DRM_LUMINANCE branch, which checks
dm->actual_brightness[i] != dm->brightness[i], doesn't calling
drm_atomic_helper_commit_luminance_enables(state) -> amdgpu_dm_set_luminance() ->
amdgpu_dm_backlight_set_level() here reprogram the backlight on every atomic
commit that includes an active eDP connector in state even when
state->luminance has not changed?
In amdgpu_dm_atomic_commit_tail(), drm_atomic_helper_commit_luminance_enables()
runs after amdgpu_dm_commit_planes(). Inside amdgpu_dm_backlight_set_level()
in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c, the driver
exits IPS, reprograms the backlight via mod_power_set_backlight_nits() or
mod_power_set_backlight_percent(), and disables PSR and Panel Replay until the
next plane flip:
amdgpu_dm_psr_set_event(dm, stream, true,
psr_event_hw_programming, true);
amdgpu_dm_replay_set_event(dm, stream, true,
replay_event_hw_programming, true);
Because amdgpu_dm_commit_planes() -> amdgpu_dm_enable_self_refresh() has
already run earlier in amdgpu_dm_atomic_commit_tail(), any commit that pulls
the active eDP connector into state without changing state->luminance (such as
CRTC color management or VRR updates via drm_atomic_add_affected_connectors(),
ABM/HDR/scaling connector property updates, or compositor commits that include
connector properties) will immediately re-assert psr_event_hw_programming and
replay_event_hw_programming and block PSR/Replay entry.
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> index 0f664c658a127..0857354e7327e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
[ ... ]
> @@ -657,6 +662,10 @@ void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
> {
> struct dc_link *link = aconnector->dc_link;
> int bl_idx = dm->num_of_edps;
> + struct backlight_properties props = { 0 };
> + struct amdgpu_dm_backlight_caps *caps;
> + struct drm_connector *connector = &aconnector->base;
> + int ret;
>
> if (!(link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) ||
> link->type == dc_connection_none)
> @@ -675,6 +684,19 @@ void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
>
> amdgpu_dm_update_connector_ext_caps(aconnector);
>
> + caps = &dm->backlight_caps[bl_idx];
> + amdgpu_dm_backlight_fill_props(caps, power_supply_is_system_supplied() > 0,
> + !(amdgpu_dc_debug_mask &
> + DC_DISABLE_CUSTOM_BRIGHTNESS_CURVE),
> + &props);
> + if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> + unsigned int max = props.max_brightness;
> + unsigned int value = props.brightness;
> +
> + ret = drm_connector_attach_luminance_property(connector, 0, max, value);
> + drm_WARN_ON(adev_to_drm(dm->adev), ret);
> + }
> +
> /* Offer ABM property when user didn't turn off by module parameter.
> * OLED panels are included to support CACP (Content Adaptive
> * Contrast and Power) feature via set_abm_level.
[ ... ]
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> index b498fc5fbb140..f2c2a2e730d3e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
[ ... ]
> @@ -2038,7 +2039,8 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector)
> return r;
> }
>
> - amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
> + if (!IS_ENABLED(CONFIG_DRM_LUMINANCE))
> + amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
[Severity: High]
When CONFIG_DRM_LUMINANCE is enabled, skipping
amdgpu_dm_register_backlight_device() leaves dm->backlight_dev[i] as NULL and
leaves dm->brightness[bl_idx] uninitialized (0) in
amdgpu_dm_setup_backlight_device(). Doesn't this break several backlight
restoration and initialization paths in Display Core?
Specifically:
1. In dm_resume() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, the
GPU reset recovery branch (if (amdgpu_in_reset(adev))) does not go through
drm_atomic_helper_resume() or amdgpu_dm_atomic_commit_tail(), and instead
restores panel brightness via:
/* set the backlight after a reset */
for (i = 0; i < dm->num_of_edps; i++) {
if (dm->backlight_dev[i])
amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
}
Because dm->backlight_dev[i] is NULL (and dm->brightness[i] is 0 until a
luminance commit occurs), panel brightness will not be restored after a GPU
reset.
2. In amdgpu_dm_commit_streams() in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, after dc_commit_streams()
is called, backlight restoration is likewise gated on dm->backlight_dev[i]:
if (set_backlight_level) {
for (i = 0; i < dm->num_of_edps; i++) {
if (dm->backlight_dev[i])
amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
}
}
If a modeset happens on an external display on another CRTC while the eDP
connector is not part of the atomic commit state, neither
amdgpu_dm_commit_streams() nor drm_atomic_helper_commit_luminance_enables(state)
will restore the eDP brightness.
3. Unlike amdgpu_dm_register_backlight_device(), amdgpu_dm_setup_backlight_device()
does not check acpi_video_backlight_use_native() before attaching the
LUMINANCE property, and skipping amdgpu_dm_register_backlight_device() also
skips the acpi_video_register_backlight() fallback when native backlight
control should not be used.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=3
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 04/15] drm/amdgpu: Check bios_scratch_reg_offset in backlight level helper
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (2 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 03/15] drm/amd: add luminance property support Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 16:49 ` [PATCH v10 05/15] drm/amd/display: update KUnit luminance fixtures Mario Limonciello (AMD)
` (11 subsequent siblings)
15 siblings, 0 replies; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau
Ensure adev->bios_scratch_reg_offset is non-zero before accessing
scratch registers in amdgpu_atombios_scratch_regs_set_backlight_level()
to avoid dereferencing unmapped MMIO offsets.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
index 27c0dc8f6137a..2d8c2ba04e670 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -1622,7 +1622,12 @@ void amdgpu_atombios_scratch_regs_engine_hung(struct amdgpu_device *adev,
void amdgpu_atombios_scratch_regs_set_backlight_level(struct amdgpu_device *adev,
u32 backlight_level)
{
- u32 tmp = RREG32(adev->bios_scratch_reg_offset + 2);
+ u32 tmp;
+
+ if (!adev->bios_scratch_reg_offset)
+ return;
+
+ tmp = RREG32(adev->bios_scratch_reg_offset + 2);
tmp &= ~ATOM_S2_CURRENT_BL_LEVEL_MASK;
tmp |= (backlight_level << ATOM_S2_CURRENT_BL_LEVEL_SHIFT) &
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v10 05/15] drm/amd/display: update KUnit luminance fixtures
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (3 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 04/15] drm/amdgpu: Check bios_scratch_reg_offset in backlight level helper Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 18:00 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 06/15] drm/i915/display: add luminance property support Mario Limonciello (AMD)
` (10 subsequent siblings)
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau
Initialize complete DRM connector state in backlight test fixtures so
the atomic LUMINANCE property can be attached. Check the optional
property and keep the ABM property expectations correct with either
interface selected.
Also initialize the display-manager lock and mode configuration needed
by the existing backlight registration test.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
.../amdgpu_dm/tests/amdgpu_dm_backlight_test.c | 43 ++++++++++++++++++----
.../amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c | 7 +++-
2 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
index b21850788f36f..8883010e8af9c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
@@ -41,12 +41,21 @@ static void setup_test_connector(struct kunit *test,
struct dm_backlight_connector_fixture *fixture,
int bl_idx, enum signal_type signal)
{
- fixture->adev = kunit_kzalloc(test, sizeof(*fixture->adev), GFP_KERNEL);
- fixture->aconnector = kunit_kzalloc(test, sizeof(*fixture->aconnector), GFP_KERNEL);
+ int ret;
+
+ fixture->adev = dm_kunit_alloc_adev(test);
+ ret = drmm_mode_config_init(&fixture->adev->ddev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ fixture->aconnector = dm_kunit_alloc_connector(test, fixture->adev, NULL);
fixture->link = kunit_kzalloc(test, sizeof(*fixture->link), GFP_KERNEL);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->adev);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->aconnector);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->link);
+ ret = drmm_connector_init(&fixture->adev->ddev,
+ &fixture->aconnector->base,
+ &dm_backlight_test_connector_funcs,
+ DRM_MODE_CONNECTOR_eDP, NULL);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+ drm_atomic_helper_connector_reset(&fixture->aconnector->base);
fixture->aconnector->bl_idx = bl_idx;
fixture->aconnector->dc_link = fixture->link;
@@ -483,12 +492,17 @@ static void dm_test_register_backlight_device_success(struct kunit *test)
struct dc_link *link = dm_kunit_alloc_link(test);
struct drm_minor *primary;
unsigned int max;
+ int ret;
+
+ ret = drmm_mode_config_init(&adev->ddev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
setup_test_link_service(test, link);
link->dc->link_srv->edp_get_backlight_level = dm_test_get_backlight_level_error;
primary = kunit_kzalloc(test, sizeof(*primary), GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, primary);
adev->ddev.primary = primary;
+ adev->dm.ddev = &adev->ddev;
adev->dm.backlight_link[0] = link;
aconnector = dm_kunit_alloc_connector(test, adev, link);
@@ -1872,6 +1886,7 @@ static void dm_test_setup_backlight_device_attaches_abm_property(struct kunit *t
struct dc_link *link;
int saved_abm_level = amdgpu_dm_get_abm_level_param();
int saved_backlight = amdgpu_dm_get_backlight_param();
+ int abm_index;
int old_count;
int ret;
@@ -1890,6 +1905,7 @@ static void dm_test_setup_backlight_device_attaches_abm_property(struct kunit *t
&dm_backlight_test_connector_funcs,
DRM_MODE_CONNECTOR_eDP, NULL);
KUNIT_ASSERT_EQ(test, ret, 0);
+ drm_atomic_helper_connector_reset(&aconnector->base);
link = dm_kunit_alloc_link(test);
link->connector_signal = SIGNAL_TYPE_EDP;
@@ -1900,15 +1916,26 @@ static void dm_test_setup_backlight_device_attaches_abm_property(struct kunit *t
dm->adev = adev;
dm->ddev = &adev->ddev;
old_count = aconnector->base.base.properties->count;
+ abm_index = old_count + IS_ENABLED(CONFIG_DRM_LUMINANCE);
amdgpu_dm_setup_backlight_device(dm, aconnector);
KUNIT_EXPECT_EQ(test, dm->num_of_edps, 1);
KUNIT_EXPECT_EQ(test, aconnector->bl_idx, 0);
- KUNIT_EXPECT_EQ(test, aconnector->base.base.properties->count, old_count + 1);
- KUNIT_EXPECT_PTR_EQ(test, aconnector->base.base.properties->properties[old_count], prop);
- KUNIT_EXPECT_EQ(test, aconnector->base.base.properties->values[old_count],
- (uint64_t)ABM_SYSFS_CONTROL);
+ KUNIT_EXPECT_EQ(test, aconnector->base.base.properties->count,
+ old_count + 1 + IS_ENABLED(CONFIG_DRM_LUMINANCE));
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ KUNIT_EXPECT_NOT_NULL(test,
+ aconnector->base.luminance_property);
+ KUNIT_EXPECT_LE(test,
+ aconnector->base.state->luminance,
+ (uint64_t)MAX_BACKLIGHT_LEVEL);
+ }
+ KUNIT_EXPECT_PTR_EQ(test,
+ aconnector->base.base.properties->properties[abm_index],
+ prop);
+ KUNIT_EXPECT_EQ(test, aconnector->base.base.properties->values[abm_index],
+ (uint64_t)ABM_SYSFS_CONTROL);
amdgpu_dm_set_backlight_param(saved_backlight);
amdgpu_dm_set_abm_level_param(saved_abm_level);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c
index aad122c7229f5..66c9b2cb70fcd 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c
@@ -20,6 +20,7 @@
struct amdgpu_device *dm_kunit_alloc_adev(struct kunit *test)
{
+ struct amdgpu_device *adev;
struct drm_device *drm;
struct device *dev;
@@ -32,7 +33,11 @@ struct amdgpu_device *dm_kunit_alloc_adev(struct kunit *test)
DRIVER_MODESET | DRIVER_ATOMIC);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drm);
- return drm_to_adev(drm);
+ adev = drm_to_adev(drm);
+
+ mutex_init(&adev->dm.dc_lock);
+
+ return adev;
}
EXPORT_SYMBOL(dm_kunit_alloc_adev);
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 05/15] drm/amd/display: update KUnit luminance fixtures
2026-11-02 16:49 ` [PATCH v10 05/15] drm/amd/display: update KUnit luminance fixtures Mario Limonciello (AMD)
@ 2026-11-02 18:00 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 18:00 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/amd/display: wrong connector_type IDA freed in setup_test_connector() user
- [Medium] drm/amd/display: link failure in backlight KUnit test when DRM_LUMINANCE=y
--
commit 66d823079de9e4318719fbb2464a13997a1ae155
Author: Mario Limonciello (AMD) <superm1@kernel.org>
drm/amd/display: update KUnit luminance fixtures
This commit updates the backlight KUnit test fixtures to initialize DRM mode
configuration, connector state, and the display manager lock so tests work
when the atomic LUMINANCE property is enabled.
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> index b21850788f36f..8883010e8af9c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
> @@ -41,12 +41,21 @@ static void setup_test_connector(struct kunit *test,
> struct dm_backlight_connector_fixture *fixture,
> int bl_idx, enum signal_type signal)
> {
> - fixture->adev = kunit_kzalloc(test, sizeof(*fixture->adev), GFP_KERNEL);
> - fixture->aconnector = kunit_kzalloc(test, sizeof(*fixture->aconnector), GFP_KERNEL);
> + int ret;
> +
> + fixture->adev = dm_kunit_alloc_adev(test);
> + ret = drmm_mode_config_init(&fixture->adev->ddev);
> + KUNIT_ASSERT_EQ(test, ret, 0);
> +
> + fixture->aconnector = dm_kunit_alloc_connector(test, fixture->adev, NULL);
> fixture->link = kunit_kzalloc(test, sizeof(*fixture->link), GFP_KERNEL);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->adev);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->aconnector);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fixture->link);
> + ret = drmm_connector_init(&fixture->adev->ddev,
> + &fixture->aconnector->base,
> + &dm_backlight_test_connector_funcs,
> + DRM_MODE_CONNECTOR_eDP, NULL);
> + KUNIT_ASSERT_EQ(test, ret, 0);
> + drm_atomic_helper_connector_reset(&fixture->aconnector->base);
[Severity: Medium]
Does hardcoding DRM_MODE_CONNECTOR_eDP in setup_test_connector() corrupt or
leak the connector_type_id IDA when dm_test_should_create_sysfs_non_edp() runs?
drmm_connector_init() -> drm_connector_init_and_add() ->
drm_connector_init_only() allocates connector->connector_type_id from
drm_connector_enum_list[DRM_MODE_CONNECTOR_eDP].ida and registers
drm_connector_cleanup_action() for managed teardown.
However, dm_test_should_create_sysfs_non_edp() mutates connector_type after
calling setup_test_connector():
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c:dm_test_should_create_sysfs_non_edp() {
setup_test_connector(test, &fixture, 0, SIGNAL_TYPE_HDMI_TYPE_A);
fixture.aconnector->base.connector_type = DRM_MODE_CONNECTOR_HDMIA;
...
}
When the test finishes and drm_connector_cleanup() runs:
drivers/gpu/drm/drm_connector.c:drm_connector_cleanup() {
...
ida_free(&drm_connector_enum_list[connector->connector_type].ida,
connector->connector_type_id);
...
}
it frees connector_type_id back to the DRM_MODE_CONNECTOR_HDMIA IDA instead of
DRM_MODE_CONNECTOR_eDP, triggering a warning in ida_free() (or freeing an
active HDMI-A ID) while leaking the eDP connector_type_id.
> @@ -483,12 +492,17 @@ static void dm_test_register_backlight_device_success(struct kunit *test)
> struct dc_link *link = dm_kunit_alloc_link(test);
> struct drm_minor *primary;
> unsigned int max;
> + int ret;
> +
> + ret = drmm_mode_config_init(&adev->ddev);
> + KUNIT_ASSERT_EQ(test, ret, 0);
>
> setup_test_link_service(test, link);
> link->dc->link_srv->edp_get_backlight_level = dm_test_get_backlight_level_error;
> primary = kunit_kzalloc(test, sizeof(*primary), GFP_KERNEL);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, primary);
> adev->ddev.primary = primary;
> + adev->dm.ddev = &adev->ddev;
> adev->dm.backlight_link[0] = link;
[Severity: Medium]
Can this test suite link when CONFIG_DRM_LUMINANCE=y and
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled?
With commit de26f6665aed ("drm/amd: add luminance property support"),
DRM_AMDGPU in drivers/gpu/drm/amd/amdgpu/Kconfig only selects
BACKLIGHT_CLASS_DEVICE when !DRM_LUMINANCE:
select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
Neither DRM_AMD_DC_KUNIT_TEST in drivers/gpu/drm/amd/display/Kconfig nor
drivers/gpu/drm/amd/display/amdgpu_dm/tests/.kunitconfig enables
CONFIG_BACKLIGHT_CLASS_DEVICE.
However, dm_test_register_backlight_device_success() and
dm_test_unregister_backlight_device() in amdgpu_dm_backlight_test.c, as well
as amdgpu_dm_register_backlight_device() in amdgpu_dm_backlight.c, still
unconditionally call backlight_device_register() and
backlight_device_unregister():
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c:dm_test_unregister_backlight_device() {
backlight_device_unregister(data);
}
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c:amdgpu_dm_register_backlight_device() {
...
dm->backlight_dev[aconnector->bl_idx] =
backlight_device_register(bl_name, aconnector->base.kdev, dm,
&amdgpu_dm_backlight_ops, &props);
...
}
Because include/linux/backlight.h does not provide inline stubs for
backlight_device_register() or backlight_device_unregister() when
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled, won't building the KUnit tests (or
amdgpu) with CONFIG_DRM_LUMINANCE=y and CONFIG_BACKLIGHT_CLASS_DEVICE=n fail
at link time with undefined references to those symbols?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=5
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 06/15] drm/i915/display: add luminance property support
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (4 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 05/15] drm/amd/display: update KUnit luminance fixtures Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 18:17 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 07/15] drm/bridge: add connector luminance operations Mario Limonciello (AMD)
` (9 subsequent siblings)
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Expose the DRM LUMINANCE connector property for eDP, LVDS, and DSI panels
when CONFIG_DRM_LUMINANCE is enabled. Keep registering the existing
backlight class device otherwise, preserving its userspace ABI.
Use the existing Intel brightness scaling and programming path from
connector helper callbacks. Apply atomic luminance after modeset enables
so updates made while a panel is inactive take effect when it is enabled
again. The shared display code provides the same behavior to i915 and Xe.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/i915/Kconfig | 2 +-
drivers/gpu/drm/i915/display/icl_dsi.c | 1 +
drivers/gpu/drm/i915/display/intel_backlight.c | 20 ++++++++++++++++++++
drivers/gpu/drm/i915/display/intel_backlight.h | 1 +
drivers/gpu/drm/i915/display/intel_display.c | 2 ++
drivers/gpu/drm/i915/display/intel_dp.c | 1 +
drivers/gpu/drm/i915/display/intel_lvds.c | 1 +
drivers/gpu/drm/i915/display/intel_panel.c | 8 +++++---
drivers/gpu/drm/i915/display/vlv_dsi.c | 1 +
drivers/gpu/drm/xe/Kconfig | 2 +-
10 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 71426b9c91f00..720191673a1e7 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -26,7 +26,7 @@ config DRM_I915
select IRQ_WORK
# i915 depends on ACPI_VIDEO when ACPI is enabled
# but for select to work, need to select ACPI_VIDEO's dependencies, ick
- select BACKLIGHT_CLASS_DEVICE if ACPI
+ select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE
select INPUT if ACPI
select ACPI_WMI if ACPI
select ACPI_VIDEO if ACPI
diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
index ea0cdb7822f39..37ff84c03ce05 100644
--- a/drivers/gpu/drm/i915/display/icl_dsi.c
+++ b/drivers/gpu/drm/i915/display/icl_dsi.c
@@ -1801,6 +1801,7 @@ static const struct drm_connector_helper_funcs gen11_dsi_connector_helper_funcs
.get_modes = intel_dsi_get_modes,
.mode_valid = gen11_dsi_mode_valid,
.atomic_check = intel_digital_connector_atomic_check,
+ .set_luminance = intel_backlight_set_luminance,
};
static int gen11_dsi_host_attach(struct mipi_dsi_host *host,
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
index b128896cb1c2d..6d658c7b5d99f 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -9,6 +9,7 @@
#include <linux/string_helpers.h>
#include <acpi/video.h>
+#include <drm/drm_connector.h>
#include <drm/drm_file.h>
#include <drm/drm_print.h>
#include <drm/intel/pci_config.h>
@@ -1670,6 +1671,9 @@ int intel_backlight_setup(struct intel_connector *connector, enum pipe pipe)
{
struct intel_display *display = to_intel_display(connector);
struct intel_panel *panel = &connector->panel;
+ struct drm_connector *base = &connector->base;
+ u32 brightness;
+ u32 max;
int ret;
if (!connector->panel.vbt.backlight.present) {
@@ -1703,6 +1707,13 @@ int intel_backlight_setup(struct intel_connector *connector, enum pipe pipe)
panel->backlight.present = true;
+ max = panel->backlight.max;
+ brightness = scale_hw_to_user(connector, panel->backlight.level, max);
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ ret = drm_connector_attach_luminance_property(base, 0, max, brightness);
+ drm_WARN_ON(display->drm, ret);
+ }
+
drm_dbg_kms(display->drm,
"[CONNECTOR:%d:%s] backlight initialized, %s, brightness %u/%u\n",
connector->base.base.id, connector->base.name,
@@ -1712,6 +1723,15 @@ int intel_backlight_setup(struct intel_connector *connector, enum pipe pipe)
return 0;
}
+void intel_backlight_set_luminance(const struct drm_connector_state *conn_state)
+{
+ struct intel_connector *connector =
+ to_intel_connector(conn_state->connector);
+
+ intel_panel_set_backlight(conn_state, conn_state->luminance,
+ connector->panel.backlight.max);
+}
+
void intel_backlight_destroy(struct intel_panel *panel)
{
/* dispose of the pwm */
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.h b/drivers/gpu/drm/i915/display/intel_backlight.h
index 339643f638978..1634ce414695a 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.h
+++ b/drivers/gpu/drm/i915/display/intel_backlight.h
@@ -32,6 +32,7 @@ void intel_backlight_set_acpi(const struct drm_connector_state *conn_state,
u32 level, u32 max);
void intel_backlight_set_pwm_level(const struct drm_connector_state *conn_state,
u32 level);
+void intel_backlight_set_luminance(const struct drm_connector_state *conn_state);
u32 intel_backlight_invert_pwm_level(struct intel_connector *connector, u32 level);
u32 intel_backlight_level_to_pwm(struct intel_connector *connector, u32 level);
u32 intel_backlight_level_from_pwm(struct intel_connector *connector, u32 val);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index d5d4453f283bb..3c81ed630c365 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7696,6 +7696,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
/* Now enable the clocks, plane, pipe, and connectors that we set up. */
display->modeset.funcs->commit_modeset_enables(state);
+ drm_atomic_helper_commit_luminance_enables(&state->base);
+
intel_display_power_dc3co_compute(state);
/* FIXME probably need to sequence this properly */
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index d8c0d9381471d..0b8ca17cc1152 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6883,6 +6883,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs =
.get_modes = intel_dp_get_modes,
.mode_valid = intel_dp_mode_valid,
.atomic_check = intel_dp_connector_atomic_check,
+ .set_luminance = intel_backlight_set_luminance,
};
enum irqreturn
diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
index 872753478cf21..fa668b1debb21 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -502,6 +502,7 @@ static const struct drm_connector_helper_funcs intel_lvds_connector_helper_funcs
.get_modes = intel_lvds_get_modes,
.mode_valid = intel_lvds_mode_valid,
.atomic_check = intel_digital_connector_atomic_check,
+ .set_luminance = intel_backlight_set_luminance,
};
static const struct drm_connector_funcs intel_lvds_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
index 81e638d0c7b33..501573a75b89c 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -610,9 +610,11 @@ int intel_panel_register(struct intel_connector *connector)
struct intel_panel *panel = &connector->panel;
int ret;
- ret = intel_backlight_device_register(connector);
- if (ret)
- return ret;
+ if (!IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ ret = intel_backlight_device_register(connector);
+ if (ret)
+ return ret;
+ }
if (connector->base.connector_type == DRM_MODE_CONNECTOR_DSI ||
connector->base.connector_type == DRM_MODE_CONNECTOR_eDP) {
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
index 8829f365592ef..7e60407e0af4a 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -1555,6 +1555,7 @@ static const struct drm_connector_helper_funcs intel_dsi_connector_helper_funcs
.get_modes = intel_dsi_get_modes,
.mode_valid = vlv_dsi_mode_valid,
.atomic_check = intel_digital_connector_atomic_check,
+ .set_luminance = intel_backlight_set_luminance,
};
static const struct drm_connector_funcs intel_dsi_connector_funcs = {
diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 502c7b3c1f16a..6418ccc8e3468 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -26,7 +26,7 @@ config DRM_XE
select IRQ_WORK
# xe depends on ACPI_VIDEO when ACPI is enabled
# but for select to work, need to select ACPI_VIDEO's dependencies, ick
- select BACKLIGHT_CLASS_DEVICE if ACPI
+ select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE
select INPUT if ACPI
select ACPI_VIDEO if ACPI
select ACPI_WMI if ACPI
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 06/15] drm/i915/display: add luminance property support
2026-11-02 16:49 ` [PATCH v10 06/15] drm/i915/display: add luminance property support Mario Limonciello (AMD)
@ 2026-11-02 18:17 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 18:17 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/i915/display: missing acpi_video_backlight_use_native() call
- [Medium] drm/i915/display: stale backlight level programmed on modeset enable
- [Low] drm/i915/display: build failure in intel_backlight_set_luminance()
- [Low] drm/i915: unmet BACKLIGHT_CLASS_DEVICE dependency for ACPI_VIDEO
--
commit e18daa9a197d727d1dc5e00c9bb74f3e09fc2495
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/i915/display: add luminance property support
This commit adds support for the DRM LUMINANCE connector property on eDP,
LVDS, and DSI panels in i915 and Xe when CONFIG_DRM_LUMINANCE is enabled,
while retaining the backlight class device when it is disabled.
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 71426b9c91f00..720191673a1e7 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -26,7 +26,7 @@ config DRM_I915
> select IRQ_WORK
> # i915 depends on ACPI_VIDEO when ACPI is enabled
> # but for select to work, need to select ACPI_VIDEO's dependencies, ick
> - select BACKLIGHT_CLASS_DEVICE if ACPI
> + select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE
> select INPUT if ACPI
> select ACPI_WMI if ACPI
> select ACPI_VIDEO if ACPI
[Severity: Low]
Does this cause an unmet direct dependency for ACPI_VIDEO when
CONFIG_DRM_LUMINANCE=y, CONFIG_ACPI=y, and CONFIG_BACKLIGHT_CLASS_DEVICE=n?
In drivers/acpi/Kconfig, ACPI_VIDEO depends directly on
BACKLIGHT_CLASS_DEVICE:
config ACPI_VIDEO
tristate "Video"
depends on BACKLIGHT_CLASS_DEVICE
Both DRM_I915 here and DRM_XE in drivers/gpu/drm/xe/Kconfig still have:
select ACPI_VIDEO if ACPI
When DRM_LUMINANCE is enabled and nothing else selects
BACKLIGHT_CLASS_DEVICE, Kconfig warns about an unmet direct dependency on
BACKLIGHT_CLASS_DEVICE and drivers/acpi/acpi_video.c fails to link due to
undefined references to backlight_device_register() and
backlight_device_unregister().
[ ... ]
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index b128896cb1c2d..6d658c7b5d99f 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
[ ... ]
> @@ -1712,6 +1723,15 @@ int intel_backlight_setup(struct intel_connector *connector, enum pipe pipe)
> return 0;
> }
>
> +void intel_backlight_set_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct intel_connector *connector =
> + to_intel_connector(conn_state->connector);
> +
> + intel_panel_set_backlight(conn_state, conn_state->luminance,
> + connector->panel.backlight.max);
> +}
[Severity: Low]
Will drivers/gpu/drm/i915/display/intel_backlight.c fail to build when
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled?
In drivers/gpu/drm/i915/display/intel_backlight.c, scale_user_to_hw() and
intel_panel_set_backlight() are defined inside an
#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) block:
#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
static u32 intel_panel_get_backlight(struct intel_connector *connector)
...
static void intel_panel_set_backlight(const struct drm_connector_state *conn_state,
u32 user_level, u32 user_max)
Because intel_backlight_set_luminance() is compiled unconditionally outside
that guard, building with CONFIG_BACKLIGHT_CLASS_DEVICE=n (for example when
CONFIG_DRM_LUMINANCE=y) fails with an implicit declaration of
intel_panel_set_backlight().
[ ... ]
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index d5d4453f283bb..3c81ed630c365 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7696,6 +7696,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> /* Now enable the clocks, plane, pipe, and connectors that we set up. */
> display->modeset.funcs->commit_modeset_enables(state);
>
> + drm_atomic_helper_commit_luminance_enables(&state->base);
> +
> intel_display_power_dc3co_compute(state);
[Severity: Medium]
When a panel's LUMINANCE property is updated while the CRTC is inactive (or
in the same atomic commit that enables the CRTC), doesn't calling
drm_atomic_helper_commit_luminance_enables() after commit_modeset_enables()
initially turn on the panel at the stale brightness level?
While the CRTC is inactive, drm_atomic_helper_commit_luminance_enables()
skips calling .set_luminance(), so panel->backlight.level keeps the old
hardware brightness level while conn_state->luminance holds the new value.
During a subsequent modeset enable in intel_atomic_commit_tail():
display->modeset.funcs->commit_modeset_enables(state)
-> intel_edp_backlight_on() / intel_enable_lvds() / gen11_dsi_enable()
-> intel_backlight_enable()
-> __intel_backlight_enable()
-> panel->backlight.funcs->enable(crtc_state, conn_state,
panel->backlight.level);
This powers on and programs the backlight with the old
panel->backlight.level and waits for vblank before
drm_atomic_helper_commit_luminance_enables() calls
intel_backlight_set_luminance() to update panel->backlight.level and
reprogram the hardware a second time.
[ ... ]
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index 81e638d0c7b33..501573a75b89c 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -610,9 +610,11 @@ int intel_panel_register(struct intel_connector *connector)
> struct intel_panel *panel = &connector->panel;
> int ret;
>
> - ret = intel_backlight_device_register(connector);
> - if (ret)
> - return ret;
> + if (!IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> + ret = intel_backlight_device_register(connector);
> + if (ret)
> + return ret;
> + }
[Severity: High]
When CONFIG_DRM_LUMINANCE is enabled, how does the driver coordinate with
ACPI video backlight detection and BIOS OpRegion ASLE backlight requests now
that intel_backlight_device_register() is skipped?
Previously, intel_backlight_device_register() called
acpi_video_backlight_use_native(), which notifies drivers/acpi/video_detect.c
that native GPU backlight control is present (setting native_available = true)
and checks whether native backlight control should be used. It also set
panel->backlight.device.
With intel_backlight_device_register() skipped and
intel_backlight_setup() attaching the LUMINANCE property without calling
acpi_video_backlight_use_native():
1. In drivers/gpu/drm/i915/display/intel_acpi.c, intel_acpi_video_register()
checks:
if (panel->backlight.funcs && !panel->backlight.device) {
acpi_video_register_backlight();
break;
}
Because panel->backlight.device is NULL and acpi_video_backlight_use_native()
was never called, intel_acpi_video_register() will register a conflicting
acpi_video backlight device if CONFIG_BACKLIGHT_CLASS_DEVICE is enabled.
2. In drivers/gpu/drm/i915/display/intel_opregion.c, asle_set_backlight()
checks:
if (acpi_video_get_backlight_type() == acpi_backlight_native) {
drm_dbg_kms(display->drm,
"opregion backlight request ignored\n");
return 0;
}
Because acpi_video_backlight_use_native() was never called,
acpi_video_get_backlight_type() will not return acpi_backlight_native, so
BIOS OpRegion ASLE requests will call intel_backlight_set_acpi() and modify
panel->backlight.level behind the atomic LUMINANCE property's back.
3. In intel_backlight_setup(), the LUMINANCE property is attached even on
systems where acpi_video_backlight_use_native() would return false.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=6
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 07/15] drm/bridge: add connector luminance operations
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (5 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 06/15] drm/i915/display: add luminance property support Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 18:24 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 08/15] drm/panel: add connector luminance forwarding Mario Limonciello (AMD)
` (8 subsequent siblings)
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Allow bridges that own brightness control to advertise a luminance
operation and its supported relative range.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v10:
* Split bridge operations out of the combined panel and bridge patch
---
include/drm/drm_bridge.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 1981d24a700d2..95f5c2b8656cd 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -62,6 +62,15 @@ enum drm_bridge_attach_flags {
* struct drm_bridge_funcs - drm_bridge control functions
*/
struct drm_bridge_funcs {
+ /**
+ * @set_luminance:
+ *
+ * Apply a relative luminance level to a bridge that controls panel
+ * emission. Bridges implementing this callback shall set
+ * @DRM_BRIDGE_OP_LUMINANCE.
+ */
+ void (*set_luminance)(struct drm_bridge *bridge, unsigned int luminance);
+
/**
* @attach:
*
@@ -1021,6 +1030,11 @@ enum drm_bridge_ops {
* &drm_bridge_funcs->hdmi_clear_spd_infoframe callbacks.
*/
DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME = BIT(10),
+
+ /**
+ * @DRM_BRIDGE_OP_LUMINANCE: The bridge controls panel luminance.
+ */
+ DRM_BRIDGE_OP_LUMINANCE = BIT(11),
};
/**
@@ -1071,6 +1085,12 @@ struct drm_bridge {
void *driver_private;
/** @ops: bitmask of operations supported by the bridge */
enum drm_bridge_ops ops;
+
+ /** @luminance_max: Maximum relative luminance level. */
+ unsigned int luminance_max;
+
+ /** @luminance: Current relative luminance level. */
+ unsigned int luminance;
/**
* @type: Type of the connection at the bridge output
* (DRM_MODE_CONNECTOR_*). For bridges at the end of this chain this
@@ -1522,6 +1542,60 @@ void drm_bridge_hpd_disable(struct drm_bridge *bridge);
void drm_bridge_hpd_notify(struct drm_bridge *bridge,
enum drm_connector_status status);
+#ifdef CONFIG_DRM_PANEL_BRIDGE
+bool drm_bridge_is_panel(const struct drm_bridge *bridge);
+struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);
+struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
+ u32 connector_type);
+void drm_panel_bridge_remove(struct drm_bridge *bridge);
+int drm_panel_bridge_set_orientation(struct drm_connector *connector,
+ struct drm_bridge *bridge);
+struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
+ struct drm_panel *panel);
+struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
+ struct drm_panel *panel,
+ u32 connector_type);
+struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
+ struct drm_panel *panel);
+struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
+#else
+static inline bool drm_bridge_is_panel(const struct drm_bridge *bridge)
+{
+ return false;
+}
+
+static inline int drm_panel_bridge_set_orientation(struct drm_connector *connector,
+ struct drm_bridge *bridge)
+{
+ return -EINVAL;
+}
+
+#endif
+
+#if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL_BRIDGE)
+struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
+ u32 port, u32 endpoint);
+struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, struct device_node *node,
+ u32 port, u32 endpoint);
+#else
+static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
+ struct device_node *node,
+ u32 port,
+ u32 endpoint)
+{
+ return ERR_PTR(-ENODEV);
+}
+
+static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
+ struct device_node *node,
+ u32 port,
+ u32 endpoint)
+{
+ return ERR_PTR(-ENODEV);
+}
+#endif
+
+void devm_drm_put_bridge(struct device *dev, struct drm_bridge *bridge);
void drm_bridge_debugfs_params(struct dentry *root);
void drm_bridge_debugfs_encoder_params(struct dentry *root, struct drm_encoder *encoder);
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 07/15] drm/bridge: add connector luminance operations
2026-11-02 16:49 ` [PATCH v10 07/15] drm/bridge: add connector luminance operations Mario Limonciello (AMD)
@ 2026-11-02 18:24 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 18:24 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/bridge: rebase artifact in drm_bridge.h causes redefinition build failure
--
commit 13f21a6ed9ffad8454f947df0968b7d42361d36f
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/bridge: add connector luminance operations
This commit adds a set_luminance callback, DRM_BRIDGE_OP_LUMINANCE flag, and
relative luminance fields to struct drm_bridge so bridges that control panel
brightness can advertise luminance operations.
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 1981d24a700d2..95f5c2b8656cd 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
[ ... ]
> @@ -1522,6 +1542,60 @@ void drm_bridge_hpd_disable(struct drm_bridge *bridge);
> void drm_bridge_hpd_notify(struct drm_bridge *bridge,
> enum drm_connector_status status);
>
> +#ifdef CONFIG_DRM_PANEL_BRIDGE
> +bool drm_bridge_is_panel(const struct drm_bridge *bridge);
> +struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);
> +struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
> + u32 connector_type);
> +void drm_panel_bridge_remove(struct drm_bridge *bridge);
> +int drm_panel_bridge_set_orientation(struct drm_connector *connector,
> + struct drm_bridge *bridge);
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> + struct drm_panel *panel);
> +struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
> + struct drm_panel *panel,
> + u32 connector_type);
> +struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
> + struct drm_panel *panel);
> +struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
> +#else
> +static inline bool drm_bridge_is_panel(const struct drm_bridge *bridge)
> +{
> + return false;
> +}
> +
> +static inline int drm_panel_bridge_set_orientation(struct drm_connector *connector,
> + struct drm_bridge *bridge)
> +{
> + return -EINVAL;
> +}
> +
> +#endif
> +
> +#if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL_BRIDGE)
> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
> + u32 port, u32 endpoint);
> +struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, struct device_node *node,
> + u32 port, u32 endpoint);
> +#else
> +static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> + struct device_node *node,
> + u32 port,
> + u32 endpoint)
> +{
> + return ERR_PTR(-ENODEV);\n> +}
> +
> +static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
> + struct device_node *node,
> + u32 port,
> + u32 endpoint)
> +{
> + return ERR_PTR(-ENODEV);\n> +}
> +#endif
> +
> +void devm_drm_put_bridge(struct device *dev, struct drm_bridge *bridge);
[Severity: Low]
Was this hunk included by mistake during a rebase or patch split?
Prior to this series, commit aa8a8c7e4641 ("drm/bridge: panel: move all code
to drm_panel.c") moved the CONFIG_DRM_PANEL_BRIDGE and CONFIG_OF panel-bridge
declarations and their #else static inline fallback definitions from
include/drm/drm_bridge.h into include/drm/drm_panel.h, and commit 51afe16e33a0
("drm/bridge: remove devm_drm_put_bridge()") removed devm_drm_put_bridge().
Because include/drm/drm_panel.h includes drm/drm_bridge.h and already defines
those same static inline functions in its #else branches:
include/drm/drm_panel.h:
#ifdef CONFIG_DRM_PANEL_BRIDGE
bool drm_bridge_is_panel(const struct drm_bridge *bridge);
...
#else
static inline bool drm_bridge_is_panel(const struct drm_bridge *bridge)
{
return false;
}
static inline int drm_panel_bridge_set_orientation(struct drm_connector *connector,
struct drm_bridge *bridge)
{
return -EINVAL;
}
#endif
#if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL_BRIDGE)
...
#else
static inline struct drm_bridge *devm_drm_of_get_bridge(...)
...
static inline struct drm_bridge *drmm_of_get_bridge(...)
...
#endif
does re-adding this block in include/drm/drm_bridge.h cause redefinition build
failures in translation units that include drm/drm_panel.h (such as
drivers/gpu/drm/display/drm_bridge_connector.c, drivers/gpu/drm/drm_panel.c,
and drivers/gpu/drm/drm_of.c) when CONFIG_OF or CONFIG_DRM_PANEL_BRIDGE is
disabled?
Although a subsequent patch in the series, commit 3eac0878c8a6 ("drm/panel:
add connector luminance forwarding"), removes the #else static inline stubs
and the CONFIG_OF block from include/drm/drm_panel.h, this breaks bisection at
this commit and leaves the misplaced panel-bridge and OF bridge declarations
along with the deleted devm_drm_put_bridge() prototype in
include/drm/drm_bridge.h.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=7
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 08/15] drm/panel: add connector luminance forwarding
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (6 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 07/15] drm/bridge: add connector luminance operations Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 18:34 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 09/15] drm/bridge_connector: add luminance support Mario Limonciello (AMD)
` (7 subsequent siblings)
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Add helpers that expose a panel backlight through the connector
LUMINANCE property and forward connector updates to the backlight
backend.
Hook the helpers into panel bridges that create their own connector.
Panels consumed directly, or by a panel bridge attached with
DRM_BRIDGE_ATTACH_NO_CONNECTOR, remain unchanged.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v10:
* Split panel support out of the combined panel and bridge patch
* Limit integration to panel bridges that own their connector
---
drivers/gpu/drm/drm_panel.c | 99 +++++++++++++++++++++++++++++++++++++++++----
include/drm/drm_panel.h | 39 +++---------------
2 files changed, 97 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index c00529bfb706b..a5706edad9735 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -38,6 +38,19 @@ static LIST_HEAD(panel_list);
* take look at drm_panel_bridge_add() and devm_drm_panel_bridge_add().
*/
+struct panel_bridge {
+ struct drm_bridge bridge;
+ struct drm_connector connector;
+ struct drm_panel *panel;
+ u32 connector_type;
+};
+
+static inline struct panel_bridge *
+drm_connector_to_panel_bridge(const struct drm_connector *connector)
+{
+ return container_of(connector, struct panel_bridge, connector);
+}
+
static inline struct drm_panel *
drm_bridge_to_panel(const struct drm_bridge *bridge)
{
@@ -50,13 +63,6 @@ drm_connector_to_panel(const struct drm_connector *connector)
return container_of(connector, struct drm_panel, connector);
}
-struct panel_bridge {
- struct drm_bridge bridge;
- struct drm_connector connector;
- struct drm_panel *panel;
- u32 connector_type;
-};
-
static int panel_bridge_connector_get_modes(struct drm_connector *connector)
{
struct drm_panel *panel = drm_connector_to_panel(connector);
@@ -64,9 +70,29 @@ static int panel_bridge_connector_get_modes(struct drm_connector *connector)
return drm_panel_get_modes(panel, connector);
}
+static void
+panel_bridge_connector_set_luminance(const struct drm_connector_state *conn_state)
+{
+ struct panel_bridge *panel_bridge =
+ drm_connector_to_panel_bridge(conn_state->connector);
+
+ drm_panel_set_luminance(panel_bridge->panel, conn_state->luminance);
+}
+
+static void
+panel_bridge_connector_disable_luminance(const struct drm_connector_state *conn_state)
+{
+ struct panel_bridge *panel_bridge =
+ drm_connector_to_panel_bridge(conn_state->connector);
+
+ drm_panel_disable_luminance(panel_bridge->panel);
+}
+
static const struct drm_connector_helper_funcs
panel_bridge_connector_helper_funcs = {
.get_modes = panel_bridge_connector_get_modes,
+ .set_luminance = panel_bridge_connector_set_luminance,
+ .disable_luminance = panel_bridge_connector_disable_luminance,
};
static const struct drm_connector_funcs panel_bridge_connector_funcs = {
@@ -101,6 +127,10 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
drm_panel_bridge_set_orientation(connector, bridge);
+ ret = drm_panel_attach_luminance_property(panel, connector);
+ if (ret)
+ return ret;
+
drm_connector_attach_encoder(connector, encoder);
if (bridge->dev->registered) {
@@ -861,6 +891,61 @@ void drm_panel_disable(struct drm_panel *panel)
}
EXPORT_SYMBOL(drm_panel_disable);
+/**
+ * drm_panel_attach_luminance_property - expose a panel backlight through DRM
+ * @panel: panel whose backlight is controlled
+ * @connector: connector on which to expose the property
+ *
+ * The backlight class device remains the internal hardware backend. When the
+ * luminance UAPI is enabled, userspace controls that backend through the DRM
+ * connector property instead.
+ *
+ * Return: 0 on success or a negative error code
+ */
+int drm_panel_attach_luminance_property(struct drm_panel *panel,
+ struct drm_connector *connector)
+{
+ struct backlight_device *backlight;
+
+ if (!IS_ENABLED(CONFIG_DRM_LUMINANCE) || !panel)
+ return 0;
+
+ backlight = panel->backlight;
+ if (!backlight)
+ return 0;
+
+ return drm_connector_attach_luminance_property(connector, 0,
+ backlight->props.max_brightness,
+ backlight->props.brightness);
+}
+EXPORT_SYMBOL(drm_panel_attach_luminance_property);
+
+/**
+ * drm_panel_set_luminance - set a panel backlight's relative luminance
+ * @panel: panel whose backlight is controlled
+ * @luminance: relative luminance level
+ */
+void drm_panel_set_luminance(struct drm_panel *panel, unsigned int luminance)
+{
+ if (!panel || !panel->backlight)
+ return;
+
+ panel->backlight->props.brightness = luminance;
+ backlight_update_status(panel->backlight);
+}
+EXPORT_SYMBOL(drm_panel_set_luminance);
+
+/**
+ * drm_panel_disable_luminance - turn off a panel backlight
+ * @panel: panel whose backlight is controlled
+ */
+void drm_panel_disable_luminance(struct drm_panel *panel)
+{
+ if (panel)
+ backlight_disable(panel->backlight);
+}
+EXPORT_SYMBOL(drm_panel_disable_luminance);
+
/**
* drm_panel_get_modes - probe the available display modes of a panel
* @panel: DRM panel
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 58113ea7d63a0..76d49d4485ff0 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -342,6 +342,11 @@ void drm_panel_unprepare(struct drm_panel *panel);
void drm_panel_enable(struct drm_panel *panel);
void drm_panel_disable(struct drm_panel *panel);
+int drm_panel_attach_luminance_property(struct drm_panel *panel,
+ struct drm_connector *connector);
+void drm_panel_set_luminance(struct drm_panel *panel, unsigned int luminance);
+void drm_panel_disable_luminance(struct drm_panel *panel);
+
int drm_panel_get_modes(struct drm_panel *panel, struct drm_connector *connector);
#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_DRM_PANEL)
@@ -418,40 +423,6 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
struct drm_panel *panel);
struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
-#else
-static inline bool drm_bridge_is_panel(const struct drm_bridge *bridge)
-{
- return false;
-}
-
-static inline int drm_panel_bridge_set_orientation(struct drm_connector *connector,
- struct drm_bridge *bridge)
-{
- return -EINVAL;
-}
-#endif
-
-#if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL_BRIDGE)
-struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
- u32 port, u32 endpoint);
-struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, struct device_node *node,
- u32 port, u32 endpoint);
-#else
-static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
- struct device_node *node,
- u32 port,
- u32 endpoint)
-{
- return ERR_PTR(-ENODEV);
-}
-
-static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
- struct device_node *node,
- u32 port,
- u32 endpoint)
-{
- return ERR_PTR(-ENODEV);
-}
#endif
#endif
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 08/15] drm/panel: add connector luminance forwarding
2026-11-02 16:49 ` [PATCH v10 08/15] drm/panel: add connector luminance forwarding Mario Limonciello (AMD)
@ 2026-11-02 18:34 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 18:34 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] drm/panel: invalid container_of cast to panel_bridge in luminance callbacks
- [High] drm/panel: drm_panel_disable_luminance() and drm_panel_set_luminance() break PSR and blank state
- [Medium] drm/panel: missing drm_connector_cleanup() on error in panel_bridge_attach()
- [Low] drm/panel: unintended removal of panel bridge stubs from drm_panel.h
--
commit 3eac0878c8a6609d8eb6adf67569604a16f5b3e4
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/panel: add connector luminance forwarding
This commit adds helpers to expose a panel backlight via the DRM connector
luminance property and hooks them into panel bridges that create their own
connector.
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index c00529bfb706b..a5706edad9735 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -38,6 +38,19 @@ static LIST_HEAD(panel_list);
> * take look at drm_panel_bridge_add() and devm_drm_panel_bridge_add().
> */
>
> +struct panel_bridge {
> + struct drm_bridge bridge;
> + struct drm_connector connector;
> + struct drm_panel *panel;
> + u32 connector_type;
> +};
> +
> +static inline struct panel_bridge *
> +drm_connector_to_panel_bridge(const struct drm_connector *connector)
> +{
> + return container_of(connector, struct panel_bridge, connector);
> +}
> +
> static inline struct drm_panel *
> drm_bridge_to_panel(const struct drm_bridge *bridge)
> {
> @@ -50,13 +63,6 @@ drm_connector_to_panel(const struct drm_connector *connector)
> return container_of(connector, struct drm_panel, connector);
> }
>
> -struct panel_bridge {
> - struct drm_bridge bridge;
> - struct drm_connector connector;
> - struct drm_panel *panel;
> - u32 connector_type;
> -};
> -
> static int panel_bridge_connector_get_modes(struct drm_connector *connector)
> {
> struct drm_panel *panel = drm_connector_to_panel(connector);
> @@ -64,9 +70,29 @@ static int panel_bridge_connector_get_modes(struct drm_connector *connector)
> return drm_panel_get_modes(panel, connector);
> }
>
> +static void
> +panel_bridge_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct panel_bridge *panel_bridge =
> + drm_connector_to_panel_bridge(conn_state->connector);
> +
> + drm_panel_set_luminance(panel_bridge->panel, conn_state->luminance);
> +}
> +
> +static void
> +panel_bridge_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct panel_bridge *panel_bridge =
> + drm_connector_to_panel_bridge(conn_state->connector);
> +
> + drm_panel_disable_luminance(panel_bridge->panel);
> +}
[Severity: Critical]
Since commit b7797081ccfe ("drm/panel: embed a drm_bridge into every
drm_panel"), the bridge and connector are embedded directly inside struct
drm_panel, and struct panel_bridge is an unused leftover definition that is
never allocated.
In panel_bridge_attach(), the connector initialized with
panel_bridge_connector_helper_funcs is &panel->connector:
struct drm_panel *panel = drm_bridge_to_panel(bridge);
struct drm_connector *connector = &panel->connector;
Calling drm_connector_to_panel_bridge(conn_state->connector) in
panel_bridge_connector_set_luminance() and
panel_bridge_connector_disable_luminance() computes container_of() for struct
panel_bridge on a struct drm_panel instance, so reading panel_bridge->panel
reads the fields following connector in struct drm_panel (connector_type and
list) as a struct drm_panel pointer.
Won't passing that pointer to drm_panel_set_luminance() or
drm_panel_disable_luminance() dereference an invalid panel->backlight address,
and should these callbacks use drm_connector_to_panel(conn_state->connector)
instead?
> static const struct drm_connector_helper_funcs
> panel_bridge_connector_helper_funcs = {
> .get_modes = panel_bridge_connector_get_modes,
> + .set_luminance = panel_bridge_connector_set_luminance,
> + .disable_luminance = panel_bridge_connector_disable_luminance,
> };
>
> static const struct drm_connector_funcs panel_bridge_connector_funcs = {
> @@ -101,6 +127,10 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>
> drm_panel_bridge_set_orientation(connector, bridge);
>
> + ret = drm_panel_attach_luminance_property(panel, connector);
> + if (ret)
> + return ret;
[Severity: Medium]
In panel_bridge_attach(), drm_connector_init() has already succeeded and
linked &panel->connector into bridge->dev->mode_config.connector_list before
drm_panel_attach_luminance_property() is called.
When bridge->funcs->attach() fails in drm_bridge_attach(), the error path
resets the bridge without calling bridge->funcs->detach().
Should this error path call drm_connector_cleanup(connector) before returning
ret so that &panel->connector is not left linked on mode_config.connector_list
if drm_panel_attach_luminance_property() fails?
[ ... ]
> @@ -861,6 +891,61 @@ void drm_panel_disable(struct drm_panel *panel)
> }
> EXPORT_SYMBOL(drm_panel_disable);
>
> +/**
> + * drm_panel_attach_luminance_property - expose a panel backlight through DRM
> + * @panel: panel whose backlight is controlled
> + * @connector: connector on which to expose the property
> + *
> + * The backlight class device remains the internal hardware backend. When the
> + * luminance UAPI is enabled, userspace controls that backend through the DRM
> + * connector property instead.
> + *
> + * Return: 0 on success or a negative error code
> + */
> +int drm_panel_attach_luminance_property(struct drm_panel *panel,
> + struct drm_connector *connector)
> +{
> + struct backlight_device *backlight;
> +
> + if (!IS_ENABLED(CONFIG_DRM_LUMINANCE) || !panel)
> + return 0;
> +
> + backlight = panel->backlight;
> + if (!backlight)
> + return 0;
> +
> + return drm_connector_attach_luminance_property(connector, 0,
> + backlight->props.max_brightness,
> + backlight->props.brightness);
> +}
> +EXPORT_SYMBOL(drm_panel_attach_luminance_property);
> +
> +/**
> + * drm_panel_set_luminance - set a panel backlight's relative luminance
> + * @panel: panel whose backlight is controlled
> + * @luminance: relative luminance level
> + */
> +void drm_panel_set_luminance(struct drm_panel *panel, unsigned int luminance)
> +{
> + if (!panel || !panel->backlight)
> + return;
> +
> + panel->backlight->props.brightness = luminance;
> + backlight_update_status(panel->backlight);
> +}
> +EXPORT_SYMBOL(drm_panel_set_luminance);
> +
> +/**
> + * drm_panel_disable_luminance - turn off a panel backlight
> + * @panel: panel whose backlight is controlled
> + */
> +void drm_panel_disable_luminance(struct drm_panel *panel)
> +{
> + if (panel)
> + backlight_disable(panel->backlight);
> +}
> +EXPORT_SYMBOL(drm_panel_disable_luminance);
[Severity: High]
How do drm_panel_disable_luminance() and drm_panel_set_luminance() interact
with panel self-refresh and the backlight blank state?
In drm_panel_disable_luminance(), backlight_disable() sets:
bd->props.power = BACKLIGHT_POWER_OFF;
bd->props.state |= BL_CORE_FBBLANK;
whereas drm_panel_set_luminance() only updates props.brightness and calls
backlight_update_status() without clearing BL_CORE_FBBLANK or restoring
BACKLIGHT_POWER_ON.
In panel_bridge_atomic_disable() and panel_bridge_atomic_enable(), the bridge
checks self_refresh_active and returns early so that drm_panel_disable() and
drm_panel_enable() are skipped during self-refresh:
new_crtc_state = drm_atomic_get_new_crtc_state(atomic_state, crtc);
if (new_crtc_state && new_crtc_state->self_refresh_active)
return;
However, disable_outputs() still runs drm_atomic_helper_commit_luminance_disables()
when entering self-refresh, which calls panel_bridge_connector_disable_luminance()
and turns the backlight off via backlight_disable(). When exiting
self-refresh, panel_bridge_atomic_enable() skips drm_panel_enable() (so
backlight_enable() is never called), and drm_panel_set_luminance() leaves
BACKLIGHT_POWER_OFF and BL_CORE_FBBLANK set, keeping the backlight blanked.
Also, on a normal modeset disable, drm_atomic_helper_commit_luminance_disables()
runs before drm_atomic_helper_commit_encoder_bridge_disable(), which disables
the backlight before panel_disabling followers run in drm_panel_disable(), and
on modeset enable drm_panel_enable() calls backlight_enable() with the old
props.brightness before drm_panel_set_luminance() updates it.
Could drm_panel_set_luminance() unblank the backlight (or use
backlight_enable()), and should panel luminance disable/enable respect
self-refresh and panel sequencing?
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 58113ea7d63a0..76d49d4485ff0 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
[ ... ]
> @@ -418,40 +423,6 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
> struct drm_bridge *drmm_panel_bridge_add(struct drm_device *drm,
> struct drm_panel *panel);
> struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
> -#else
> -static inline bool drm_bridge_is_panel(const struct drm_bridge *bridge)
> -{
> - return false;
> -}
> -
> -static inline int drm_panel_bridge_set_orientation(struct drm_connector *connector,
> - struct drm_bridge *bridge)
> -{
> - return -EINVAL;
> -}
> -#endif
> -
> -#if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL_BRIDGE)
> -struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
> - u32 port, u32 endpoint);
> -struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, struct device_node *node,
> - u32 port, u32 endpoint);
> -#else
> -static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> - struct device_node *node,
> - u32 port,
> - u32 endpoint)
> -{
> - return ERR_PTR(-ENODEV);
> -}
> -
> -static inline struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
> - struct device_node *node,
> - u32 port,
> - u32 endpoint)
> -{
> - return ERR_PTR(-ENODEV);
> -}
> #endif
[Severity: Low]
Was this removal of the fallback stubs and the devm_drm_of_get_bridge() /
drmm_of_get_bridge() declarations in include/drm/drm_panel.h an unintended
rebase artifact against commit aa8a8c7e4641 ("drm/bridge: panel: move all code
to drm_panel.c")?
It looks like the preceding patch in the series re-added these blocks to
include/drm/drm_bridge.h, and this hunk removes only the #else half and the
of_get_bridge block from include/drm/drm_panel.h while leaving the #ifdef
CONFIG_DRM_PANEL_BRIDGE declarations duplicated in both headers.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=8
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 09/15] drm/bridge_connector: add luminance support
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (7 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 08/15] drm/panel: add connector luminance forwarding Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 16:49 ` [PATCH v10 10/15] drm/nouveau: add connector " Mario Limonciello (AMD)
` (6 subsequent siblings)
15 siblings, 0 replies; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Discover the last bridge in a chain that owns brightness control, attach
its relative luminance range to the connector, and forward atomic
luminance updates to it.
Do not infer panel luminance support from a terminal panel bridge. Panels
still have direct consumers, so their connector integration must remain
explicit.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v10:
* Split bridge connector integration into a separate patch
* Do not special-case panels in the generic bridge connector
---
drivers/gpu/drm/display/drm_bridge_connector.c | 48 ++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 81f3e26f6fdf5..bf0a6d162a7d3 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -125,6 +125,12 @@ struct drm_bridge_connector {
*/
struct drm_bridge *bridge_hdmi_cec;
+ /**
+ * @bridge_luminance:
+ *
+ * The last bridge in the chain that controls panel luminance.
+ */
+ struct drm_bridge *bridge_luminance;
/**
* @hdmi_funcs:
*
@@ -421,6 +427,28 @@ static int drm_bridge_connector_atomic_check(struct drm_connector *connector,
return 0;
}
+static void
+drm_bridge_connector_set_luminance(const struct drm_connector_state *conn_state)
+{
+ struct drm_bridge_connector *bridge_connector =
+ to_drm_bridge_connector(conn_state->connector);
+ struct drm_bridge *bridge = bridge_connector->bridge_luminance;
+
+ if (bridge)
+ bridge->funcs->set_luminance(bridge, conn_state->luminance);
+}
+
+static void
+drm_bridge_connector_disable_luminance(const struct drm_connector_state *conn_state)
+{
+ struct drm_bridge_connector *bridge_connector =
+ to_drm_bridge_connector(conn_state->connector);
+ struct drm_bridge *bridge = bridge_connector->bridge_luminance;
+
+ if (bridge)
+ bridge->funcs->set_luminance(bridge, 0);
+}
+
static const struct drm_connector_helper_funcs drm_bridge_connector_helper_funcs = {
.get_modes = drm_bridge_connector_get_modes,
.detect_ctx = drm_bridge_connector_detect_ctx,
@@ -429,6 +457,8 @@ static const struct drm_connector_helper_funcs drm_bridge_connector_helper_funcs
.enable_hpd = drm_bridge_connector_enable_hpd,
.disable_hpd = drm_bridge_connector_disable_hpd,
.atomic_check = drm_bridge_connector_atomic_check,
+ .set_luminance = drm_bridge_connector_set_luminance,
+ .disable_luminance = drm_bridge_connector_disable_luminance,
};
static enum drm_mode_status
@@ -835,6 +865,7 @@ static void drm_bridge_connector_put_bridges(struct drm_device *dev, void *data)
drm_bridge_put(bridge_connector->bridge_hdmi_audio);
drm_bridge_put(bridge_connector->bridge_dp_audio);
drm_bridge_put(bridge_connector->bridge_hdmi_cec);
+ drm_bridge_put(bridge_connector->bridge_luminance);
}
/**
@@ -918,6 +949,13 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
drm_bridge_put(bridge_connector->bridge_detect);
bridge_connector->bridge_detect = drm_bridge_get(bridge);
}
+ if (bridge->ops & DRM_BRIDGE_OP_LUMINANCE) {
+ if (!bridge->funcs->set_luminance)
+ return ERR_PTR(-EINVAL);
+
+ drm_bridge_put(bridge_connector->bridge_luminance);
+ bridge_connector->bridge_luminance = drm_bridge_get(bridge);
+ }
if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
if (bridge_connector->bridge_hdmi)
return ERR_PTR(-EBUSY);
@@ -1144,6 +1182,16 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
if (panel_bridge)
drm_panel_bridge_set_orientation(connector, panel_bridge);
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE) && bridge_connector->bridge_luminance && !connector->luminance_property) {
+ struct drm_bridge *bridge = bridge_connector->bridge_luminance;
+
+ ret = drm_connector_attach_luminance_property(connector, 0,
+ bridge->luminance_max,
+ bridge->luminance);
+ if (ret)
+ return ERR_PTR(ret);
+ }
+
if (support_hdcp && IS_REACHABLE(CONFIG_DRM_DISPLAY_HELPER) &&
IS_ENABLED(CONFIG_DRM_DISPLAY_HDCP_HELPER))
drm_connector_attach_content_protection_property(connector, true);
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v10 10/15] drm/nouveau: add connector luminance support
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (8 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 09/15] drm/bridge_connector: add luminance support Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 18:49 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 11/15] drm/gud: " Mario Limonciello (AMD)
` (5 subsequent siblings)
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Expose Nouveau panel brightness through the connector LUMINANCE property
when DRM_LUMINANCE is enabled. Reuse the existing NV40, NV50, and DPCD
hardware paths while preserving the backlight class interface otherwise.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/nouveau/Kconfig | 4 +-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_backlight.c | 110 ++++++++++++++++++++++------
drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +
drivers/gpu/drm/nouveau/nouveau_connector.h | 14 ++++
5 files changed, 108 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 3b5757aed9c84..2dcb7c5344f48 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -17,13 +17,13 @@ config DRM_NOUVEAU
select DRM_SCHED
select I2C
select I2C_ALGOBIT
- select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHT
+ select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHT && !DRM_LUMINANCE
select X86_PLATFORM_DEVICES if ACPI && X86
select ACPI_WMI if ACPI && X86
select MXM_WMI if ACPI && X86
select POWER_SUPPLY
# Similar to i915, we need to select ACPI_VIDEO and it's dependencies
- select BACKLIGHT_CLASS_DEVICE if ACPI && X86
+ select BACKLIGHT_CLASS_DEVICE if ACPI && X86 && !DRM_LUMINANCE
select INPUT if ACPI && X86
select THERMAL if ACPI && X86
select ACPI_VIDEO if ACPI && X86
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index e91130f93db67..79be7900a0794 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1838,7 +1838,7 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_commit *st
backlight = nv_connector->backlight;
if (backlight && backlight->uses_dpcd)
drm_edp_backlight_enable(&nv_connector->aux, &backlight->edp_info,
- backlight->dev->props.brightness);
+ backlight->brightness);
#endif
break;
diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index da1ee24a2531d..fefcfa8c2a3a9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -73,12 +73,10 @@ nv40_get_intensity(struct backlight_device *bd)
}
static int
-nv40_set_intensity(struct backlight_device *bd)
+nv40_set_intensity_value(struct nouveau_encoder *nv_encoder, int val)
{
- struct nouveau_encoder *nv_encoder = bl_get_data(bd);
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);
struct nvif_object *device = &drm->client.device.object;
- int val = bd->props.brightness;
int reg = nvif_rd32(device, NV40_PMC_BACKLIGHT);
nvif_wr32(device, NV40_PMC_BACKLIGHT,
@@ -87,6 +85,12 @@ nv40_set_intensity(struct backlight_device *bd)
return 0;
}
+static int
+nv40_set_intensity(struct backlight_device *bd)
+{
+ return nv40_set_intensity_value(bl_get_data(bd), bd->props.brightness);
+}
+
static const struct backlight_ops nv40_bl_ops = {
.options = BL_CORE_SUSPENDRESUME,
.get_brightness = nv40_get_intensity,
@@ -105,6 +109,8 @@ nv40_backlight_init(struct nouveau_encoder *encoder,
return -ENODEV;
props->max_brightness = 31;
+ props->brightness = (nvif_rd32(device, NV40_PMC_BACKLIGHT) &
+ NV40_PMC_BACKLIGHT_MASK) >> 16;
*ops = &nv40_bl_ops;
return 0;
}
@@ -234,12 +240,15 @@ nv50_backlight_init(struct nouveau_backlight *bl,
const struct backlight_ops **ops)
{
struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev);
+ int brightness;
+
+ brightness = nvif_outp_bl_get(&nv_encoder->outp);
/*
* Note when this runs the connectors have not been probed yet,
* so nv_conn->base.status is not set yet.
*/
- if (nvif_outp_bl_get(&nv_encoder->outp) < 0 ||
+ if (brightness < 0 ||
drm_helper_probe_detect(&nv_conn->base, NULL, false) != connector_status_connected)
return -ENODEV;
@@ -284,6 +293,7 @@ nv50_backlight_init(struct nouveau_backlight *bl,
*ops = &nv50_bl_ops;
props->max_brightness = 100;
+ props->brightness = brightness;
return 0;
}
@@ -317,10 +327,13 @@ nouveau_backlight_init(struct drm_connector *connector)
bl = kzalloc_obj(*bl);
if (!bl)
return -ENOMEM;
+ bl->id = -1;
+ bl->encoder = nv_encoder;
switch (device->info.family) {
case NV_DEVICE_INFO_V0_CURIE:
ret = nv40_backlight_init(nv_encoder, &props, &ops);
+ bl->uses_nv40 = true;
break;
case NV_DEVICE_INFO_V0_TESLA:
case NV_DEVICE_INFO_V0_FERMI:
@@ -349,26 +362,39 @@ nouveau_backlight_init(struct drm_connector *connector)
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;
- }
-
props.type = BACKLIGHT_RAW;
- bl->dev = backlight_device_register(backlight_name, connector->kdev,
- nv_encoder, ops, &props);
- if (IS_ERR(bl->dev)) {
- if (bl->id >= 0)
- ida_free(&bl_ida, bl->id);
- ret = PTR_ERR(bl->dev);
- goto fail_alloc;
+ bl->brightness = props.brightness;
+ bl->max_brightness = props.max_brightness;
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ ret = drm_connector_attach_luminance_property(connector, 0, props.max_brightness,
+ props.brightness);
+ if (ret)
+ goto fail_alloc;
+ } else {
+ 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;
+ }
+
+ bl->dev = backlight_device_register(backlight_name, connector->kdev,
+ nv_encoder, ops, &props);
+ if (IS_ERR(bl->dev)) {
+ if (bl->id >= 0)
+ ida_free(&bl_ida, bl->id);
+ ret = PTR_ERR(bl->dev);
+ goto fail_alloc;
+ }
}
nouveau_connector(connector)->backlight = bl;
- if (!bl->dev->props.brightness)
- bl->dev->props.brightness =
- bl->dev->ops->get_brightness(bl->dev);
- backlight_update_status(bl->dev);
+ if (bl->dev) {
+ if (!bl->dev->props.brightness)
+ bl->dev->props.brightness =
+ bl->dev->ops->get_brightness(bl->dev);
+ bl->brightness = bl->dev->props.brightness;
+ backlight_update_status(bl->dev);
+ }
return 0;
@@ -396,7 +422,9 @@ nouveau_backlight_fini(struct drm_connector *connector)
if (bl->id >= 0)
ida_free(&bl_ida, bl->id);
- backlight_device_unregister(bl->dev);
+ if (!IS_ENABLED(CONFIG_DRM_LUMINANCE))
+ backlight_device_unregister(bl->dev);
+
nv_conn->backlight = NULL;
kfree(bl);
}
@@ -412,3 +440,43 @@ nouveau_backlight_dtor(void)
{
ida_destroy(&bl_ida);
}
+
+void
+nouveau_backlight_set_luminance(const struct drm_connector_state *conn_state)
+{
+ struct nouveau_connector *nv_connector = nouveau_connector(conn_state->connector);
+ struct nouveau_backlight *bl = nv_connector->backlight;
+ unsigned int luminance = conn_state->luminance;
+
+ if (!bl || !bl->max_brightness)
+ return;
+
+ bl->brightness = luminance;
+ if (bl->uses_dpcd) {
+ drm_edp_backlight_set_level(&nv_connector->aux, &bl->edp_info,
+ luminance);
+ return;
+ }
+
+ if (bl->uses_nv40)
+ nv40_set_intensity_value(bl->encoder, luminance);
+ else
+ nvif_outp_bl_set(&bl->encoder->outp, luminance);
+}
+
+void
+nouveau_backlight_disable_luminance(const struct drm_connector_state *conn_state)
+{
+ struct nouveau_connector *nv_connector = nouveau_connector(conn_state->connector);
+ struct nouveau_backlight *bl = nv_connector->backlight;
+
+ if (!bl)
+ return;
+
+ if (bl->uses_dpcd)
+ drm_edp_backlight_disable(&nv_connector->aux, &bl->edp_info);
+ else if (bl->uses_nv40)
+ nv40_set_intensity_value(bl->encoder, 0);
+ else
+ nvif_outp_bl_set(&bl->encoder->outp, 0);
+}
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index cc3caf8de64b0..56ee579ea7909 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -1225,6 +1225,8 @@ nouveau_connector_helper_funcs = {
.mode_valid = nouveau_connector_mode_valid,
.best_encoder = nouveau_connector_best_encoder,
.atomic_check = nouveau_connector_atomic_check,
+ .set_luminance = nouveau_backlight_set_luminance,
+ .disable_luminance = nouveau_backlight_disable_luminance,
};
static const struct drm_connector_funcs
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h b/drivers/gpu/drm/nouveau/nouveau_connector.h
index eb292d2ba4bcd..0cee3e4c1d11c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.h
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.h
@@ -50,9 +50,13 @@ struct nouveau_backlight {
struct backlight_device *dev;
struct drm_edp_backlight_info edp_info;
+ struct nouveau_encoder *encoder;
bool uses_dpcd : 1;
+ bool uses_nv40 : 1;
int id;
+ int brightness;
+ int max_brightness;
};
#endif
@@ -239,6 +243,8 @@ extern int nouveau_backlight_init(struct drm_connector *);
extern void nouveau_backlight_fini(struct drm_connector *);
extern void nouveau_backlight_ctor(void);
extern void nouveau_backlight_dtor(void);
+void nouveau_backlight_set_luminance(const struct drm_connector_state *conn_state);
+void nouveau_backlight_disable_luminance(const struct drm_connector_state *conn_state);
#else
static inline int
nouveau_backlight_init(struct drm_connector *connector)
@@ -257,6 +263,14 @@ nouveau_backlight_ctor(void) {
static inline void
nouveau_backlight_dtor(void) {
}
+
+static inline void
+nouveau_backlight_set_luminance(const struct drm_connector_state *conn_state) {
+}
+
+static inline void
+nouveau_backlight_disable_luminance(const struct drm_connector_state *conn_state) {
+}
#endif
#endif /* __NOUVEAU_CONNECTOR_H__ */
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 10/15] drm/nouveau: add connector luminance support
2026-11-02 16:49 ` [PATCH v10 10/15] drm/nouveau: add connector " Mario Limonciello (AMD)
@ 2026-11-02 18:49 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 18:49 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/nouveau: missing luminance commit calls in nv50_disp_atomic_commit_tail()
- [High] drm/nouveau: WARN_ON attaching luminance prop in nouveau_backlight_init()
- [High] drm/nouveau: atomic luminance property unusable on non-atomic NV40 KMS
- [Medium] drm/nouveau: stale brightness in nv50_sor_atomic_enable() on eDP modeset
- [Low] drm/nouveau: unmet BACKLIGHT_CLASS_DEVICE dependency for ACPI_VIDEO
--
commit 5d2c149e206a77b8a0aa35e6607301d35db8ef5b
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/nouveau: add connector luminance support
This commit exposes Nouveau panel brightness through the connector LUMINANCE
property when CONFIG_DRM_LUMINANCE is enabled, reusing the NV40, NV50, and
DPCD backlight hardware paths while keeping the backlight class device
interface when CONFIG_DRM_LUMINANCE is disabled.
> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
> index 3b5757aed9c84..2dcb7c5344f48 100644
> --- a/drivers/gpu/drm/nouveau/Kconfig
> +++ b/drivers/gpu/drm/nouveau/Kconfig
> @@ -17,13 +17,13 @@ config DRM_NOUVEAU
> select DRM_SCHED
> select I2C
> select I2C_ALGOBIT
> - select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHT
> + select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHT && !DRM_LUMINANCE
> select X86_PLATFORM_DEVICES if ACPI && X86
> select ACPI_WMI if ACPI && X86
> select MXM_WMI if ACPI && X86
> select POWER_SUPPLY
> # Similar to i915, we need to select ACPI_VIDEO and it's dependencies
> - select BACKLIGHT_CLASS_DEVICE if ACPI && X86
> + select BACKLIGHT_CLASS_DEVICE if ACPI && X86 && !DRM_LUMINANCE
> select INPUT if ACPI && X86
> select THERMAL if ACPI && X86
> select ACPI_VIDEO if ACPI && X86
[Severity: Low]
Does conditioning BACKLIGHT_CLASS_DEVICE on !DRM_LUMINANCE cause an unmet
Kconfig dependency for ACPI_VIDEO when CONFIG_DRM_LUMINANCE is enabled and
nothing else selects BACKLIGHT_CLASS_DEVICE?
In drivers/acpi/Kconfig, ACPI_VIDEO depends on BACKLIGHT_CLASS_DEVICE:
config ACPI_VIDEO
tristate "Video"
depends on BACKLIGHT_CLASS_DEVICE
Because DRM_NOUVEAU still unconditionally selects ACPI_VIDEO if ACPI && X86
(and nouveau_backlight_init() still falls back to
nouveau_acpi_video_register_backlight() under fail_alloc), enabling
CONFIG_DRM_LUMINANCE with CONFIG_BACKLIGHT_CLASS_DEVICE disabled produces an
unmet direct dependency warning for ACPI_VIDEO and link failures in
drivers/acpi/acpi_video.o for backlight_device_register() and
backlight_device_unregister().
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index e91130f93db67..79be7900a0794 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1838,7 +1838,7 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_commit *st
> backlight = nv_connector->backlight;
> if (backlight && backlight->uses_dpcd)
> drm_edp_backlight_enable(&nv_connector->aux, &backlight->edp_info,
> - backlight->dev->props.brightness);
> + backlight->brightness);
[Severity: Medium]
When CONFIG_DRM_LUMINANCE is disabled, won't passing backlight->brightness
here in nv50_sor_atomic_enable() reset an eDP panel's brightness to its
initial boot-time value on subsequent modesets, DPMS wakeups, or resume?
backlight->brightness is initialized once in nouveau_backlight_init(),
whereas user brightness updates through /sys/class/backlight call
nv50_edp_set_brightness() in drivers/gpu/drm/nouveau/nouveau_backlight.c:
static int
nv50_edp_set_brightness(struct backlight_device *bd)
{
...
if (crtc->state->active)
ret = drm_edp_backlight_set_level(aux, &nv_bl->edp_info, bd->props.brightness);
...
}
Because nv50_edp_set_brightness() never updates nv_bl->brightness from
bd->props.brightness, nv50_sor_atomic_enable() will pass the stale initial
backlight->brightness value to drm_edp_backlight_enable().
> diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> index da1ee24a2531d..fefcfa8c2a3a9 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
[ ... ]
> @@ -317,10 +327,13 @@ nouveau_backlight_init(struct drm_connector *connector)
> bl = kzalloc_obj(*bl);
> if (!bl)
> return -ENOMEM;
> + bl->id = -1;
> + bl->encoder = nv_encoder;
>
> switch (device->info.family) {
> case NV_DEVICE_INFO_V0_CURIE:
> ret = nv40_backlight_init(nv_encoder, &props, &ops);
> + bl->uses_nv40 = true;
> break;
[Severity: High]
Can the LUMINANCE property work on NV40 (NV_DEVICE_INFO_V0_CURIE) or when
Nouveau is booted with atomic modesetting disabled (nouveau.atomic=0)?
NV40 uses the legacy dispnv04 display engine, where nouveau_drm_device_new()
selects driver_legacy_kms without DRIVER_ATOMIC and nv04_display_create() in
drivers/gpu/drm/nouveau/dispnv04/disp.c explicitly clears DRIVER_ATOMIC:
/* Pre-nv50 doesn't support atomic, so don't expose the ioctls */
dev->driver_features &= ~DRIVER_ATOMIC;
However, drm_connector_attach_luminance_property() creates the LUMINANCE
property with DRM_MODE_PROP_ATOMIC. On non-atomic drivers, atomic client
capability is rejected, DRM_MODE_PROP_ATOMIC properties are hidden from
drm_mode_object_get_properties(), and legacy property updates via
nouveau_connector_set_property() -> nouveau_conn_atomic_set_property() reject
the property with -EINVAL without calling nouveau_backlight_set_luminance().
Should nouveau_backlight_init() also check
drm_drv_uses_atomic_modeset(connector->dev) before using the LUMINANCE
property instead of registering the backlight class device?
> case NV_DEVICE_INFO_V0_TESLA:
> case NV_DEVICE_INFO_V0_FERMI:
> @@ -349,26 +362,39 @@ nouveau_backlight_init(struct drm_connector *connector)
> 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;
> - }
> -
> props.type = BACKLIGHT_RAW;
> - bl->dev = backlight_device_register(backlight_name, connector->kdev,
> - nv_encoder, ops, &props);
> - if (IS_ERR(bl->dev)) {
> - if (bl->id >= 0)
> - ida_free(&bl_ida, bl->id);
> - ret = PTR_ERR(bl->dev);
> - goto fail_alloc;
> + bl->brightness = props.brightness;
> + bl->max_brightness = props.max_brightness;
> + if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> + ret = drm_connector_attach_luminance_property(connector, 0, props.max_brightness,
> + props.brightness);
> + if (ret)
> + goto fail_alloc;
[Severity: High]
Won't calling drm_connector_attach_luminance_property() here in
nouveau_backlight_init() trigger a WARN_ON in __drm_mode_object_add() on
every internal panel registration?
nouveau_backlight_init() is called from nouveau_connector_late_register(),
which runs during drm_dev_register() -> drm_modeset_register_all() ->
drm_connector_register() after drm_dev_register() has already registered the
DRM minor device nodes and set dev->registered = true:
nouveau_connector_late_register()
-> nouveau_backlight_init()
-> drm_connector_attach_luminance_property()
-> drm_property_create_range()
-> drm_property_create()
-> drm_mode_object_add()
-> __drm_mode_object_add()
In drivers/gpu/drm/drm_mode_object.c:__drm_mode_object_add():
WARN_ON(!dev->driver->load && dev->registered && !obj_free_cb);
Because DRM_MODE_OBJECT_PROPERTY objects have no obj_free_cb and
dev->registered is already true in late_register, this triggers the WARN_ON
and mutates dev->mode_config.property_list, connector->base.properties, and
connector->state->luminance without locking after the DRM device has been
exposed to userspace.
[ ... ]
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index cc3caf8de64b0..56ee579ea7909 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -1225,6 +1225,8 @@ nouveau_connector_helper_funcs = {
> .mode_valid = nouveau_connector_mode_valid,
> .best_encoder = nouveau_connector_best_encoder,
> .atomic_check = nouveau_connector_atomic_check,
> + .set_luminance = nouveau_backlight_set_luminance,
> + .disable_luminance = nouveau_backlight_disable_luminance,
> };
[Severity: High]
Are nouveau_backlight_set_luminance() and
nouveau_backlight_disable_luminance() ever invoked during atomic commits in
Nouveau?
In drivers/gpu/drm/drm_atomic_helper.c, the .disable_luminance and
.set_luminance connector helper callbacks are called from
drm_atomic_helper_commit_luminance_disables() and
drm_atomic_helper_commit_luminance_enables() (or via
drm_atomic_helper_commit_modeset_disables() and
drm_atomic_helper_commit_modeset_enables()).
However, Nouveau's NV50+ display driver in
drivers/gpu/drm/nouveau/dispnv50/disp.c implements its own atomic commit path
in nv50_disp_atomic_commit() -> nv50_disp_atomic_commit_tail(), which
disables and enables heads, planes, and outputs directly without calling
drm_atomic_helper_commit_modeset_disables(),
drm_atomic_helper_commit_modeset_enables(),
drm_atomic_helper_commit_luminance_disables(), or
drm_atomic_helper_commit_luminance_enables().
Without calling those helpers in nv50_disp_atomic_commit_tail(), won't
updates to the LUMINANCE connector property be silently ignored by the
hardware?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=10
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 11/15] drm/gud: add connector luminance support
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (9 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 10/15] drm/nouveau: add connector " Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 19:02 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 12/15] drm/radeon: " Mario Limonciello (AMD)
` (4 subsequent siblings)
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Expose GUD display brightness through the connector LUMINANCE property
when DRM_LUMINANCE is enabled. Keep brightness state in the connector and
retain the asynchronous USB update path.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/gud/Kconfig | 2 +-
drivers/gpu/drm/gud/gud_connector.c | 48 +++++++++++++++++++++++++++++++++++--
2 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/gud/Kconfig b/drivers/gpu/drm/gud/Kconfig
index b4d2136942f0e..6c250d3d5e203 100644
--- a/drivers/gpu/drm/gud/Kconfig
+++ b/drivers/gpu/drm/gud/Kconfig
@@ -7,7 +7,7 @@ config DRM_GUD
select DRM_CLIENT_SELECTION
select DRM_KMS_HELPER
select DRM_GEM_SHMEM_HELPER
- select BACKLIGHT_CLASS_DEVICE
+ select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
help
This is a DRM display driver for GUD USB Displays or display
adapters.
diff --git a/drivers/gpu/drm/gud/gud_connector.c b/drivers/gpu/drm/gud/gud_connector.c
index 8141c3a1e30ad..bd7688676e9e1 100644
--- a/drivers/gpu/drm/gud/gud_connector.c
+++ b/drivers/gpu/drm/gud/gud_connector.c
@@ -16,6 +16,7 @@
#include <drm/drm_modeset_helper_vtables.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
+
#include <drm/gud.h>
#include "gud_internal.h"
@@ -25,6 +26,7 @@ struct gud_connector {
struct drm_encoder encoder;
struct backlight_device *backlight;
struct work_struct backlight_work;
+ unsigned int brightness;
/* Supported properties */
u16 *properties;
@@ -84,7 +86,7 @@ static void gud_connector_backlight_update_status_work(struct work_struct *work)
}
/* Reuse tv.brightness to avoid having to subclass */
- connector_state->tv.brightness = gconn->backlight->props.brightness;
+ connector_state->tv.brightness = READ_ONCE(gconn->brightness);
ret = drm_atomic_commit(state);
out:
@@ -110,6 +112,8 @@ static int gud_connector_backlight_update_status(struct backlight_device *bd)
struct drm_connector *connector = bl_get_data(bd);
struct gud_connector *gconn = to_gud_connector(connector);
+ WRITE_ONCE(gconn->brightness, bd->props.brightness);
+
/* The USB timeout is 5 seconds so use system_long_wq for worst case scenario */
queue_work(system_long_wq, &gconn->backlight_work);
@@ -132,6 +136,19 @@ static int gud_connector_backlight_register(struct gud_connector *gconn)
.brightness = gconn->initial_brightness,
};
+ gconn->brightness = props.brightness;
+
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ int ret;
+
+ ret = drm_connector_attach_luminance_property(connector, 0, props.max_brightness,
+ props.brightness);
+ if (ret)
+ return ret;
+
+ return 0;
+ }
+
name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
connector->dev->primary->index, connector->name);
if (!name)
@@ -314,12 +331,37 @@ static int gud_connector_atomic_check(struct drm_connector *connector,
return 0;
}
+static void gud_connector_set_luminance(const struct drm_connector_state *conn_state);
+static void gud_connector_disable_luminance(const struct drm_connector_state *conn_state);
+
static const struct drm_connector_helper_funcs gud_connector_helper_funcs = {
.detect_ctx = gud_connector_detect,
.get_modes = gud_connector_get_modes,
.atomic_check = gud_connector_atomic_check,
+ .set_luminance = gud_connector_set_luminance,
+ .disable_luminance = gud_connector_disable_luminance,
};
+static void gud_connector_set_luminance(const struct drm_connector_state *conn_state)
+{
+ struct drm_connector *connector = conn_state->connector;
+ struct gud_connector *gconn = to_gud_connector(connector);
+ unsigned int luminance = conn_state->luminance;
+
+ WRITE_ONCE(gconn->brightness, luminance);
+
+ /* The workqueue will apply it to the hardware */
+ queue_work(system_long_wq, &gconn->backlight_work);
+}
+
+static void gud_connector_disable_luminance(const struct drm_connector_state *conn_state)
+{
+ struct gud_connector *gconn = to_gud_connector(conn_state->connector);
+
+ WRITE_ONCE(gconn->brightness, 0);
+ queue_work(system_long_wq, &gconn->backlight_work);
+}
+
static int gud_connector_late_register(struct drm_connector *connector)
{
struct gud_connector *gconn = to_gud_connector(connector);
@@ -334,7 +376,9 @@ static void gud_connector_early_unregister(struct drm_connector *connector)
{
struct gud_connector *gconn = to_gud_connector(connector);
- backlight_device_unregister(gconn->backlight);
+ if (gconn->backlight)
+ backlight_device_unregister(gconn->backlight);
+
cancel_work_sync(&gconn->backlight_work);
}
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 11/15] drm/gud: add connector luminance support
2026-11-02 16:49 ` [PATCH v10 11/15] drm/gud: " Mario Limonciello (AMD)
@ 2026-11-02 19:02 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 19:02 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] drm/gud: use-after-free on disconnect via gud_connector_disable_luminance()
- [High] drm/gud: infinite commit loop in gud_connector_set_luminance()
- [High] drm/gud: brightness clobbered to 0 in gud_connector_disable_luminance()
- [Medium] drm/gud: late property creation in gud_connector_backlight_register()
- [Low] drm/gud: undefined reference to backlight_device_unregister()
--
commit 58c61672659ddff3d1dca17855a27dcf7212a5b6
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/gud: add connector luminance support
This commit exposes GUD display brightness through the connector LUMINANCE
property when CONFIG_DRM_LUMINANCE is enabled, storing brightness state on
struct gud_connector and reusing the asynchronous backlight workqueue path.
> diff --git a/drivers/gpu/drm/gud/Kconfig b/drivers/gpu/drm/gud/Kconfig
> index b4d2136942f0e..6c250d3d5e203 100644
> --- a/drivers/gpu/drm/gud/Kconfig
> +++ b/drivers/gpu/drm/gud/Kconfig
> @@ -7,7 +7,7 @@ config DRM_GUD
> select DRM_CLIENT_SELECTION
> select DRM_KMS_HELPER
> select DRM_GEM_SHMEM_HELPER
> - select BACKLIGHT_CLASS_DEVICE
> + select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
> help
> This is a DRM display driver for GUD USB Displays or display
> adapters.
> diff --git a/drivers/gpu/drm/gud/gud_connector.c b/drivers/gpu/drm/gud/gud_connector.c
> index 8141c3a1e30ad..bd7688676e9e1 100644
> --- a/drivers/gpu/drm/gud/gud_connector.c
> +++ b/drivers/gpu/drm/gud/gud_connector.c
[ ... ]
> @@ -84,7 +86,7 @@ static void gud_connector_backlight_update_status_work(struct work_struct *work)
> }
>
> /* Reuse tv.brightness to avoid having to subclass */
> - connector_state->tv.brightness = gconn->backlight->props.brightness;
> + connector_state->tv.brightness = READ_ONCE(gconn->brightness);
>
> ret = drm_atomic_commit(state);
> out:
[ ... ]
> @@ -132,6 +136,19 @@ static int gud_connector_backlight_register(struct gud_connector *gconn)
> .brightness = gconn->initial_brightness,
> };
>
> + gconn->brightness = props.brightness;
> +
> + if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> + int ret;
> +
> + ret = drm_connector_attach_luminance_property(connector, 0, props.max_brightness,
> + props.brightness);
> + if (ret)
> + return ret;
> +
> + return 0;
> + }
[Severity: Medium]
Won't attaching the LUMINANCE property in gud_connector_backlight_register()
trigger a WARN_ON in __drm_mode_object_add() on every probe when
CONFIG_DRM_LUMINANCE is enabled?
During gud_probe(), drm_dev_register() registers the DRM minor nodes and sets
dev->registered = true before calling:
drm_modeset_register_all()
-> drm_connector_register_all()
-> drm_connector_register()
-> gud_connector_late_register()
-> gud_connector_backlight_register()
-> drm_connector_attach_luminance_property()
-> drm_property_create_range()
-> drm_property_create()
-> drm_mode_object_add()
-> __drm_mode_object_add()
In drivers/gpu/drm/drm_mode_object.c:__drm_mode_object_add():
WARN_ON(!dev->driver->load && dev->registered && !obj_free_cb);
Because gud_drm_driver.load is NULL, dev->registered is already true, and
obj_free_cb is NULL for DRM properties, this warning fires unconditionally and
mutates dev->mode_config.property_list, connector->base.properties, and
connector->state->luminance after userspace already has access to the DRM
device. Should drm_connector_attach_luminance_property() be called during
connector initialization in gud_connector_add_properties() before
drm_mode_config_reset() and drm_dev_register() instead?
> +
> name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
> connector->dev->primary->index, connector->name);
> if (!name)
> @@ -314,12 +331,37 @@ static int gud_connector_atomic_check(struct drm_connector *connector,
> return 0;
> }
>
> +static void gud_connector_set_luminance(const struct drm_connector_state *conn_state);
> +static void gud_connector_disable_luminance(const struct drm_connector_state *conn_state);
> +
> static const struct drm_connector_helper_funcs gud_connector_helper_funcs = {
> .detect_ctx = gud_connector_detect,
> .get_modes = gud_connector_get_modes,
> .atomic_check = gud_connector_atomic_check,
> + .set_luminance = gud_connector_set_luminance,
> + .disable_luminance = gud_connector_disable_luminance,
> };
>
> +static void gud_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct drm_connector *connector = conn_state->connector;
> + struct gud_connector *gconn = to_gud_connector(connector);
> + unsigned int luminance = conn_state->luminance;
> +
> + WRITE_ONCE(gconn->brightness, luminance);
> +
> + /* The workqueue will apply it to the hardware */
> + queue_work(system_long_wq, &gconn->backlight_work);
> +}
[Severity: High]
Does queueing gconn->backlight_work from gud_connector_set_luminance() cause
an infinite loop of atomic commits on system_long_wq whenever the connector is
active on a CRTC?
When gconn->backlight_work runs gud_connector_backlight_update_status_work(),
the workqueue core clears WORK_STRUCT_PENDING_BIT before invoking the work
callback. gud_connector_backlight_update_status_work() then adds the connector
to a new commit state via drm_atomic_get_connector_state(state, connector) and
calls drm_atomic_commit(state) synchronously:
gud_connector_backlight_update_status_work()
-> drm_atomic_commit()
-> drm_atomic_helper_commit_tail_rpm()
-> drm_atomic_helper_commit_modeset_enables()
-> drm_atomic_helper_commit_luminance_enables()
-> gud_connector_set_luminance()
In drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_commit_luminance_enables():
for_each_new_connector_in_state(state, connector, new_conn_state, i) {
const struct drm_connector_helper_funcs *funcs;
if (!connector->luminance_property || !new_conn_state->crtc ||
!new_conn_state->crtc->state->active)
continue;
funcs = connector->helper_private;
if (funcs && funcs->set_luminance)
funcs->set_luminance(new_conn_state);
}
Because the connector is in the commit state, connector->luminance_property is
non-NULL, and new_conn_state->crtc->state->active is true,
drm_atomic_helper_commit_luminance_enables() unconditionally invokes
gud_connector_set_luminance() again during the work item's own
drm_atomic_commit() call. That re-queues gconn->backlight_work while the work
function is still executing, so system_long_wq immediately runs
gud_connector_backlight_update_status_work() again in an unbounded loop.
In addition, when old_state->tv.brightness != new_state->tv.brightness,
gud_connector_atomic_check() sets new_crtc_state->connectors_changed = true,
causing drm_atomic_helper_commit_modeset_disables() to also call
drm_atomic_helper_commit_luminance_disables() ->
gud_connector_disable_luminance() before
drm_atomic_helper_commit_luminance_enables() runs.
> +
> +static void gud_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct gud_connector *gconn = to_gud_connector(conn_state->connector);
> +
> + WRITE_ONCE(gconn->brightness, 0);
> + queue_work(system_long_wq, &gconn->backlight_work);
> +}
[Severity: Critical]
Can gud_connector_disable_luminance() re-queue gconn->backlight_work after
gud_connector_early_unregister() has already canceled it, leading to a
use-after-free of struct gud_connector on USB disconnect?
In drivers/gpu/drm/gud/gud_drv.c:gud_disconnect():
drm_kms_helper_poll_fini(drm);
drm_dev_unplug(drm);
drm_atomic_helper_shutdown(drm);
First, drm_dev_unplug() runs:
drm_dev_unplug()
-> drm_dev_unregister()
-> drm_modeset_unregister_all()
-> drm_connector_unregister()
-> gud_connector_early_unregister()
-> cancel_work_sync(&gconn->backlight_work)
Next, drm_atomic_helper_shutdown() disables all active outputs:
drm_atomic_helper_shutdown()
-> drm_atomic_helper_disable_all()
-> drm_atomic_commit()
-> drm_atomic_helper_commit_modeset_disables()
-> disable_outputs()
-> drm_atomic_helper_commit_luminance_disables()
-> gud_connector_disable_luminance()
-> queue_work(system_long_wq, &gconn->backlight_work)
When gud_disconnect() finishes and devres releases the drm_device,
drm_mode_config_cleanup() calls gud_connector_destroy(), which cleans up the
connector (zeroing connector->dev) and calls kfree(gconn) without canceling
gconn->backlight_work. If gud_connector_backlight_update_status_work() is
still queued or running on system_long_wq, won't it access freed gconn memory
and dereference a NULL or freed connector->dev in drm_dev_enter()?
[Severity: High]
Also in gud_connector_disable_luminance(), doesn't setting gconn->brightness
to 0 and queueing gconn->backlight_work clobber connector_state->tv.brightness
and send an unpaired GUD_REQ_SET_STATE_CHECK over USB when a CRTC enters DPMS
off?
When a CRTC is set to DPMS off (new_crtc_state->enable == true,
new_crtc_state->active == false), drm_atomic_helper_commit_luminance_disables()
calls gud_connector_disable_luminance(), whereas
drm_atomic_helper_commit_luminance_enables() does not run because
new_crtc_state->active is false.
When gud_connector_backlight_update_status_work() subsequently runs on
system_long_wq, it sets connector_state->tv.brightness = 0 and commits:
1. gud_connector_atomic_check() sees old_state->tv.brightness !=
new_state->tv.brightness and sets new_crtc_state->connectors_changed = true.
2. Because crtc_state->enable is still true, gud_plane_atomic_check()
serializes connector_state->tv.brightness = 0 into GUD_REQ_SET_STATE_CHECK
and sends it to the USB device.
3. Because new_crtc_state->active is false,
drm_atomic_helper_commit_crtc_enable() skips gud_crtc_atomic_enable(),
leaving GUD_REQ_SET_STATE_CHECK unpaired with GUD_REQ_SET_STATE_COMMIT.
4. When a later commit re-enables the CRTC (active = true),
gud_plane_atomic_check() and gud_crtc_atomic_enable() run before
drm_atomic_helper_commit_luminance_enables(), so the display is first
enabled with tv.brightness == 0 before gud_connector_set_luminance() queues
a second full modeset commit to restore conn_state->luminance.
> +
> static int gud_connector_late_register(struct drm_connector *connector)
> {
> struct gud_connector *gconn = to_gud_connector(connector);
> @@ -334,7 +376,9 @@ static void gud_connector_early_unregister(struct drm_connector *connector)
> {
> struct gud_connector *gconn = to_gud_connector(connector);
>
> - backlight_device_unregister(gconn->backlight);
> + if (gconn->backlight)
> + backlight_device_unregister(gconn->backlight);
> +
> cancel_work_sync(&gconn->backlight_work);
> }
[Severity: Low]
Will gud_connector_early_unregister() fail to link when CONFIG_DRM_LUMINANCE=y
and CONFIG_BACKLIGHT_CLASS_DEVICE is disabled (or when CONFIG_DRM_GUD=y and
CONFIG_BACKLIGHT_CLASS_DEVICE=m)?
The Kconfig change updates DRM_GUD to:
select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
However, include/linux/backlight.h declares backlight_device_unregister() as
an external function without an inline stub when CONFIG_BACKLIGHT_CLASS_DEVICE
is not enabled. Because if (gconn->backlight) is a runtime check on a struct
member rather than !IS_ENABLED(CONFIG_DRM_LUMINANCE), the compiler cannot
eliminate the call to backlight_device_unregister() at compile time.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=11
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 12/15] drm/radeon: add connector luminance support
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (10 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 11/15] drm/gud: " Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 19:13 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 13/15] drm/gma500: " Mario Limonciello (AMD)
` (3 subsequent siblings)
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Expose Radeon panel brightness through the connector LUMINANCE property
when DRM_LUMINANCE is enabled. Reuse the AtomBIOS and legacy hardware
programming paths and preserve the backlight class interface otherwise.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/radeon/Kconfig | 2 +-
drivers/gpu/drm/radeon/atombios_encoders.c | 32 ++++++++++++++--
drivers/gpu/drm/radeon/radeon_connectors.c | 34 +++++++++++++++++
drivers/gpu/drm/radeon/radeon_legacy_encoders.c | 49 ++++++++++++++++++++-----
drivers/gpu/drm/radeon/radeon_mode.h | 2 +
5 files changed, 106 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/radeon/Kconfig b/drivers/gpu/drm/radeon/Kconfig
index 3cd70aef770e1..2ccad1f2f8cee 100644
--- a/drivers/gpu/drm/radeon/Kconfig
+++ b/drivers/gpu/drm/radeon/Kconfig
@@ -17,7 +17,7 @@ config DRM_RADEON
select SND_HDA_COMPONENT if SND_HDA_CORE
select POWER_SUPPLY
select HWMON
- select BACKLIGHT_CLASS_DEVICE
+ select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
select INTERVAL_TREE
select I2C
select I2C_ALGOBIT
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c
index 5b9c72bf40994..e6f762625bf1a 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -227,6 +227,27 @@ void radeon_atom_backlight_init(struct radeon_encoder *radeon_encoder,
memset(&props, 0, sizeof(props));
props.max_brightness = RADEON_MAX_BL_LEVEL;
props.type = BACKLIGHT_RAW;
+ pdata->encoder = radeon_encoder;
+ pdata->atom = true;
+ radeon_encoder->backlight_priv = pdata;
+
+ dig = radeon_encoder->enc_priv;
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ props.brightness = atombios_get_backlight_level(radeon_encoder);
+ if (!props.brightness)
+ props.brightness = RADEON_MAX_BL_LEVEL;
+
+ if (drm_connector_attach_luminance_property(drm_connector, 0,
+ props.max_brightness,
+ props.brightness))
+ goto error;
+
+ atombios_set_backlight_level(radeon_encoder, props.brightness);
+ dig->bl_dev = NULL;
+ rdev->mode_info.bl_encoder = radeon_encoder;
+ return;
+ }
+
snprintf(bl_name, sizeof(bl_name),
"radeon_bl%d", dev->primary->index);
bd = backlight_device_register(bl_name, drm_connector->kdev,
@@ -236,9 +257,6 @@ void radeon_atom_backlight_init(struct radeon_encoder *radeon_encoder,
goto error;
}
- pdata->encoder = radeon_encoder;
-
- dig = radeon_encoder->enc_priv;
dig->bl_dev = bd;
bd->props.brightness = radeon_atom_backlight_get_brightness(bd);
@@ -258,6 +276,7 @@ void radeon_atom_backlight_init(struct radeon_encoder *radeon_encoder,
return;
error:
+ radeon_encoder->backlight_priv = NULL;
kfree(pdata);
return;
}
@@ -282,12 +301,19 @@ static void radeon_atom_backlight_exit(struct radeon_encoder *radeon_encoder)
bd = dig->bl_dev;
dig->bl_dev = NULL;
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ kfree(radeon_encoder->backlight_priv);
+ radeon_encoder->backlight_priv = NULL;
+ return;
+ }
+
if (bd) {
struct radeon_legacy_backlight_privdata *pdata;
pdata = bl_get_data(bd);
backlight_device_unregister(bd);
kfree(pdata);
+ radeon_encoder->backlight_priv = NULL;
DRM_INFO("radeon atom LVDS backlight unloaded\n");
}
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index 16b048f58c9be..655361407b819 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -31,6 +31,7 @@
#include <drm/drm_probe_helper.h>
#include <drm/radeon_drm.h>
#include "radeon.h"
+#include "radeon_asic.h"
#include "radeon_audio.h"
#include "atom.h"
@@ -931,11 +932,44 @@ static int radeon_lvds_set_property(struct drm_connector *connector,
return 0;
}
+static void radeon_set_luminance(const struct drm_connector_state *conn_state)
+{
+ struct drm_encoder *encoder = radeon_best_single_encoder(conn_state->connector);
+ struct radeon_encoder *radeon_encoder;
+ struct radeon_backlight_privdata *pdata;
+ u8 level = conn_state->luminance;
+
+ if (!encoder)
+ return;
+
+ radeon_encoder = to_radeon_encoder(encoder);
+ pdata = radeon_encoder->backlight_priv;
+ if (!pdata)
+ return;
+
+ if (pdata->atom) {
+ atombios_set_backlight_level(radeon_encoder, level);
+ } else {
+ if (pdata->negative)
+ level = RADEON_MAX_BL_LEVEL - level;
+ radeon_legacy_set_backlight_level(radeon_encoder, level);
+ }
+}
+
+static void radeon_disable_luminance(const struct drm_connector_state *conn_state)
+{
+ struct drm_connector_state state = *conn_state;
+
+ state.luminance = 0;
+ radeon_set_luminance(&state);
+}
static const struct drm_connector_helper_funcs radeon_lvds_connector_helper_funcs = {
.get_modes = radeon_lvds_get_modes,
.mode_valid = radeon_lvds_mode_valid,
.best_encoder = radeon_best_single_encoder,
+ .set_luminance = radeon_set_luminance,
+ .disable_luminance = radeon_disable_luminance,
};
static const struct drm_connector_funcs radeon_lvds_connector_funcs = {
diff --git a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
index 1f82e2b57f5d5..96b0dfd8e3521 100644
--- a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
+++ b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
@@ -403,16 +403,9 @@ void radeon_legacy_backlight_init(struct radeon_encoder *radeon_encoder,
memset(&props, 0, sizeof(props));
props.max_brightness = RADEON_MAX_BL_LEVEL;
props.type = BACKLIGHT_RAW;
- snprintf(bl_name, sizeof(bl_name),
- "radeon_bl%d", dev->primary->index);
- bd = backlight_device_register(bl_name, drm_connector->kdev,
- pdata, &radeon_backlight_ops, &props);
- if (IS_ERR(bd)) {
- DRM_ERROR("Backlight registration failed\n");
- goto error;
- }
-
pdata->encoder = radeon_encoder;
+ pdata->atom = false;
+ radeon_encoder->backlight_priv = pdata;
backlight_level = (RREG32(RADEON_LVDS_GEN_CNTL) >>
RADEON_LVDS_BL_MOD_LEVEL_SHIFT) & 0xff;
@@ -441,6 +434,36 @@ void radeon_legacy_backlight_init(struct radeon_encoder *radeon_encoder,
#endif
}
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ props.brightness = pdata->negative ?
+ RADEON_MAX_BL_LEVEL - backlight_level : backlight_level;
+ if (drm_connector_attach_luminance_property(drm_connector, 0,
+ props.max_brightness,
+ props.brightness))
+ goto error;
+
+ if (rdev->is_atom_bios) {
+ struct radeon_encoder_atom_dig *lvds = radeon_encoder->enc_priv;
+
+ lvds->bl_dev = NULL;
+ } else {
+ struct radeon_encoder_lvds *lvds = radeon_encoder->enc_priv;
+
+ lvds->bl_dev = NULL;
+ }
+ rdev->mode_info.bl_encoder = radeon_encoder;
+ return;
+ }
+
+ snprintf(bl_name, sizeof(bl_name),
+ "radeon_bl%d", dev->primary->index);
+ bd = backlight_device_register(bl_name, drm_connector->kdev,
+ pdata, &radeon_backlight_ops, &props);
+ if (IS_ERR(bd)) {
+ DRM_ERROR("Backlight registration failed\n");
+ goto error;
+ }
+
if (rdev->is_atom_bios) {
struct radeon_encoder_atom_dig *lvds = radeon_encoder->enc_priv;
lvds->bl_dev = bd;
@@ -459,6 +482,7 @@ void radeon_legacy_backlight_init(struct radeon_encoder *radeon_encoder,
return;
error:
+ radeon_encoder->backlight_priv = NULL;
kfree(pdata);
return;
}
@@ -482,12 +506,19 @@ static void radeon_legacy_backlight_exit(struct radeon_encoder *radeon_encoder)
lvds->bl_dev = NULL;
}
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ kfree(radeon_encoder->backlight_priv);
+ radeon_encoder->backlight_priv = NULL;
+ return;
+ }
+
if (bd) {
struct radeon_backlight_privdata *pdata;
pdata = bl_get_data(bd);
backlight_device_unregister(bd);
kfree(pdata);
+ radeon_encoder->backlight_priv = NULL;
DRM_INFO("radeon legacy LVDS backlight unloaded\n");
}
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
index 6a16d821915d2..1c5b9ed0b6989 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -284,6 +284,7 @@ struct radeon_mode_info {
struct radeon_backlight_privdata {
struct radeon_encoder *encoder;
uint8_t negative;
+ bool atom;
};
#define MAX_H_CODE_TIMING_LEN 32
@@ -456,6 +457,7 @@ struct radeon_encoder {
uint32_t underscan_vborder;
struct drm_display_mode native_mode;
void *enc_priv;
+ struct radeon_backlight_privdata *backlight_priv;
int audio_polling_active;
bool is_ext_encoder;
u16 caps;
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 12/15] drm/radeon: add connector luminance support
2026-11-02 16:49 ` [PATCH v10 12/15] drm/radeon: " Mario Limonciello (AMD)
@ 2026-11-02 19:13 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 19:13 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm/radeon: unreachable atomic LUMINANCE property on non-atomic KMS driver
- [High] drm/radeon: legacy LVDS backlight ignored and panel turned off in radeon_legacy_lvds_update()
- [High] drm/radeon: missing luminance callbacks in radeon_dp_connector_helper_funcs
- [Medium] drm/radeon: Kconfig unmet dependency and link error with DRM_LUMINANCE
- [Medium] drm/radeon: ACPI ATIF brightness events ignored when dig->bl_dev is NULL
- [Medium] drm/radeon: memory leak of backlight_priv when LCD encoder links to multiple connectors
--
commit d872f715df68798d7e323e8f154d8558d421e434
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/radeon: add connector luminance support
This commit adds support for exposing Radeon panel brightness through the
connector LUMINANCE property when CONFIG_DRM_LUMINANCE is enabled, reusing
the AtomBIOS and legacy hardware programming paths while keeping the
backlight class interface otherwise.
> diff --git a/drivers/gpu/drm/radeon/Kconfig b/drivers/gpu/drm/radeon/Kconfig
> index 3cd70aef770e1..2ccad1f2f8cee 100644
> --- a/drivers/gpu/drm/radeon/Kconfig
> +++ b/drivers/gpu/drm/radeon/Kconfig
> @@ -17,7 +17,7 @@ config DRM_RADEON
> select SND_HDA_COMPONENT if SND_HDA_CORE
> select POWER_SUPPLY
> select HWMON
> - select BACKLIGHT_CLASS_DEVICE
> + select BACKLIGHT_CLASS_DEVICE if !DRM_LUMINANCE
> select INTERVAL_TREE
> select I2C
> select I2C_ALGOBIT
[Severity: Medium]
Can making BACKLIGHT_CLASS_DEVICE conditional on !DRM_LUMINANCE cause an
unmet Kconfig dependency and linker failures when CONFIG_DRM_LUMINANCE=y,
CONFIG_ACPI=y, and CONFIG_BACKLIGHT_CLASS_DEVICE is disabled (or modular
with built-in DRM_RADEON)?
Just below in drivers/gpu/drm/radeon/Kconfig, DRM_RADEON still
unconditionally selects ACPI_VIDEO:
# radeon depends on ACPI_VIDEO when ACPI is enabled, for select to work
# ACPI_VIDEO's dependencies must also be selected.
select INPUT if ACPI
select ACPI_VIDEO if ACPI
In drivers/acpi/Kconfig, ACPI_VIDEO depends on BACKLIGHT_CLASS_DEVICE:
config ACPI_VIDEO
tristate "Video"
depends on BACKLIGHT_CLASS_DEVICE
In addition, radeon_atif_handler() in drivers/gpu/drm/radeon/radeon_acpi.c
unconditionally calls backlight_force_update(), which has no inline stub in
include/linux/backlight.h when CONFIG_BACKLIGHT_CLASS_DEVICE is disabled:
if (rdev->is_atom_bios) {
struct radeon_encoder_atom_dig *dig = enc->enc_priv;
backlight_force_update(dig->bl_dev,
BACKLIGHT_UPDATE_HOTKEY);
} else {
struct radeon_encoder_lvds *dig = enc->enc_priv;
backlight_force_update(dig->bl_dev,
BACKLIGHT_UPDATE_HOTKEY);
}
> diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c
> index 5b9c72bf40994..e6f762625bf1a 100644
> --- a/drivers/gpu/drm/radeon/atombios_encoders.c
> +++ b/drivers/gpu/drm/radeon/atombios_encoders.c
> @@ -227,6 +227,27 @@ void radeon_atom_backlight_init(struct radeon_encoder *radeon_encoder,
> memset(&props, 0, sizeof(props));
> props.max_brightness = RADEON_MAX_BL_LEVEL;
> props.type = BACKLIGHT_RAW;
> + pdata->encoder = radeon_encoder;
> + pdata->atom = true;
> + radeon_encoder->backlight_priv = pdata;
[Severity: Medium]
Can this leak the struct radeon_backlight_privdata allocation in pdata
when an encoder with ATOM_DEVICE_LCD_SUPPORT is shared across multiple
connectors?
In radeon_add_atom_encoder() and radeon_add_legacy_encoder(), encoders
sharing the same encoder_enum combine their device bitmasks via
radeon_encoder->devices |= supported_device. When
radeon_link_encoder_connector() in drivers/gpu/drm/radeon/radeon_encoders.c
links encoders and connectors, it checks radeon_encoder->devices rather
than radeon_connector->devices:
if (radeon_encoder->devices & radeon_connector->devices) {
drm_connector_attach_encoder(connector, encoder);
if (radeon_encoder->devices & (ATOM_DEVICE_LCD_SUPPORT))
radeon_encoder_add_backlight(radeon_encoder, connector);
}
When radeon_encoder_add_backlight() is invoked a second time for the same
radeon_encoder (both here in radeon_atom_backlight_init() and in
radeon_legacy_backlight_init()), radeon_encoder->backlight_priv is
overwritten with the newly allocated pdata. With CONFIG_DRM_LUMINANCE
enabled, drm_connector_attach_luminance_property() succeeds on the second
connector and leaks the first pdata allocation (or if registration fails
and jumps to error:, radeon_encoder->backlight_priv is cleared to NULL
without freeing or preserving the first allocation).
> +
> + dig = radeon_encoder->enc_priv;
> + if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> + props.brightness = atombios_get_backlight_level(radeon_encoder);
> + if (!props.brightness)
> + props.brightness = RADEON_MAX_BL_LEVEL;
> +
> + if (drm_connector_attach_luminance_property(drm_connector, 0,
> + props.max_brightness,
> + props.brightness))
> + goto error;
> +
> + atombios_set_backlight_level(radeon_encoder, props.brightness);
> + dig->bl_dev = NULL;
> + rdev->mode_info.bl_encoder = radeon_encoder;
> + return;
> + }
[Severity: High]
How can userspace access or update the LUMINANCE property on radeon when
radeon is a non-atomic KMS driver?
In drivers/gpu/drm/radeon/radeon_drv.c, kms_driver does not advertise
DRIVER_ATOMIC:
.driver_features =
DRIVER_GEM | DRIVER_RENDER | DRIVER_MODESET,
However, drm_connector_attach_luminance_property() creates the LUMINANCE
property with DRM_MODE_PROP_ATOMIC:
property = drm_property_create_range(connector->dev, DRM_MODE_PROP_ATOMIC,
"LUMINANCE", min, max);
Because DRIVER_ATOMIC is not set:
1. drm_setclientcap() rejects DRM_CLIENT_CAP_ATOMIC, so
drm_mode_object_get_properties() hides DRM_MODE_PROP_ATOMIC properties
from userspace.
2. Legacy property updates go through drm_mode_obj_set_property_ioctl() ->
set_property_legacy() -> radeon_lvds_set_property(), which ignores
everything except scaling_mode_property.
3. radeon_set_luminance() and radeon_disable_luminance() are only invoked
by drm_atomic_helper_commit_luminance_enables() and
drm_atomic_helper_commit_luminance_disables(), which radeon never calls.
Since radeon_atom_backlight_init() and radeon_legacy_backlight_init()
return early before backlight_device_register() when CONFIG_DRM_LUMINANCE
is enabled (and set rdev->mode_info.bl_encoder, which also prevents the
acpi_video_register_backlight() fallback in radeon_encoder_add_backlight()),
won't this leave panels with no working brightness control interface?
[Severity: Medium]
Also, when dig->bl_dev is set to NULL here (and in
radeon_legacy_backlight_init()), won't ACPI ATIF panel brightness events
stop working?
In drivers/gpu/drm/radeon/radeon_acpi.c, radeon_acpi_init() locates
atif->encoder_for_bl by checking dig->bl_dev:
if (rdev->is_atom_bios) {
struct radeon_encoder_atom_dig *dig = enc->enc_priv;
if (dig->bl_dev) {
target = enc;
break;
}
} else {
struct radeon_encoder_lvds *dig = enc->enc_priv;
if (dig->bl_dev) {
target = enc;
break;
}
}
When dig->bl_dev is NULL, atif->encoder_for_bl remains NULL and
radeon_atif_handler() ignores ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST events.
[ ... ]
> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> index 16b048f58c9be..655361407b819 100644
> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
[ ... ]
> @@ -931,11 +932,44 @@ static int radeon_lvds_set_property(struct drm_connector *connector,
> return 0;
> }
>
> +static void radeon_set_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct drm_encoder *encoder = radeon_best_single_encoder(conn_state->connector);
> + struct radeon_encoder *radeon_encoder;
> + struct radeon_backlight_privdata *pdata;
> + u8 level = conn_state->luminance;
> +
> + if (!encoder)
> + return;
> +
> + radeon_encoder = to_radeon_encoder(encoder);
> + pdata = radeon_encoder->backlight_priv;
> + if (!pdata)
> + return;
> +
> + if (pdata->atom) {
> + atombios_set_backlight_level(radeon_encoder, level);
> + } else {
> + if (pdata->negative)
> + level = RADEON_MAX_BL_LEVEL - level;
> + radeon_legacy_set_backlight_level(radeon_encoder, level);
> + }
> +}
> +
> +static void radeon_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct drm_connector_state state = *conn_state;
> +
> + state.luminance = 0;
> + radeon_set_luminance(&state);
> +}
>
> static const struct drm_connector_helper_funcs radeon_lvds_connector_helper_funcs = {
> .get_modes = radeon_lvds_get_modes,
> .mode_valid = radeon_lvds_mode_valid,
> .best_encoder = radeon_best_single_encoder,
> + .set_luminance = radeon_set_luminance,
> + .disable_luminance = radeon_disable_luminance,
> };
[Severity: High]
Are .set_luminance and .disable_luminance also needed in
radeon_dp_connector_helper_funcs?
In radeon_add_atom_connector(), native DRM_MODE_CONNECTOR_eDP connectors
and DP-bridge (TRAVIS/NUTMEG) DRM_MODE_CONNECTOR_LVDS/eDP connectors use
radeon_dp_connector_helper_funcs:
drm_connector_init_with_ddc(dev, &radeon_connector->base,
&radeon_edp_connector_funcs,
connector_type,
ddc);
drm_connector_helper_add(&radeon_connector->base, &radeon_dp_connector_helper_funcs);
Their encoders have ATOM_DEVICE_LCD_SUPPORT set, so
radeon_link_encoder_connector() -> radeon_encoder_add_backlight() ->
radeon_atom_backlight_init() attaches the LUMINANCE property to those
connectors, but radeon_dp_connector_helper_funcs does not wire up
.set_luminance or .disable_luminance (and on DP-bridge connectors,
radeon_dvi_encoder() rather than radeon_best_single_encoder() selects the
encoder with enc_priv).
> diff --git a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
> index 1f82e2b57f5d5..96b0dfd8e3521 100644
> --- a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
> +++ b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
[ ... ]
> @@ -441,6 +434,36 @@ void radeon_legacy_backlight_init(struct radeon_encoder *radeon_encoder,
> #endif
> }
>
> + if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
> + props.brightness = pdata->negative ?
> + RADEON_MAX_BL_LEVEL - backlight_level : backlight_level;
> + if (drm_connector_attach_luminance_property(drm_connector, 0,
> + props.max_brightness,
> + props.brightness))
> + goto error;
> +
> + if (rdev->is_atom_bios) {
> + struct radeon_encoder_atom_dig *lvds = radeon_encoder->enc_priv;
> +
> + lvds->bl_dev = NULL;
> + } else {
> + struct radeon_encoder_lvds *lvds = radeon_encoder->enc_priv;
> +
> + lvds->bl_dev = NULL;
> + }
> + rdev->mode_info.bl_encoder = radeon_encoder;
> + return;
> + }
[Severity: High]
When lvds->bl_dev is set to NULL here without initializing
lvds->backlight_level, won't radeon_legacy_lvds_update() ignore brightness
updates and radeon_legacy_set_backlight_level() power off the panel?
In radeon_legacy_lvds_update(), lvds->backlight_level is only used if
lvds->bl_dev is non-NULL:
if (radeon_encoder->enc_priv) {
if (rdev->is_atom_bios) {
struct radeon_encoder_atom_dig *lvds = radeon_encoder->enc_priv;
panel_pwr_delay = lvds->panel_pwr_delay;
if (lvds->bl_dev)
backlight_level = lvds->backlight_level;
} else {
struct radeon_encoder_lvds *lvds = radeon_encoder->enc_priv;
panel_pwr_delay = lvds->panel_pwr_delay;
if (lvds->bl_dev)
backlight_level = lvds->backlight_level;
}
}
Furthermore, radeon_legacy_set_backlight_level() checks the previous value
of lvds->backlight_level before updating it:
if (radeon_encoder->enc_priv) {
if (rdev->is_atom_bios) {
struct radeon_encoder_atom_dig *lvds = radeon_encoder->enc_priv;
if (lvds->backlight_level > 0)
dpms_mode = lvds->dpms_mode;
else
dpms_mode = DRM_MODE_DPMS_OFF;
lvds->backlight_level = level;
} else {
struct radeon_encoder_lvds *lvds = radeon_encoder->enc_priv;
if (lvds->backlight_level > 0)
dpms_mode = lvds->dpms_mode;
else
dpms_mode = DRM_MODE_DPMS_OFF;
lvds->backlight_level = level;
}
}
radeon_legacy_lvds_update(&radeon_encoder->base, dpms_mode);
Because lvds->backlight_level starts at 0 (or becomes 0 when maximum
brightness is set on a pdata->negative panel where
level = RADEON_MAX_BL_LEVEL - level), radeon_legacy_set_backlight_level()
will pass DRM_MODE_DPMS_OFF to radeon_legacy_lvds_update().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=12
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 13/15] drm/gma500: add connector luminance support
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (11 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 12/15] drm/radeon: " Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 19:22 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 14/15] drm/bridge: add PS8622 " Mario Limonciello (AMD)
` (2 subsequent siblings)
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Expose GMA500 panel brightness through the connector LUMINANCE property
when DRM_LUMINANCE is enabled. Route updates through the existing platform
backlight operations and preserve legacy registration otherwise.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/gma500/Kconfig | 2 +-
drivers/gpu/drm/gma500/backlight.c | 45 +++++++++++++++++++++------------
drivers/gpu/drm/gma500/cdv_intel_lvds.c | 2 ++
drivers/gpu/drm/gma500/psb_drv.c | 2 +-
drivers/gpu/drm/gma500/psb_drv.h | 4 ++-
drivers/gpu/drm/gma500/psb_intel_lvds.c | 3 ++-
6 files changed, 38 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
index dc4c566388192..08871a8f6fa22 100644
--- a/drivers/gpu/drm/gma500/Kconfig
+++ b/drivers/gpu/drm/gma500/Kconfig
@@ -9,7 +9,7 @@ config DRM_GMA500
select I2C_ALGOBIT
# GMA500 depends on ACPI_VIDEO when ACPI is enabled, just like i915
select ACPI_VIDEO if ACPI
- select BACKLIGHT_CLASS_DEVICE if ACPI
+ select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE
select INPUT if ACPI
select ACPI_WMI if ACPI
help
diff --git a/drivers/gpu/drm/gma500/backlight.c b/drivers/gpu/drm/gma500/backlight.c
index c8f1716a12d52..4bb39fba04539 100644
--- a/drivers/gpu/drm/gma500/backlight.c
+++ b/drivers/gpu/drm/gma500/backlight.c
@@ -8,7 +8,6 @@
*/
#include <linux/backlight.h>
-
#include <acpi/video.h>
#include <drm/drm_print.h>
@@ -73,10 +72,9 @@ static const struct backlight_ops gma_backlight_ops __maybe_unused = {
.update_status = gma_backlight_update_status,
};
-int gma_backlight_init(struct drm_device *dev)
+int gma_backlight_init(struct drm_device *dev, struct drm_connector *connector)
{
struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
- struct backlight_properties props __maybe_unused = {};
int ret;
dev_priv->backlight_enabled = true;
@@ -92,28 +90,43 @@ int gma_backlight_init(struct drm_device *dev)
return 0;
}
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
- props.brightness = dev_priv->backlight_level;
- props.max_brightness = PSB_MAX_BRIGHTNESS;
- props.type = BACKLIGHT_RAW;
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE))
+ return drm_connector_attach_luminance_property(connector, 0,
+ PSB_MAX_BRIGHTNESS,
+ dev_priv->backlight_level);
+
+ {
+ struct backlight_properties props = {};
- dev_priv->backlight_device =
- backlight_device_register(dev_priv->ops->backlight_name,
- dev->dev, dev,
- &gma_backlight_ops, &props);
- if (IS_ERR(dev_priv->backlight_device))
- return PTR_ERR(dev_priv->backlight_device);
-#endif
+ props.brightness = dev_priv->backlight_level;
+ props.max_brightness = PSB_MAX_BRIGHTNESS;
+ props.type = BACKLIGHT_RAW;
+
+ dev_priv->backlight_device =
+ backlight_device_register(dev_priv->ops->backlight_name,
+ dev->dev, dev,
+ &gma_backlight_ops, &props);
+ if (IS_ERR(dev_priv->backlight_device))
+ return PTR_ERR(dev_priv->backlight_device);
+ }
return 0;
}
+void gma_connector_set_luminance(const struct drm_connector_state *conn_state)
+{
+ gma_backlight_set(conn_state->connector->dev, conn_state->luminance);
+}
+
+void gma_connector_disable_luminance(const struct drm_connector_state *conn_state)
+{
+ gma_backlight_disable(conn_state->connector->dev);
+}
+
void gma_backlight_exit(struct drm_device *dev)
{
-#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
if (dev_priv->backlight_device)
backlight_device_unregister(dev_priv->backlight_device);
-#endif
}
diff --git a/drivers/gpu/drm/gma500/cdv_intel_lvds.c b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
index 336ab411d6995..cef97f1532358 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_lvds.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
@@ -412,6 +412,8 @@ static const struct drm_connector_helper_funcs
.get_modes = cdv_intel_lvds_get_modes,
.mode_valid = cdv_intel_lvds_mode_valid,
.best_encoder = gma_best_encoder,
+ .set_luminance = gma_connector_set_luminance,
+ .disable_luminance = gma_connector_disable_luminance,
};
static const struct drm_connector_funcs cdv_intel_lvds_connector_funcs = {
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index d17cb5b4a4bf2..5de7af651ac39 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -396,7 +396,7 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
if (gma_encoder->type == INTEL_OUTPUT_LVDS ||
gma_encoder->type == INTEL_OUTPUT_MIPI) {
- ret = gma_backlight_init(dev);
+ ret = gma_backlight_init(dev, connector);
if (ret == 0)
acpi_video_register_backlight();
break;
diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h
index db197b865b90a..4fc22c66e6c8f 100644
--- a/drivers/gpu/drm/gma500/psb_drv.h
+++ b/drivers/gpu/drm/gma500/psb_drv.h
@@ -604,11 +604,13 @@ int psb_fbdev_driver_fbdev_probe(struct drm_fb_helper *fb_helper,
#endif
/* backlight.c */
-int gma_backlight_init(struct drm_device *dev);
+int gma_backlight_init(struct drm_device *dev, struct drm_connector *connector);
void gma_backlight_exit(struct drm_device *dev);
void gma_backlight_disable(struct drm_device *dev);
void gma_backlight_enable(struct drm_device *dev);
void gma_backlight_set(struct drm_device *dev, int v);
+void gma_connector_set_luminance(const struct drm_connector_state *conn_state);
+void gma_connector_disable_luminance(const struct drm_connector_state *conn_state);
/* oaktrail_crtc.c */
extern const struct drm_crtc_helper_funcs oaktrail_helper_funcs;
diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c b/drivers/gpu/drm/gma500/psb_intel_lvds.c
index cd882786d9461..fa0bc8492ff7f 100644
--- a/drivers/gpu/drm/gma500/psb_intel_lvds.c
+++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c
@@ -610,6 +610,8 @@ const struct drm_connector_helper_funcs
.get_modes = psb_intel_lvds_get_modes,
.mode_valid = psb_intel_lvds_mode_valid,
.best_encoder = gma_best_encoder,
+ .set_luminance = gma_connector_set_luminance,
+ .disable_luminance = gma_connector_disable_luminance,
};
const struct drm_connector_funcs psb_intel_lvds_connector_funcs = {
@@ -808,4 +810,3 @@ void psb_intel_lvds_init(struct drm_device *dev,
err_free_encoder:
kfree(gma_encoder);
}
-
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 13/15] drm/gma500: add connector luminance support
2026-11-02 16:49 ` [PATCH v10 13/15] drm/gma500: " Mario Limonciello (AMD)
@ 2026-11-02 19:22 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 19:22 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/gma500: unreachable atomic luminance support in gma_backlight_init()
- [High] drm/gma500: gma_connector_disable_luminance() permanently disables backlight
- [Medium] drm/gma500: Kconfig unmet dependency and backlight link errors in backlight.c
--
commit 44c343eaada226f65e1091fb750905e2745ab610
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/gma500: add connector luminance support
This commit exposes panel brightness on gma500 through the connector
luminance property when drm luminance support is enabled, routing updates
through existing platform backlight operations.
> diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
> index dc4c566388192..08871a8f6fa22 100644
> --- a/drivers/gpu/drm/gma500/Kconfig
> +++ b/drivers/gpu/drm/gma500/Kconfig
> @@ -9,7 +9,7 @@ config DRM_GMA500
> select I2C_ALGOBIT
> # GMA500 depends on ACPI_VIDEO when ACPI is enabled, just like i915
> select ACPI_VIDEO if ACPI
> - select BACKLIGHT_CLASS_DEVICE if ACPI
> + select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE
> select INPUT if ACPI
> select ACPI_WMI if ACPI
> help
> diff --git a/drivers/gpu/drm/gma500/backlight.c b/drivers/gpu/drm/gma500/backlight.c
> index c8f1716a12d52..4bb39fba04539 100644
> --- a/drivers/gpu/drm/gma500/backlight.c
> +++ b/drivers/gpu/drm/gma500/backlight.c
[ ... ]
> @@ -73,10 +72,9 @@ static const struct backlight_ops gma_backlight_ops __maybe_unused = {
> .update_status = gma_backlight_update_status,
> };
>
> -int gma_backlight_init(struct drm_device *dev)
> +int gma_backlight_init(struct drm_device *dev, struct drm_connector *connector)
> {
> struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> - struct backlight_properties props __maybe_unused = {};
> int ret;
>
> dev_priv->backlight_enabled = true;
> @@ -92,28 +90,43 @@ int gma_backlight_init(struct drm_device *dev)
> return 0;
> }
>
> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
> - props.brightness = dev_priv->backlight_level;
> - props.max_brightness = PSB_MAX_BRIGHTNESS;
> - props.type = BACKLIGHT_RAW;
> + if (IS_ENABLED(CONFIG_DRM_LUMINANCE))
> + return drm_connector_attach_luminance_property(connector, 0,
> + PSB_MAX_BRIGHTNESS,
> + dev_priv->backlight_level);
[Severity: High]
Does attaching the luminance property and returning early in
psb_driver_load() -> gma_backlight_init() leave panel brightness completely
uncontrollable when drm luminance is enabled?
In drivers/gpu/drm/gma500/psb_drv.c, the driver does not support atomic
modesetting:
static const struct drm_driver driver = {
.driver_features = DRIVER_MODESET | DRIVER_GEM,
However, drm_connector_attach_luminance_property() creates the property with
the atomic flag:
property = drm_property_create_range(connector->dev, DRM_MODE_PROP_ATOMIC,
"LUMINANCE", min, max);
Because gma500 does not advertise atomic support, drm_setclientcap() rejects
atomic client capability requests and file_priv->atomic is always false, so
drm_mode_object_get_properties() hides the property from userspace.
In addition, legacy property updates via drm_mode_obj_set_property_ioctl() ->
set_property_legacy() dispatch to psb_intel_lvds_set_property() or
cdv_intel_lvds_set_property(), neither of which handles the luminance
property, and the atomic commit helpers that invoke
gma_connector_set_luminance() and gma_connector_disable_luminance() are never
called by this driver.
Since gma_backlight_init() also skips backlight_device_register() on this
path, is there any way for userspace to adjust panel brightness when drm
luminance is enabled?
> +
> + {
> + struct backlight_properties props = {};
>
> - dev_priv->backlight_device =
> - backlight_device_register(dev_priv->ops->backlight_name,
> - dev->dev, dev,
> - &gma_backlight_ops, &props);
> - if (IS_ERR(dev_priv->backlight_device))
> - return PTR_ERR(dev_priv->backlight_device);
> -#endif
> + props.brightness = dev_priv->backlight_level;
> + props.max_brightness = PSB_MAX_BRIGHTNESS;
> + props.type = BACKLIGHT_RAW;
> +
> + dev_priv->backlight_device =
> + backlight_device_register(dev_priv->ops->backlight_name,
> + dev->dev, dev,
> + &gma_backlight_ops, &props);
> + if (IS_ERR(dev_priv->backlight_device))
> + return PTR_ERR(dev_priv->backlight_device);
> + }
>
> return 0;
> }
>
> +void gma_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> + gma_backlight_set(conn_state->connector->dev, conn_state->luminance);
> +}
> +
> +void gma_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> + gma_backlight_disable(conn_state->connector->dev);
> +}
[Severity: High]
Once gma_connector_disable_luminance() has been called, will subsequent calls
to gma_connector_set_luminance() fail to turn the backlight back on?
In drivers/gpu/drm/gma500/backlight.c, gma_connector_disable_luminance()
calls gma_backlight_disable(), which clears dev_priv->backlight_enabled:
void gma_backlight_disable(struct drm_device *dev)
{
struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
dev_priv->backlight_enabled = false;
dev_priv->ops->backlight_set(dev, 0);
}
When gma_connector_set_luminance() is later called to restore or change the
brightness, it calls gma_backlight_set(), which only updates the hardware if
dev_priv->backlight_enabled is true:
void gma_backlight_set(struct drm_device *dev, int v)
{
struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
dev_priv->backlight_level = v;
if (dev_priv->backlight_enabled)
dev_priv->ops->backlight_set(dev, v);
}
Since nothing in gma_connector_set_luminance() or the encoder enable paths
sets dev_priv->backlight_enabled back to true (or calls
gma_backlight_enable()), won't the panel backlight remain permanently off
after the first disable?
> +
> void gma_backlight_exit(struct drm_device *dev)
> {
> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
> struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>
> if (dev_priv->backlight_device)
> backlight_device_unregister(dev_priv->backlight_device);
> -#endif
> }
[Severity: Medium]
Can removing the preprocessor guards in gma_backlight_init() and
gma_backlight_exit() while dropping the backlight class selection in
drivers/gpu/drm/gma500/Kconfig cause unmet dependency warnings and link
failures?
First, in drivers/gpu/drm/gma500/Kconfig:
select ACPI_VIDEO if ACPI
select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE
Because acpi video in drivers/acpi/Kconfig depends on the backlight class
device, enabling acpi and drm luminance while the backlight class device is
disabled selects acpi video without its direct dependency.
Second, include/linux/backlight.h does not provide inline stubs for
backlight_device_register() or backlight_device_unregister() when the
backlight class device is disabled.
In gma_backlight_exit(), the call to backlight_device_unregister() is only
guarded by a runtime check on dev_priv->backlight_device, so the compiler
still emits an undefined reference to backlight_device_unregister() when the
backlight class device is disabled.
Similarly, if acpi and drm luminance are both disabled (or if gma500 is
built-in while the backlight class device is built as a module),
gma_backlight_init() and gma_backlight_exit() will fail to link against both
backlight_device_register() and backlight_device_unregister().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=13
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 14/15] drm/bridge: add PS8622 connector luminance support
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (12 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 13/15] drm/gma500: " Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 19:31 ` sashiko-bot
2026-11-02 16:49 ` [PATCH v10 15/15] drm/solomon: add SSD130x " Mario Limonciello (AMD)
2026-11-02 18:52 ` ✗ Fi.CI.BUILD: failure for Add support for a DRM backlight capability (rev5) Patchwork
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Expose PS8622 brightness through bridge luminance operations when
DRM_LUMINANCE is enabled. Keep registering the backlight class device
otherwise.
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/gpu/drm/bridge/Kconfig | 2 +-
drivers/gpu/drm/bridge/parade-ps8622.c | 45 +++++++++++++++++++++++++---------
2 files changed, 34 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index c7da6b02d7b6b..d27e9b2485fd6 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -278,7 +278,7 @@ config DRM_PARADE_PS8622
depends on OF
select DRM_PANEL_BRIDGE
select DRM_KMS_HELPER
- select BACKLIGHT_CLASS_DEVICE
+ depends on DRM_LUMINANCE || BACKLIGHT_CLASS_DEVICE = y || !BACKLIGHT_CLASS_DEVICE
help
Parade eDP-LVDS bridge chip driver.
diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
index 3a2ec482037df..229d9d7218c4e 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -45,6 +45,7 @@ struct ps8622_bridge {
struct drm_bridge *panel_bridge;
struct regulator *v12;
struct backlight_device *bl;
+ u8 brightness;
struct gpio_desc *gpio_slp;
struct gpio_desc *gpio_rst;
@@ -276,7 +277,7 @@ static int ps8622_send_config(struct ps8622_bridge *ps8622)
/* FFh for 100% brightness, 0h for 0% brightness */
err = ps8622_set(cl, 0x01, 0xa7,
- ps8622->bl->props.brightness);
+ ps8622->brightness);
if (err)
goto error;
} else {
@@ -324,6 +325,8 @@ static int ps8622_backlight_update(struct backlight_device *bl)
struct ps8622_bridge *ps8622 = dev_get_drvdata(&bl->dev);
int ret, brightness = backlight_get_brightness(bl);
+ ps8622->brightness = brightness;
+
if (!ps8622->enabled)
return -EINVAL;
@@ -332,6 +335,15 @@ static int ps8622_backlight_update(struct backlight_device *bl)
return ret;
}
+static void ps8622_set_luminance(struct drm_bridge *bridge,
+ unsigned int luminance)
+{
+ struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
+
+ ps8622->brightness = luminance;
+ ps8622_set(ps8622->client, 0x01, 0xa7, luminance);
+}
+
static const struct backlight_ops ps8622_backlight_ops = {
.update_status = ps8622_backlight_update,
};
@@ -438,6 +450,7 @@ static const struct drm_bridge_funcs ps8622_bridge_funcs = {
.atomic_disable = ps8622_disable,
.atomic_post_disable = ps8622_post_disable,
.attach = ps8622_attach,
+ .set_luminance = ps8622_set_luminance,
};
static const struct of_device_id ps8622_devices[] = {
@@ -502,18 +515,25 @@ static int ps8622_probe(struct i2c_client *client)
ps8622->lane_count = ps8622->max_lane_count;
}
+ ps8622->brightness = PS8622_MAX_BRIGHTNESS;
if (!of_property_read_bool(dev->of_node, "use-external-pwm")) {
- ps8622->bl = backlight_device_register("ps8622-backlight",
- dev, ps8622, &ps8622_backlight_ops,
- NULL);
- if (IS_ERR(ps8622->bl)) {
- DRM_ERROR("failed to register backlight\n");
- ret = PTR_ERR(ps8622->bl);
- ps8622->bl = NULL;
- return ret;
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ ps8622->bridge.ops |= DRM_BRIDGE_OP_LUMINANCE;
+ ps8622->bridge.luminance_max = PS8622_MAX_BRIGHTNESS;
+ ps8622->bridge.luminance = ps8622->brightness;
+ } else {
+ ps8622->bl = backlight_device_register("ps8622-backlight",
+ dev, ps8622,
+ &ps8622_backlight_ops, NULL);
+ if (IS_ERR(ps8622->bl)) {
+ DRM_ERROR("failed to register backlight\n");
+ ret = PTR_ERR(ps8622->bl);
+ ps8622->bl = NULL;
+ return ret;
+ }
+ ps8622->bl->props.max_brightness = PS8622_MAX_BRIGHTNESS;
+ ps8622->bl->props.brightness = ps8622->brightness;
}
- ps8622->bl->props.max_brightness = PS8622_MAX_BRIGHTNESS;
- ps8622->bl->props.brightness = PS8622_MAX_BRIGHTNESS;
}
ps8622->bridge.type = DRM_MODE_CONNECTOR_LVDS;
@@ -529,7 +549,8 @@ static void ps8622_remove(struct i2c_client *client)
{
struct ps8622_bridge *ps8622 = i2c_get_clientdata(client);
- backlight_device_unregister(ps8622->bl);
+ if (ps8622->bl)
+ backlight_device_unregister(ps8622->bl);
drm_bridge_remove(&ps8622->bridge);
}
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 14/15] drm/bridge: add PS8622 connector luminance support
2026-11-02 16:49 ` [PATCH v10 14/15] drm/bridge: add PS8622 " Mario Limonciello (AMD)
@ 2026-11-02 19:31 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 19:31 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/bridge: ps8622: ps8622_send_config() checks bl instead of luminance ops
- [Medium] drm/bridge: ps8622: Kconfig and remove() break link without backlight class
- [Medium] drm/bridge: ps8622: ps8622_set_luminance() writes I2C when bridge disabled
--
commit 1f704624e2ea918bbeb4f431e2e749044116d3f0
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/bridge: add PS8622 connector luminance support
This commit adds bridge luminance support to the Parade PS8622 driver when
CONFIG_DRM_LUMINANCE is enabled, while retaining the backlight class device
registration otherwise.
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index c7da6b02d7b6b..d27e9b2485fd6 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -278,7 +278,7 @@ config DRM_PARADE_PS8622
> depends on OF
> select DRM_PANEL_BRIDGE
> select DRM_KMS_HELPER
> - select BACKLIGHT_CLASS_DEVICE
> + depends on DRM_LUMINANCE || BACKLIGHT_CLASS_DEVICE = y || !BACKLIGHT_CLASS_DEVICE
> help
> Parade eDP-LVDS bridge chip driver.
[Severity: Medium]
Can this Kconfig dependency cause linker failures when
CONFIG_BACKLIGHT_CLASS_DEVICE is disabled or built as a module?
When CONFIG_DRM_LUMINANCE=n and CONFIG_BACKLIGHT_CLASS_DEVICE=n, the
!BACKLIGHT_CLASS_DEVICE term evaluates to true, allowing
CONFIG_DRM_PARADE_PS8622 to be enabled. In that configuration,
ps8622_probe() and ps8622_remove() call backlight_device_register() and
backlight_device_unregister(), which do not have inline stubs in
include/linux/backlight.h when CONFIG_BACKLIGHT_CLASS_DEVICE=n.
Furthermore, when CONFIG_DRM_LUMINANCE=y and CONFIG_BACKLIGHT_CLASS_DEVICE=n
(or CONFIG_DRM_PARADE_PS8622=y and CONFIG_BACKLIGHT_CLASS_DEVICE=m),
ps8622_remove() only guards backlight_device_unregister(ps8622->bl) with a
runtime check on ps8622->bl rather than a compile-time
!IS_ENABLED(CONFIG_DRM_LUMINANCE) check:
drivers/gpu/drm/bridge/parade-ps8622.c:ps8622_remove() {
...
if (ps8622->bl)
backlight_device_unregister(ps8622->bl);
drm_bridge_remove(&ps8622->bridge);
}
Because ps8622->bl is not a compile-time constant, the call to
backlight_device_unregister() is not eliminated and fails to link. Also,
when CONFIG_DRM_LUMINANCE=n and CONFIG_BACKLIGHT_CLASS_DEVICE=m, this
expression evaluates to n and prevents building DRM_PARADE_PS8622=m.
> diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
> index 3a2ec482037df..229d9d7218c4e 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8622.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8622.c
[ ... ]
> @@ -276,7 +277,7 @@ static int ps8622_send_config(struct ps8622_bridge *ps8622)
>
> /* FFh for 100% brightness, 0h for 0% brightness */
> err = ps8622_set(cl, 0x01, 0xa7,
> - ps8622->bl->props.brightness);
> + ps8622->brightness);
> if (err)
> goto error;
> } else {
[Severity: High]
When CONFIG_DRM_LUMINANCE is enabled, will ps8622_send_config() ever reach
this branch?
In ps8622_probe(), when use-external-pwm is not set and
CONFIG_DRM_LUMINANCE is enabled, the driver sets DRM_BRIDGE_OP_LUMINANCE on
ps8622->bridge.ops instead of calling backlight_device_register(), leaving
ps8622->bl as NULL.
However, ps8622_pre_enable() -> ps8622_send_config() still checks
ps8622->bl to choose between internal and external PWM:
drivers/gpu/drm/bridge/parade-ps8622.c:ps8622_send_config() {
...
if (ps8622->bl) {
/* DPCD720, internal PWM */
err = ps8622_set(cl, 0x01, 0xa5, 0xa0);
if (err)
goto error;
/* FFh for 100% brightness, 0h for 0% brightness */
err = ps8622_set(cl, 0x01, 0xa7,
ps8622->brightness);
if (err)
goto error;
} else {
/* DPCD720, external PWM */
err = ps8622_set(cl, 0x01, 0xa5, 0x80);
if (err)
goto error;
}
...
}
Because ps8622->bl is NULL, won't ps8622_send_config() always take the else
branch, configure register 0x01:0xa5 for external PWM (0x80) instead of
internal PWM (0xa0), and skip programming the initial brightness into
register 0x01:0xa7?
> @@ -324,6 +325,8 @@ static int ps8622_backlight_update(struct backlight_device *bl)
> struct ps8622_bridge *ps8622 = dev_get_drvdata(&bl->dev);
> int ret, brightness = backlight_get_brightness(bl);
>
> + ps8622->brightness = brightness;
> +
> if (!ps8622->enabled)
> return -EINVAL;
>
> @@ -332,6 +335,15 @@ static int ps8622_backlight_update(struct backlight_device *bl)
> return ret;
> }
>
> +static void ps8622_set_luminance(struct drm_bridge *bridge,
> + unsigned int luminance)
> +{
> + struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
> +
> + ps8622->brightness = luminance;
> + ps8622_set(ps8622->client, 0x01, 0xa7, luminance);
> +}
[Severity: Medium]
Should ps8622_set_luminance() check ps8622->enabled before calling
ps8622_set(), just like ps8622_backlight_update() does right above it?
If ps8622_send_config() fails during ps8622_pre_enable(), ps8622->enabled
remains false while drm_atomic_helper_commit_modeset_enables() still calls:
drm_atomic_helper_commit_luminance_enables()
-> drm_bridge_connector_set_luminance()
-> ps8622_set_luminance()
Similarly, if a CRTC transitions from self-refresh (where
ps8622_post_disable() has already set ps8622->enabled = false, disabled
v12, and asserted gpio_slp and gpio_rst) to disabled, disable_outputs()
calls:
drm_atomic_helper_commit_luminance_disables()
-> drm_bridge_connector_disable_luminance()
-> ps8622_set_luminance()
Wouldn't calling ps8622_set(ps8622->client, 0x01, 0xa7, luminance) when
!ps8622->enabled attempt an I2C write while the bridge is powered down and
in reset?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=14
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v10 15/15] drm/solomon: add SSD130x connector luminance support
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (13 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 14/15] drm/bridge: add PS8622 " Mario Limonciello (AMD)
@ 2026-11-02 16:49 ` Mario Limonciello (AMD)
2026-11-02 19:49 ` sashiko-bot
2026-11-02 18:52 ` ✗ Fi.CI.BUILD: failure for Add support for a DRM backlight capability (rev5) Patchwork
15 siblings, 1 reply; 29+ messages in thread
From: Mario Limonciello (AMD) @ 2026-11-02 16:49 UTC (permalink / raw)
To: Maxime Ripard, Alex Deucher, Jani Nikula,
Javier Martinez Canillas, Maarten Lankhorst, Thomas Zimmermann,
David Airlie, Simona Vetter, Harry Wentland, Leo Li,
Rodrigo Siqueira
Cc: Mario Limonciello, Xaver Hugl, dri-devel, linux-kernel, amd-gfx,
intel-gfx, intel-xe, nouveau, Mario Limonciello
From: Mario Limonciello <mario.limonciello@amd.com>
Expose SSD130x contrast through the connector LUMINANCE property when
DRM_LUMINANCE is enabled. Program contrast directly and retain the
backlight class interface when BACKLIGHT_CLASS_DEVICE is enabled.
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v10:
* Guard the backlight operations and registration with
BACKLIGHT_CLASS_DEVICE
* Fix Kconfig recursive issue
* Add the Reviewed-by tag
---
drivers/gpu/drm/solomon/Kconfig | 2 +-
drivers/gpu/drm/solomon/ssd130x.c | 79 ++++++++++++++++++++++++++++++---------
2 files changed, 62 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig
index 400a6cab3a671..76852246eb9c2 100644
--- a/drivers/gpu/drm/solomon/Kconfig
+++ b/drivers/gpu/drm/solomon/Kconfig
@@ -1,7 +1,7 @@
config DRM_SSD130X
tristate "DRM support for Solomon SSD13xx OLED displays"
depends on DRM && MMU
- select BACKLIGHT_CLASS_DEVICE
+ depends on DRM_LUMINANCE || BACKLIGHT_CLASS_DEVICE = y || !BACKLIGHT_CLASS_DEVICE
select DRM_CLIENT_SELECTION
select DRM_GEM_SHMEM_HELPER
select DRM_KMS_HELPER
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index ee43cf0da8792..aaf2b194273ba 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1853,7 +1853,8 @@ static void ssd130x_encoder_atomic_enable(struct drm_encoder *encoder,
ssd130x_write_cmd(ssd130x, 1, SSD13XX_DISPLAY_ON);
- backlight_enable(ssd130x->bl_dev);
+ if (ssd130x->bl_dev)
+ backlight_enable(ssd130x->bl_dev);
return;
@@ -1879,7 +1880,8 @@ static void ssd132x_encoder_atomic_enable(struct drm_encoder *encoder,
ssd130x_write_cmd(ssd130x, 1, SSD13XX_DISPLAY_ON);
- backlight_enable(ssd130x->bl_dev);
+ if (ssd130x->bl_dev)
+ backlight_enable(ssd130x->bl_dev);
return;
@@ -1904,7 +1906,8 @@ static void ssd133x_encoder_atomic_enable(struct drm_encoder *encoder,
ssd130x_write_cmd(ssd130x, 1, SSD13XX_DISPLAY_ON);
- backlight_enable(ssd130x->bl_dev);
+ if (ssd130x->bl_dev)
+ backlight_enable(ssd130x->bl_dev);
return;
@@ -1929,7 +1932,8 @@ static void ssd135x_encoder_atomic_enable(struct drm_encoder *encoder,
ssd130x_write_cmd(ssd130x, 1, SSD13XX_DISPLAY_ON);
- backlight_enable(ssd130x->bl_dev);
+ if (ssd130x->bl_dev)
+ backlight_enable(ssd130x->bl_dev);
return;
@@ -1943,7 +1947,8 @@ static void ssd130x_encoder_atomic_disable(struct drm_encoder *encoder,
struct drm_device *drm = encoder->dev;
struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
- backlight_disable(ssd130x->bl_dev);
+ if (ssd130x->bl_dev)
+ backlight_disable(ssd130x->bl_dev);
ssd130x_write_cmd(ssd130x, 1, SSD13XX_DISPLAY_OFF);
@@ -1980,8 +1985,44 @@ static int ssd130x_connector_get_modes(struct drm_connector *connector)
return drm_connector_helper_get_modes_fixed(connector, &ssd130x->mode);
}
+static int ssd130x_set_luminance_value(struct ssd130x_device *ssd130x,
+ unsigned int luminance)
+{
+ int ret;
+
+ ssd130x->contrast = luminance;
+
+ switch (ssd130x->device_info->family_id) {
+ case SSD133X_FAMILY:
+ return ssd133x_set_contrast(ssd130x, luminance);
+ case SSD135X_FAMILY:
+ return ssd135x_set_contrast(ssd130x, luminance);
+ default:
+ ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST);
+ if (ret < 0)
+ return ret;
+ return ssd130x_write_cmd(ssd130x, 1, luminance);
+ }
+}
+
+static void ssd130x_connector_set_luminance(const struct drm_connector_state *conn_state)
+{
+ struct ssd130x_device *ssd130x = drm_to_ssd130x(conn_state->connector->dev);
+
+ ssd130x_set_luminance_value(ssd130x, conn_state->luminance);
+}
+
+static void ssd130x_connector_disable_luminance(const struct drm_connector_state *conn_state)
+{
+ struct ssd130x_device *ssd130x = drm_to_ssd130x(conn_state->connector->dev);
+
+ ssd130x_set_luminance_value(ssd130x, 0);
+}
+
static const struct drm_connector_helper_funcs ssd130x_connector_helper_funcs = {
.get_modes = ssd130x_connector_get_modes,
+ .set_luminance = ssd130x_connector_set_luminance,
+ .disable_luminance = ssd130x_connector_disable_luminance,
};
static const struct drm_connector_funcs ssd130x_connector_funcs = {
@@ -2015,23 +2056,13 @@ static const struct drm_driver ssd130x_drm_driver = {
.fops = &ssd130x_fops,
};
+#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
static int ssd130x_update_bl(struct backlight_device *bdev)
{
struct ssd130x_device *ssd130x = bl_get_data(bdev);
- int brightness = backlight_get_brightness(bdev);
- int ret;
-
- ssd130x->contrast = brightness;
-
- ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST);
- if (ret < 0)
- return ret;
- ret = ssd130x_write_cmd(ssd130x, 1, ssd130x->contrast);
- if (ret < 0)
- return ret;
-
- return 0;
+ return ssd130x_set_luminance_value(ssd130x,
+ backlight_get_brightness(bdev));
}
static int ssd133x_update_bl(struct backlight_device *bdev)
@@ -2066,6 +2097,7 @@ static const struct backlight_ops ssd130xfb_bl_ops[] = {
.update_status = ssd135x_update_bl,
},
};
+#endif
static void ssd130x_parse_properties(struct ssd130x_device *ssd130x)
{
@@ -2213,6 +2245,13 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
}
drm_connector_helper_add(connector, &ssd130x_connector_helper_funcs);
+ if (IS_ENABLED(CONFIG_DRM_LUMINANCE)) {
+ ret = drm_connector_attach_luminance_property(connector, 0,
+ MAX_CONTRAST,
+ ssd130x->contrast);
+ if (ret)
+ return ret;
+ }
ret = drm_connector_attach_encoder(connector, encoder);
if (ret) {
@@ -2245,7 +2284,9 @@ static int ssd130x_get_resources(struct ssd130x_device *ssd130x)
struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
{
struct ssd130x_device *ssd130x;
+#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
struct backlight_device *bl;
+#endif
struct drm_device *drm;
int ret;
@@ -2270,6 +2311,7 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
if (ret)
return ERR_PTR(ret);
+#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
&ssd130xfb_bl_ops[ssd130x->device_info->family_id],
NULL);
@@ -2280,6 +2322,7 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
bl->props.brightness = ssd130x->contrast;
bl->props.max_brightness = MAX_CONTRAST;
ssd130x->bl_dev = bl;
+#endif
ret = ssd130x_init_modeset(ssd130x);
if (ret)
--
2.53.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v10 15/15] drm/solomon: add SSD130x connector luminance support
2026-11-02 16:49 ` [PATCH v10 15/15] drm/solomon: add SSD130x " Mario Limonciello (AMD)
@ 2026-11-02 19:49 ` sashiko-bot
0 siblings, 0 replies; 29+ messages in thread
From: sashiko-bot @ 2026-11-02 19:49 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: dri-devel, intel-gfx, intel-xe
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/solomon: vmlinux link failure when BACKLIGHT_CLASS_DEVICE=m
- [High] drm/solomon: contrast state desynchronization in ssd130x_set_luminance_value()
- [High] drm/solomon: unsynchronized command writes in ssd130x_set_luminance_value()
- [Medium] drm/solomon: broken Kconfig dependency for BACKLIGHT_CLASS_DEVICE and DRM_LUMINANCE
- [Low] drm/solomon: duplicated contrast dispatch in ssd130xfb_bl_ops
--
commit bcc978ec95ba4732042537500fdd7f79b00e0743
Author: Mario Limonciello <mario.limonciello@amd.com>
drm/solomon: add SSD130x connector luminance support
This commit exposes SSD130x display contrast through the DRM connector
LUMINANCE property when DRM_LUMINANCE is enabled, while guarding the legacy
backlight class registration with BACKLIGHT_CLASS_DEVICE and updating the
Kconfig dependencies.
> diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig
> index 400a6cab3a671..76852246eb9c2 100644
> --- a/drivers/gpu/drm/solomon/Kconfig
> +++ b/drivers/gpu/drm/solomon/Kconfig
> @@ -1,7 +1,7 @@
> config DRM_SSD130X
> tristate "DRM support for Solomon SSD13xx OLED displays"
> depends on DRM && MMU
> - select BACKLIGHT_CLASS_DEVICE
> + depends on DRM_LUMINANCE || BACKLIGHT_CLASS_DEVICE = y || !BACKLIGHT_CLASS_DEVICE
> select DRM_CLIENT_SELECTION
> select DRM_GEM_SHMEM_HELPER
> select DRM_KMS_HELPER
[Severity: High]
Can this Kconfig dependency cause a vmlinux link failure when
CONFIG_DRM_LUMINANCE=y, CONFIG_BACKLIGHT_CLASS_DEVICE=m, and
CONFIG_DRM_SSD130X=y?
Because DRM_LUMINANCE is a bool symbol and BACKLIGHT_CLASS_DEVICE is a
tristate symbol, when DRM_LUMINANCE=y and BACKLIGHT_CLASS_DEVICE=m, the
disjunction evaluates to max(y, n, m) = y, which allows DRM_SSD130X to be
built-in (y).
However, ssd130x_probe() in drivers/gpu/drm/solomon/ssd130x.c guards the call
to devm_backlight_device_register() with #if
IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE), which evaluates to 1 when
CONFIG_BACKLIGHT_CLASS_DEVICE=m. Built-in ssd130x.o will then call
devm_backlight_device_register() from the loadable module backlight.ko (which
has no inline stub in include/linux/backlight.h), failing to link vmlinux.
[Severity: Medium]
Does replacing select BACKLIGHT_CLASS_DEVICE with this dependency also cause a
regression when both CONFIG_DRM_LUMINANCE=n and
CONFIG_BACKLIGHT_CLASS_DEVICE=n?
When both options are disabled, !BACKLIGHT_CLASS_DEVICE evaluates to y,
allowing DRM_SSD130X to build with neither /sys/class/backlight nor the
connector LUMINANCE property enabled, leaving the display with no contrast
control interface at all.
In addition, on SSD132X_FAMILY panels, ssd132x_init() hardcodes
SSD13XX_CONTRAST to 0x80 and previously relied on backlight_enable() in
ssd132x_encoder_atomic_enable() to program ssd130x->contrast (127). When both
options are disabled, neither backlight_enable() nor
drm_atomic_helper_commit_luminance_enables() runs to apply ssd130x->contrast.
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index ee43cf0da8792..aaf2b194273ba 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
[ ... ]
> @@ -1980,8 +1985,44 @@ static int ssd130x_connector_get_modes(struct drm_connector *connector)
> return drm_connector_helper_get_modes_fixed(connector, &ssd130x->mode);
> }
>
> +static int ssd130x_set_luminance_value(struct ssd130x_device *ssd130x,
> + unsigned int luminance)
> +{
> + int ret;
> +
> + ssd130x->contrast = luminance;
> +
> + switch (ssd130x->device_info->family_id) {
> + case SSD133X_FAMILY:
> + return ssd133x_set_contrast(ssd130x, luminance);
> + case SSD135X_FAMILY:
> + return ssd135x_set_contrast(ssd130x, luminance);
> + default:
> + ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST);
> + if (ret < 0)
> + return ret;
> + return ssd130x_write_cmd(ssd130x, 1, luminance);
> + }
> +}
> +
> +static void ssd130x_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct ssd130x_device *ssd130x = drm_to_ssd130x(conn_state->connector->dev);
> +
> + ssd130x_set_luminance_value(ssd130x, conn_state->luminance);
> +}
> +
> +static void ssd130x_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> + struct ssd130x_device *ssd130x = drm_to_ssd130x(conn_state->connector->dev);
> +
> + ssd130x_set_luminance_value(ssd130x, 0);
> +}
[Severity: High]
Can concurrent DRM atomic commits and sysfs backlight writes interleave the
multi-byte command writes in ssd130x_set_luminance_value()?
When both CONFIG_DRM_LUMINANCE and CONFIG_BACKLIGHT_CLASS_DEVICE are enabled,
sysfs backlight updates invoke ssd130x_update_bl(), ssd133x_update_bl(), or
ssd135x_update_bl() under bdev->update_lock, whereas DRM atomic commits invoke
ssd130x_connector_set_luminance() and ssd130x_connector_disable_luminance()
from drm_atomic_helper_commit_tail() without holding bdev->update_lock or any
shared driver lock.
In ssd130x_set_luminance_value(), ssd130x->contrast is updated without locking,
and on SSD130X_FAMILY and SSD132X_FAMILY the contrast update is sent as two
separate ssd130x_write_cmd() calls (and ssd133x_set_contrast() similarly issues
multiple separate regmap writes):
ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST);
if (ret < 0)
return ret;
return ssd130x_write_cmd(ssd130x, 1, luminance);
If a sysfs backlight write and a DRM atomic luminance update race between
those two regmap_write() calls, the second thread will send SSD13XX_CONTRAST
(0x81) before the first thread sends its parameter byte. Wouldn't the OLED
controller consume 0x81 as the contrast parameter for the first command, and
then interpret both threads' subsequent luminance parameter bytes as arbitrary
SSD13XX_COMMAND opcodes (such as 0xae SSD13XX_DISPLAY_OFF, 0xa0/0xa1 segment
remap, or 0xc0/0xc8 COM scan direction)?
[Severity: High]
Does exposing both ssd130x->bl_dev and the connector LUMINANCE property at the
same time desynchronize contrast state across atomic commits and display
disable/enable cycles?
In ssd130x_probe() and ssd130x_init_modeset(), the driver registers
ssd130x->bl_dev whenever IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) is true and
also attaches the LUMINANCE property whenever IS_ENABLED(CONFIG_DRM_LUMINANCE)
is true, unlike drivers that only register a legacy backlight device when
DRM_LUMINANCE is not used:
1. ssd130x_connector_set_luminance() updates ssd130x->contrast via
ssd130x_set_luminance_value() without updating
ssd130x->bl_dev->props.brightness, while ssd130x_update_bl(),
ssd133x_update_bl(), and ssd135x_update_bl() update ssd130x->contrast from
bl_dev->props.brightness without updating connector->state->luminance. If
brightness is changed via /sys/class/backlight, any subsequent atomic commit
including the active connector runs
drm_atomic_helper_commit_luminance_enables() and overwrites the hardware
contrast back to conn_state->luminance.
2. When disabling the display, drm_atomic_helper_commit_luminance_disables()
calls ssd130x_connector_disable_luminance() ->
ssd130x_set_luminance_value(ssd130x, 0), which clobbers ssd130x->contrast to
0, and ssd130x_encoder_atomic_disable() then calls
backlight_disable(ssd130x->bl_dev), writing contrast 0 a second time.
3. When re-enabling the display, ssd130x_encoder_atomic_enable() (and its
ssd132x, ssd133x, and ssd135x counterparts) runs before
drm_atomic_helper_commit_luminance_enables(): ssd130x_init(), ssd133x_init(),
and ssd135x_init() first initialize the panel with the clobbered
ssd130x->contrast (0) prior to SSD13XX_DISPLAY_ON; next,
backlight_enable(ssd130x->bl_dev) programs the stale bl_dev->props.brightness
while the display is on; and finally
drm_atomic_helper_commit_luminance_enables() calls
ssd130x_connector_set_luminance() to program conn_state->luminance.
4. If ssd130x_power_on() or ssd130x_init() fails in
ssd130x_encoder_atomic_enable() (or its ssd132x, ssd133x, or ssd135x variants)
and powers off vcc_reg, drm_atomic_helper_commit_modeset_enables() still calls
drm_atomic_helper_commit_luminance_enables() ->
ssd130x_connector_set_luminance(), issuing contrast commands over I2C/SPI to
an unpowered controller.
[ ... ]
> @@ -2015,23 +2056,13 @@ static const struct drm_driver ssd130x_drm_driver = {
> .fops = &ssd130x_fops,
> };
>
> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> static int ssd130x_update_bl(struct backlight_device *bdev)
> {
> struct ssd130x_device *ssd130x = bl_get_data(bdev);
> - int brightness = backlight_get_brightness(bdev);
> - int ret;
> -
> - ssd130x->contrast = brightness;
> -
> - ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST);
> - if (ret < 0)
> - return ret;
>
> - ret = ssd130x_write_cmd(ssd130x, 1, ssd130x->contrast);
> - if (ret < 0)
> - return ret;
> -
> - return 0;
> + return ssd130x_set_luminance_value(ssd130x,
> + backlight_get_brightness(bdev));
> }
>
> static int ssd133x_update_bl(struct backlight_device *bdev)
> @@ -2066,6 +2097,7 @@ static const struct backlight_ops ssd130xfb_bl_ops[] = {
> .update_status = ssd135x_update_bl,
> },
> };
> +#endif
[Severity: Low]
This isn't a bug, but since ssd130x_update_bl() now delegates to
ssd130x_set_luminance_value(), which already switches on
ssd130x->device_info->family_id and dispatches to ssd133x_set_contrast() and
ssd135x_set_contrast(), could ssd133x_update_bl(), ssd135x_update_bl(), and the
per-family ssd130xfb_bl_ops[] array be dropped in favor of a single
backlight_ops struct using ssd130x_update_bl()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=15
^ permalink raw reply [flat|nested] 29+ messages in thread
* ✗ Fi.CI.BUILD: failure for Add support for a DRM backlight capability (rev5)
2026-11-02 16:49 [PATCH v10 00/15] Add support for a DRM backlight capability Mario Limonciello (AMD)
` (14 preceding siblings ...)
2026-11-02 16:49 ` [PATCH v10 15/15] drm/solomon: add SSD130x " Mario Limonciello (AMD)
@ 2026-11-02 18:52 ` Patchwork
15 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2026-11-02 18:52 UTC (permalink / raw)
To: Mario Limonciello; +Cc: intel-gfx
== Series Details ==
Series: Add support for a DRM backlight capability (rev5)
URL : https://patchwork.freedesktop.org/series/169109/
State : failure
== Summary ==
Error: patch https://patchwork.freedesktop.org/api/1.0/series/169109/revisions/5/mbox/ not applied
Applying: drm: add connector LUMINANCE property
Applying: drm/amd/display: Pass up errors reading actual brightness
Applying: drm/amd: add luminance property support
Applying: drm/amdgpu: Check bios_scratch_reg_offset in backlight level helper
Applying: drm/amd/display: update KUnit luminance fixtures
Using index info to reconstruct a base tree...
M drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
M drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_kunit_helpers.c
Auto-merging drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 drm/amd/display: update KUnit luminance fixtures
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced
^ permalink raw reply [flat|nested] 29+ messages in thread