Re: [PATCH v7 20/34] drm/atomic_helper: Compare actual and readout states once the commit is done

[sashiko-bot@kernel.org]

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 <mripard@kernel.org>

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/20261029-drm-state-readout-v7-0-25e5163d75c3@kernel.org?part=20