Re: [PATCH v8 38/39] drm/panel: Implement state readout support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/panel: active readout in panel_bridge_connector_readout()
  desyncs drm_panel
- [Medium] drm/panel: panel_bridge_connector_readout() returns -ENODEV
  on inactive CRTC
--

commit 64e424167a6978d4eae36faa89c4949c44fe1a60
Author: Maxime Ripard <mripard@kernel.org>

drm/panel: Implement state readout support

This commit implements atomic state readout callbacks for the panel bridge,
reconstructing the negotiated bus configuration and binding the connector
to the encoder's active CRTC when no earlier bridge provides connector
readout.

> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 560ea57591bd7..bef0e6af73ba5 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c

[ ... ]

> @@ -226,6 +252,100 @@ static void panel_bridge_debugfs_init(struct drm_bridge *bridge,
>  		panel->funcs->debugfs_init(panel, root);
>  }
>  

[ ... ]

> +/*
> + * A panel bridge has no way to read the panel state out of the
> + * hardware: assume the panel is enabled whenever its pipeline is.
> + */
> +static int
> +panel_bridge_connector_readout(struct drm_bridge *bridge,
> +			       struct drm_atomic_sro_state *state,
> +			       struct drm_connector_state *conn_state)
> +{
> +	struct drm_connector *connector = conn_state->connector;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_encoder *encoder;
> +	struct drm_crtc *crtc;
> +
> +	encoder = bridge->encoder;
> +	if (!encoder->funcs || !encoder->funcs->atomic_sro_get_current_crtc)
> +		return -ENODEV;
> +
> +	crtc = encoder->funcs->atomic_sro_get_current_crtc(encoder);
> +	if (!crtc)
> +		return -ENODEV;
> +
> +	crtc_state = drm_atomic_sro_get_crtc_state(state, crtc);
> +	if (!crtc_state)
> +		return -ENODEV;

[Severity: Medium]
Should panel_bridge_connector_readout() return 0 instead of -ENODEV when
crtc or crtc_state is NULL?

In include/drm/drm_encoder.h, atomic_sro_get_current_crtc() is documented
to return NULL when the encoder is disabled.  In addition, during a
commit-scoped readout in drm_atomic_sro_readout_and_compare() that touches
a disabled connector without including its CRTC in the commit,
drm_atomic_sro_get_crtc_state() returns NULL.  Returning -ENODEV in these
cases causes drm_atomic_helper_sro_build_state() to fail readout when the
pipeline is off.

> +
> +	/* The pipeline is off, there is nothing to read out. */
> +	if (!crtc_state->active)
> +		return 0;
> +
> +	crtc_state->encoder_mask |= drm_encoder_mask(encoder);
> +	crtc_state->connector_mask |= drm_connector_mask(connector);
> +
> +	/*
> +	 * drm_atomic_set_crtc_for_connector() takes a connector
> +	 * reference when setting the crtc pointer, which is later
> +	 * released by __drm_atomic_helper_connector_destroy_state().
> +	 * The readout bypasses that path, so take the reference
> +	 * explicitly.
> +	 */
> +	drm_connector_get(connector);
> +	conn_state->crtc = crtc;
> +	conn_state->best_encoder = encoder;
> +
> +	return 0;
> +}

[Severity: High]
When a pipeline is read out as active, does binding conn_state to the CRTC
without preparing or enabling the underlying struct drm_panel leave
panel->prepared and panel->enabled out of sync with the atomic state?

Nothing in the state installation path calls drm_panel_prepare() or
drm_panel_enable().  On a subsequent fastboot commit where
drm_atomic_crtc_needs_modeset() is false, panel_bridge_atomic_pre_enable()
and panel_bridge_atomic_enable() are skipped, so the panel's regulators,
clocks, backlight and followers are never enabled in the kernel.  When a
later commit disables the pipeline, panel_bridge_atomic_disable() and
panel_bridge_atomic_post_disable() call drm_panel_disable() and
drm_panel_unprepare(), which hit the !panel->enabled and !panel->prepared
guards, log a warning and skip hardware teardown.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-drm-state-readout-v8-0-cbe78624e1bc@kernel.org?part=38