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 <[email protected]> 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/[email protected]?part=15
