Re: [PATCH v10 01/15] drm: add connector LUMINANCE property

[sashiko-bot@kernel.org]

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