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 <[email protected]> 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-b2a6f2413...@kernel.org?part=8
