Re: [PATCH v6 21/28] drm/bridge: display-connector: Implement readout support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/bridge: display-connector SRO readout aborts on missing upstream state
--

commit 3d42e46c975cd0148add9f035d4abfcb174d9299
Author: Maxime Ripard <mripard@kernel.org>

drm/bridge: display-connector: Implement readout support

The display-connector bridge is the last bridge in the chain on
platforms using drm_bridge_connector. This commit implements the
atomic_sro_readout_state hook, reconstructing the bridge bus configuration
from the previous bridge's readout state and the connector's display_info.

> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> index eb3412ec30a07..7f285b7c1e84b 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
[ ... ]
> @@ -213,6 +216,65 @@ static u32 *display_connector_get_input_bus_fmts(struct drm_bridge *bridge,
[ ... ]
> +static int
> +display_connector_readout_state(struct drm_bridge *bridge,
> +				struct drm_atomic_sro_state *state,
> +				struct drm_bridge_state *bridge_state,
> +				struct drm_crtc_state *crtc_state,
> +				struct drm_connector_state *conn_state)
> +{
> +	struct drm_display_info *di = &conn_state->connector->display_info;
> +	struct drm_bridge *prev_bridge __free(drm_bridge_put) =
> +		drm_bridge_get_prev_bridge(bridge);
> +
> +	if (prev_bridge) {
> +		struct drm_private_state *prev_priv_state =
> +			drm_atomic_sro_get_private_obj_state(state,
> +							     &prev_bridge->base);
> +		struct drm_bridge_state *prev_bridge_state;
> +
> +		if (!prev_priv_state)
> +			return -ENODEV;

[Severity: Medium]
Will this hard error abort the entire SRO readout if an upstream bridge is
missing readout support?

If the DRM core is configured to attempt readout despite missing
implementations, drm_atomic_helper_sro_readout_bridges_state() will skip
bridges lacking an atomic_sro_readout_state hook:

		if (!bridge_obj_funcs->atomic_sro_readout_state) {
			drm_warn(dev,
				 "No bridge %s readout implementation.",
				 bridge->name);
			continue;
		}

If that happens, prev_bridge will be non-NULL, but prev_priv_state will be
NULL since the core continued without allocating it. Returning -ENODEV here
causes the core to fail with a warning, rather than gracefully falling back
to default values like it does when there is no prev_bridge at all.

Should this fall back to MEDIA_BUS_FMT_FIXED instead of returning an error?

> +
> +		prev_bridge_state =
> +			drm_priv_to_bridge_state(prev_priv_state);
> +
> +		bridge_state->input_bus_cfg.format =
> +			prev_bridge_state->output_bus_cfg.format;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=21