Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/atomic_sro: partial commit readout triggers WARN_ON(!conn_state) in bridge readout --
commit baa08bb6dbe7a515f8de48108d95e19b64d5be4d Author: Maxime Ripard <[email protected]> drm/atomic_helper: Compare actual and readout states once the commit is done This commit adds the atomic_sro_compare_state hook across KMS objects and introduces drm_atomic_sro_readout_and_compare(), which is called from commit_tail() after blocking commits to read out hardware state and compare it against the committed state. > diff --git a/drivers/gpu/drm/drm_atomic_state_readout.c > b/drivers/gpu/drm/drm_atomic_state_readout.c > index 2c17fa09c5f08..f29d127f99f81 100644 > --- a/drivers/gpu/drm/drm_atomic_state_readout.c > +++ b/drivers/gpu/drm/drm_atomic_state_readout.c [ ... ] > +bool drm_atomic_sro_readout_and_compare(struct drm_atomic_commit *commit) > +{ > + struct drm_device *dev = commit->dev; > + const struct drm_mode_config_helper_funcs *funcs = > + dev->mode_config.helper_private; > + struct drm_printer p = drm_err_printer(dev, NULL); > + struct drm_private_state *new_obj_state; > + struct drm_private_obj *obj; > + struct drm_plane_state *new_plane_state; > + struct drm_plane *plane; > + struct drm_crtc_state *new_crtc_state; > + struct drm_crtc *crtc; > + struct drm_connector_state *new_conn_state; > + struct drm_connector *conn; > + struct drm_atomic_sro_state *readout_state; > + unsigned int i; > + bool identical = true; > + > + might_alloc(GFP_KERNEL); > + > + if (!drm_atomic_sro_device_can_readout(dev)) > + return true; > + > + readout_state = funcs->atomic_sro_build_state(dev, commit); > + if (WARN_ON(IS_ERR(readout_state))) > + return false; [Severity: Medium] When commit_tail() calls drm_atomic_sro_readout_and_compare() for a blocking commit that only includes a subset of connectors on a multi-connector device with bridges, won't funcs->atomic_sro_build_state(dev, commit) trigger a WARN_ON(!conn_state) in find_connector_state_for_encoder()? When commit is non-NULL, drm_atomic_helper_sro_readout_connectors_state() calls drm_atomic_helper_sro_readout_commit_connectors_state() in drivers/gpu/drm/drm_atomic_state_readout_helper.c, which only populates readout_state for connectors present in commit: for_each_new_connector_in_state(commit, connector, new_connector_state, i) { ret = drm_atomic_helper_sro_readout_connector_state(dev, state, connector); if (ret) return ret; } Any other connector on dev has NULL state in readout_state. Then drm_atomic_helper_sro_readout_bridges_state() calls drm_atomic_helper_sro_readout_commit_bridges_state(), which walks the bridge chains of old_conn_state->best_encoder and new_conn_state->best_encoder and invokes drm_bridge_atomic_readout_priv_state() -> find_connector_state_for_encoder() in drivers/gpu/drm/drm_bridge.c: 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; if (encoder == conn_state->best_encoder) { drm_connector_list_iter_end(&conn_iter); return conn_state; } } Because find_connector_state_for_encoder() iterates over all connectors on the device, visiting any connector not included in commit (either an uncommitted connector preceding the active connector in the connector list, or any uncommitted connector when disabling a connector so old_encoder is no longer routed to any connector) hits WARN_ON(!conn_state). Also, when a commit disables a connector, drm_atomic_helper_check_modeset() adds old_conn_state->best_encoder's bridges to commit by duplicating their previous active bridge state, whereas drm_bridge_atomic_readout_priv_state() leaves the readout bridge state as a pristine default state when find_connector_state_for_encoder(state, old_encoder) returns NULL. Won't that cause drm_atomic_sro_readout_and_compare() to report a false mismatch on the bridge private object state when a connector is disabled? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=20
