Re: [PATCH v11 07/12] drm/panel: add connector luminance forwarding

[sashiko-bot@kernel.org]

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 <mario.limonciello@amd.com>

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-00cce7b2ba22@kernel.org?part=7