Re: [PATCH v8 25/39] drm/atomic-state-helper: Provide atomic_compare_state helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/atomic: false plane mismatches in
  drm_atomic_helper_plane_compare_state()
- [Low] drm/atomic: missing crtc return in
  drm_atomic_helper_connector_compare_state()
--

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

drm/atomic-state-helper: Provide atomic_compare_state helpers

This commit adds default atomic_sro_compare_state helper implementations
for planes, CRTCs, connectors, and bridges.

> diff --git a/drivers/gpu/drm/drm_atomic_state_readout_helper.c b/drivers/gpu/drm/drm_atomic_state_readout_helper.c
> index 4156f3a27f049..57376c9d4014b 100644
> --- a/drivers/gpu/drm/drm_atomic_state_readout_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_readout_helper.c
[ ... ]
> @@ -788,6 +789,311 @@ int drm_atomic_helper_sro_readout_state(struct drm_device *dev)
[ ... ]
> +bool drm_atomic_helper_plane_compare_state(struct drm_plane *plane,
> +					   struct drm_printer *p,
> +					   struct drm_plane_state *expected,
> +					   struct drm_plane_state *actual)
> +{
> +	bool ret = true;
> +
> +	STATE_CHECK_PTR(ret, p, plane->name, expected, actual, plane);
> +	STATE_CHECK_PTR(ret, p, plane->name, expected, actual, crtc);
> +
> +	/* Both disabled, geometry is meaningless. */
> +	if (!expected->fb && !actual->fb)
> +		return ret;
> +
> +	/* One enabled and the other disabled, that's a mismatch. */
> +	if (!expected->fb || !actual->fb) {
> +		drm_atomic_helper_print_state_mismatch(p,
> +						       plane->name,
> +						       "fb",
> +						       "expected framebuffer is %s, got %s",
> +						       expected->fb ? "non-NULL" : "NULL",
> +						       actual->fb ? "non-NULL" : "NULL");
> +		return false;
> +	}

[Severity: Medium]
Does drm_atomic_helper_plane_compare_state() report false crtc and fb
mismatches when a plane is clipped off-screen or its CRTC is set to DPMS
off?

During a DPMS off commit (enable = true, active = false) or when a plane
is clipped off-screen (visible = false), expected->crtc and expected->fb
remain non-NULL in the committed state, while the hardware plane is
disabled so hardware readout produces a pristine state with actual->crtc
and actual->fb set to NULL.  Unlike the connector, bridge, and CRTC paths,
neither drm_atomic_sro_readout_and_compare() nor
drm_atomic_helper_plane_compare_state() checks whether the plane is
visible or bound to an active CRTC before comparing crtc and fb, causing
drm_atomic_sro_readout_and_compare() to log false mismatches and return
false.

[ ... ]

> +bool drm_atomic_helper_connector_compare_state(struct drm_connector *conn,
> +					       struct drm_printer *p,
> +					       struct drm_connector_state *expected,
> +					       struct drm_connector_state *actual)
> +{
> +	bool ret = true;
> +
> +	STATE_CHECK_PTR(ret, p, conn->name, expected, actual, connector);
> +
> +	/*
> +	 * Both unbound, the routing and the negotiated properties are
> +	 * meaningless: a disabled connector keeps the values it was
> +	 * last enabled with, while a state read out from a disabled
> +	 * pipeline is pristine.
> +	 */
> +	if (!expected->crtc && !actual->crtc)
> +		return ret;
> +
> +	STATE_CHECK_PTR(ret, p, conn->name, expected, actual, crtc);
> +	STATE_CHECK_PTR(ret, p, conn->name, expected, actual, best_encoder);
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, link_status);
> +
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, tv.select_subconnector);
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, tv.subconnector);

[Severity: Low]
Should drm_atomic_helper_connector_compare_state() return false right
after checking crtc when one of expected->crtc or actual->crtc is NULL?
Continuing when one state is unbound compares negotiated properties
against stale or pristine fields and logs spurious mismatches, unlike
drm_atomic_helper_plane_compare_state() and
drm_atomic_helper_crtc_compare_state().

Also, only tv.select_subconnector and tv.subconnector are compared here,
leaving the remaining struct drm_tv_connector_state fields (margins,
legacy_mode, mode, brightness, contrast, flicker_reduction, overscan,
saturation, and hue) unchecked.

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