Re: [PATCH v8 30/39] drm/bridge: display-connector: Implement readout support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/bridge: unreachable fallback in
  display_connector_readout_state()
--

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

drm/bridge: display-connector: Implement readout support

This commit adds state readout support to the display-connector bridge by
implementing the atomic_sro_readout_state callback and wiring up
drm_atomic_helper_bridge_compare_state().

> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> index eb3412ec30a07..781684da1da4c 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
[ ... ]
> @@ -213,6 +216,70 @@ 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);
> +
> +		if (prev_priv_state) {
> +			struct drm_bridge_state *prev_bridge_state =
> +				drm_priv_to_bridge_state(prev_priv_state);
> +
> +			bridge_state->input_bus_cfg.format =
> +				prev_bridge_state->output_bus_cfg.format;
> +			bridge_state->input_bus_cfg.flags =
> +				prev_bridge_state->output_bus_cfg.flags;
> +			goto out_input;
> +		}
> +	}
> +
> +	/*
> +	 * The previous bridge might not have readout support, so we
> +	 * can't treat a missing state as an error.
> +	 */
> +	bridge_state->input_bus_cfg.format = MEDIA_BUS_FMT_FIXED;
> +	bridge_state->input_bus_cfg.flags = di->bus_flags;

[Severity: Medium]
Can this fallback ever be reached when prev_bridge is present but lacks
readout support?

When a bridge does not implement atomic_sro_readout_state,
drm_atomic_helper_sro_readout_bridge_state() still allocates a zeroed
pristine state and stores it in the SRO state container:

drivers/gpu/drm/drm_atomic_state_readout_helper.c:drm_atomic_helper_sro_readout_bridge_state() {
	if (!bridge_obj_funcs->atomic_sro_readout_state) {
		...
		bridge_obj_state = bridge_obj_funcs->atomic_create_state(bridge_obj);
		if (drm_WARN_ON(dev, IS_ERR(bridge_obj_state)))
			return PTR_ERR(bridge_obj_state);

		drm_atomic_sro_set_private_obj_state(state, bridge_obj,
						     bridge_obj_state);
		return 0;
	}
}

Because prev_priv_state is non-NULL, display_connector_readout_state()
copies the zeroed output_bus_cfg.format and output_bus_cfg.flags from that
pristine state instead of falling back to MEDIA_BUS_FMT_FIXED and
di->bus_flags, causing drm_atomic_helper_bridge_compare_state() to report
false input bus mismatches when drm.atomic_readout skips missing readout
implementations.  Checking prev_bridge->funcs->atomic_sro_readout_state
before using prev_priv_state would avoid this.

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