Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/panel: premature and redundant backlight update in drm_panel_set_luminance() --
commit 7712bf002ce98036567ec37ca544f1c1930a93be Author: Mario Limonciello <[email protected]> drm/panel: add connector luminance forwarding This commit adds helpers to expose a panel backlight via the connector LUMINANCE property and forward connector updates to the backlight backend. It hooks these helpers 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 74c96da45b7a5..bbd0e14401c18 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c [ ... ] > @@ -169,6 +184,19 @@ static void panel_bridge_atomic_enable(struct drm_bridge > *bridge, > if (old_crtc_state && old_crtc_state->self_refresh_active) > return; > > + /* > + * Program the committed luminance before drm_panel_enable() turns the > + * backlight on, so the panel does not light up at a stale level. > + */ > + if (panel->connector.luminance_property) { > + const struct drm_connector_state *conn_state = > + drm_atomic_get_new_connector_state(atomic_state, > + &panel->connector); > + > + if (conn_state) > + drm_panel_set_luminance(panel, conn_state->luminance); > + } > + > drm_panel_enable(panel); > } > > @@ -873,6 +901,71 @@ void drm_panel_disable(struct drm_panel *panel) [ ... ] > +/** > + * drm_panel_set_luminance - set a panel backlight's relative luminance > + * @panel: panel whose backlight is controlled > + * @luminance: relative luminance level > + * > + * Updates the backlight brightness without changing its power state, under > + * the same lock as brightness changes made through sysfs. Turning > + * the backlight on and off is left to drm_panel_enable() and > + * drm_panel_disable(), so a level set while the panel is disabled is applied > + * when the panel is enabled again. > + */ > +void drm_panel_set_luminance(struct drm_panel *panel, unsigned int luminance) > +{ > + struct backlight_device *backlight; > + > + if (!panel || !panel->backlight) > + return; > + > + backlight = panel->backlight; > + guard(mutex)(&backlight->ops_lock); > + if (!backlight->ops || luminance > backlight->props.max_brightness) > + return; > + > + backlight->props.brightness = luminance; > + backlight_update_status(backlight); > +} > +EXPORT_SYMBOL(drm_panel_set_luminance); [Severity: Medium] Can calling backlight_update_status() unconditionally in drm_panel_set_luminance() cause the backlight hardware to be programmed or unblanked before panel->funcs->enable() runs, as well as trigger three consecutive backlight updates on every modeset enable? During a modeset enable, drm_atomic_helper_commit_modeset_enables() runs: drm_atomic_helper_commit_encoder_bridge_enable() -> panel_bridge_atomic_enable() -> drm_panel_set_luminance() -> drm_panel_enable() drm_atomic_helper_commit_luminance_enables() -> panel_bridge_connector_set_luminance() -> drm_panel_set_luminance() When panel_bridge_atomic_enable() calls drm_panel_set_luminance() before drm_panel_enable(), panel->enabled is still false and panel->funcs->enable() has not yet executed: drivers/gpu/drm/drm_panel.c:drm_panel_enable() { ... if (panel->funcs && panel->funcs->enable) { ret = panel->funcs->enable(panel); if (ret < 0) goto exit; } panel->enabled = true; ret = backlight_enable(panel->backlight); ... } Because BACKLIGHT_POWER_ON is 0, backlight devices registered with zero-initialized props.power and props.state (or left active at boot) are not marked blanked prior to the first drm_panel_disable() call, and several panel backlight callbacks (such as visionox_r66451_bl_update_status() and rad_bl_update_status()) send MIPI_DCS_SET_DISPLAY_BRIGHTNESS commands in update_status() regardless of blank state. Calling backlight_update_status() before drm_panel_enable() can therefore access or unblank the panel before panel->funcs->enable() has completed its power-on sequence and mandatory enable delays. In addition, if an external caller invokes drm_panel_set_luminance() while !panel->enabled, calling backlight_update_status() immediately does not match the kernel-doc comment stating that a level set while the panel is disabled is applied when the panel is enabled again. Once drm_panel_enable() runs, backlight_enable() calls backlight_update_status() a second time to apply backlight->props.brightness. Immediately after, drm_atomic_helper_commit_luminance_enables() sees drm_atomic_crtc_needs_modeset(new_crtc_state) and calls panel_bridge_connector_set_luminance() -> drm_panel_set_luminance(), which calls backlight_update_status() a third time even though backlight->props.brightness already equals luminance. Should drm_panel_set_luminance() only store backlight->props.brightness when !panel->enabled (leaving the hardware update to backlight_enable() in drm_panel_enable()), and skip backlight_update_status() when backlight->props.brightness is unchanged? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261103-superm1-backlight-property-v11-0-00cce7b2b...@kernel.org?part=7
