Re: [PATCH v7 15/34] drm/bridge: Handle bridges with hardware state readout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/bridge: spurious WARN_ON in find_connector_state_for_encoder()
--

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

drm/bridge: Handle bridges with hardware state readout

This commit adds hardware state readout support for DRM bridges by introducing
drm_bridge_atomic_readout_priv_state() to look up the associated connector and
CRTC states and forward them to the bridge's atomic_sro_readout_state hook. It
also introduces a separate drm_private_state_funcs vtable so
drm_atomic_sro_device_can_readout() can distinguish bridges with and without
SRO support.

> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index b2d77824e4bed..5977157d8e679 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
[ ... ]
> @@ -561,6 +562,73 @@ drm_bridge_atomic_create_priv_state(struct drm_private_obj *obj)
>  	return &state->base;
>  }
>  
> +static struct drm_connector_state *
> +find_connector_state_for_encoder(struct drm_atomic_sro_state *state,
> +				struct drm_encoder *encoder)
> +{
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector;
> +
> +	drm_connector_list_iter_begin(drm_atomic_sro_state_get_device(state),
> +				      &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		struct drm_connector_state *conn_state =
> +			drm_atomic_sro_get_connector_state(state, connector);
> +
> +		if (WARN_ON(!conn_state))
> +			continue;

[Severity: Medium]
Can this WARN_ON(!conn_state) trigger spuriously when state readout only
populates a subset of connectors?

When drm_atomic_sro_readout_and_compare() builds an SRO state for a blocking
commit via drm_atomic_helper_sro_build_state() with a non-NULL commit, only
the connectors included in that commit have their states populated in
state->connectors[]:

include/drm/drm_modeset_helper_vtables.h:
	 * When @commit is not NULL, only the objects that have a state in
	 * @commit are read out. The caller must hold the locks of the
	 * objects in @commit through its acquire context, and the readout
	 * hooks must not access the state of any object outside of the
	 * commit, since its locks aren't held.

Similarly, connectors without readout support are skipped when atomic_readout
is set to DRM_ATOMIC_READOUT_SKIP_MISSING_READOUT.

Because find_connector_state_for_encoder() iterates over all connectors on the
drm_device via drm_for_each_connector_iter(), any connector on a
multi-connector device that was not part of the commit (or when a commit
disables the encoder's connector routing) will have a NULL conn_state and
trigger WARN_ON(!conn_state). By contrast, drm_bridge_atomic_readout_priv_state()
checks if (!crtc_state) without warning.

Additionally, iterating the live device connector list passes every connector
to drm_atomic_sro_get_connector_state():

drivers/gpu/drm/drm_atomic_state_readout.c:drm_atomic_sro_get_connector_state() {
	unsigned int index = drm_connector_index(connector);

	return state->connectors[index].state;
}

Could this also read past state->connectors[] if a dynamically registered
connector has an index greater than state->max_connector_index, or should
find_connector_state_for_encoder() iterate only over the connectors populated
in struct drm_atomic_sro_state (for example via for_each_connector_in_sro_state())
without WARN_ON(!conn_state)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-drm-state-readout-v7-0-25e5163d75c3@kernel.org?part=15