Re: [PATCH v7 22/34] drm/atomic-state-helper: Provide atomic_compare_state helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] drm/atomic: drm_atomic_helper_connector_compare_state() checks inactive/request state
- [Medium] drm/atomic: unconditional infoframe data check in connector state compare
- [Medium] drm/atomic-state-helper: plane state compare checks transient damage fields
- [Medium] drm/atomic-state-helper: false enable mismatch on DPMS OFF in drm_atomic_helper_crtc_compare_state()
--

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

drm/atomic-state-helper: Provide atomic_compare_state helpers

This commit adds default atomic_sro_compare_state helper functions for
planes, CRTCs, connectors, and bridges to compare committed software states
against states read back from hardware.

> diff --git a/drivers/gpu/drm/drm_atomic_state_readout_helper.c b/drivers/gpu/drm/drm_atomic_state_readout_helper.c
> index 33f0f78c9b265..c4089afbc946e 100644
> --- a/drivers/gpu/drm/drm_atomic_state_readout_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_readout_helper.c

[ ... ]

> @@ -680,6 +681,268 @@ 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)
> +{

[ ... ]

> +	STATE_CHECK_U32(ret, p, plane->name, expected, actual, color_encoding);
> +	STATE_CHECK_U32(ret, p, plane->name, expected, actual, color_range);
> +
> +	STATE_CHECK_PROPERTY_BLOB(ret, p, plane->name, expected, actual, fb_damage_clips);
> +	STATE_CHECK_BOOL(ret, p, plane->name, expected, actual, ignore_damage_clips);

[Severity: Medium]
Should fb_damage_clips and ignore_damage_clips be excluded from
drm_atomic_helper_plane_compare_state()?

In struct drm_plane_state, fb_damage_clips is a transient per-commit blob
describing damaged rectangles relative to the previous frame, which
__drm_atomic_helper_plane_duplicate_state() explicitly resets on duplication:

drivers/gpu/drm/drm_atomic_state_helper.c:__drm_atomic_helper_plane_duplicate_state() {
	...
	state->fence = NULL;
	state->commit = NULL;
	state->fb_damage_clips = NULL;
	state->color_mgmt_changed = false;
	...
}

Similarly, ignore_damage_clips is an internal software flag used by
drm_atomic_helper_damage_iter_init(). Neither field is stored in display
hardware registers, so the readout state allocated via atomic_create_state()
and populated by atomic_sro_readout_state() in
drm_atomic_sro_readout_and_compare() will always have actual->fb_damage_clips
set to NULL and actual->ignore_damage_clips set to false.

When a blocking atomic commit includes FB_DAMAGE_CLIPS (or comes through
drm_atomic_helper_dirtyfb()) or a driver sets ignore_damage_clips, won't
commit_tail() -> drm_atomic_sro_readout_and_compare() ->
drm_atomic_helper_plane_compare_state() report false state mismatches on
these two fields?

> +	STATE_CHECK_PTR(ret, p, plane->name, expected, actual, color_pipeline);

[ ... ]

> +bool drm_atomic_helper_crtc_compare_state(struct drm_crtc *crtc,
> +					  struct drm_printer *p,
> +					  struct drm_crtc_state *expected,
> +					  struct drm_crtc_state *actual)
> +{
> +	bool ret = true;
> +
> +	STATE_CHECK_PTR(ret, p, crtc->name, expected, actual, crtc);
> +	STATE_CHECK_BOOL(ret, p, crtc->name, expected, actual, enable);
> +	STATE_CHECK_BOOL(ret, p, crtc->name, expected, actual, active);
> +
> +	/* Both inactive, mode and properties are meaningless. */
> +	if (!expected->active && !actual->active)
> +		return ret;

[Severity: Medium]
Won't checking enable before the !expected->active && !actual->active early
return trigger a false mismatch when a CRTC is in DPMS off?

When userspace blanks a display via DPMS off
(drm_atomic_connector_commit_dpms() or an atomic commit setting ACTIVE=0
while keeping MODE_ID set), the committed state has expected->enable = true
and expected->active = false, while drm_atomic_helper_commit_crtc_disable()
powers down the CRTC hardware.

When commit_tail() -> drm_atomic_sro_readout_and_compare() reads back the
hardware state (for example via tidss_crtc_readout_state()), the hardware
CRTC is disabled so readout leaves both actual->enable = false and
actual->active = false.

Because STATE_CHECK_BOOL(ret, p, crtc->name, expected, actual, enable) runs
before the inactive check returns ret, ret is set to false and a mismatch is
logged even though both states are inactive. Should enable only be compared
after verifying that the CRTC is active?

> +
> +	/* One active and the other inactive, that's a mismatch. */
> +	if (!expected->active || !actual->active)
> +		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);
> +	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);
> +
> +	STATE_CHECK_BOOL(ret, p, conn->name, expected, actual, self_refresh_aware);
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, picture_aspect_ratio);
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, content_type);
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, hdcp_content_type);
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, scaling_mode);
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, content_protection);
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, colorspace);
> +
> +	/*
> +	 * NOTE: We can't check max_bpc, max_requested_bpc and
> +	 * color_format because they are userspace requests negotiated
> +	 * during atomic_check into output_bpc and output_format. The
> +	 * hardware only stores the negotiated result.
> +	 */
> +
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, privacy_screen_sw_state);
> +	STATE_CHECK_PROPERTY_BLOB(ret, p, conn->name, expected, actual, hdr_output_metadata);
> +
> +	STATE_CHECK_U32(ret, p, conn->name, expected, actual, hdmi.broadcast_rgb);

[Severity: Medium]
There appear to be two issues with how connector and HDMI state fields are
checked in drm_atomic_helper_connector_compare_state():

First, unlike drm_atomic_helper_plane_compare_state() (which returns early
when !expected->fb && !actual->fb) and drm_atomic_helper_crtc_compare_state()
(which returns early when !expected->active && !actual->active),
drm_atomic_helper_connector_compare_state() continues comparing all connector
and hdmi.* fields even when the connector is disabled (!expected->crtc &&
!actual->crtc). When a commit disables a previously active connector,
__drm_atomic_helper_connector_duplicate_state() copies the old state via
memcpy(), and drm_atomic_helper_connector_hdmi_check() returns early without
clearing the old hdmi.* fields:

drivers/gpu/drm/display/drm_hdmi_state_helper.c:drm_atomic_helper_connector_hdmi_check() {
	...
	if (!new_conn_state->crtc || !new_conn_state->best_encoder)
		return 0;

	ret = hdmi_compute_config(connector, new_conn_state, mode);
	if (ret)
		return ret;

	new_conn_state->hdmi.is_limited_range = hdmi_is_limited_range(connector, new_conn_state);
	...
}

Meanwhile, hardware readout creates a pristine default state via
atomic_create_state() and leaves it untouched when the connector is
disconnected from a CRTC (for example in sii902x_bridge_connector_readout()),
causing false mismatches on retained fields like hdmi.output_bpc,
hdmi.output_format, hdmi.tmds_char_rate, and hdmi.infoframes.*.

Second, even when the connector is active, hdmi.broadcast_rgb is a userspace
request (like max_bpc and color_format mentioned in the comment above) that
drm_atomic_helper_connector_hdmi_check() resolves into hdmi.is_limited_range
and the AVI infoframe quantization range. Since hardware only stores the
negotiated quantization range and atomic_create_state() initializes
actual->hdmi.broadcast_rgb to DRM_HDMI_BROADCAST_RGB_AUTO, won't setting
Broadcast RGB to Full or Limited always fail comparison?

> +	STATE_CHECK_BOOL(ret, p, conn->name, expected, actual, hdmi.infoframes.avi.set);
> +	STATE_CHECK_INFOFRAME(ret, p, conn->name, expected, actual, hdmi.infoframes.avi.data);
> +	STATE_CHECK_BOOL(ret, p, conn->name, expected, actual, hdmi.infoframes.hdr_drm.set);
> +	STATE_CHECK_INFOFRAME(ret, p, conn->name, expected, actual, hdmi.infoframes.hdr_drm.data);
> +	STATE_CHECK_BOOL(ret, p, conn->name, expected, actual, hdmi.infoframes.spd.set);
> +	STATE_CHECK_INFOFRAME(ret, p, conn->name, expected, actual, hdmi.infoframes.spd.data);
> +	STATE_CHECK_BOOL(ret, p, conn->name, expected, actual, hdmi.infoframes.hdmi.set);
> +	STATE_CHECK_INFOFRAME(ret, p, conn->name, expected, actual, hdmi.infoframes.hdmi.data);

[Severity: Medium]
Should STATE_CHECK_INFOFRAME() only be called when the corresponding
infoframe's set field is true in both expected and actual?

When a connector state is duplicated via
__drm_atomic_helper_connector_duplicate_state(), the previous
hdmi.infoframes.*.data bytes are copied with memcpy(). If an infoframe that
was previously enabled (such as hdr_drm when hdr_output_metadata is removed,
or the HDMI vendor infoframe) is not generated in a subsequent commit,
helpers like hdmi_generate_hdr_infoframe() and
hdmi_generate_hdmi_vendor_infoframe() only clear infoframe->set without
zeroing infoframe->data:

drivers/gpu/drm/display/drm_hdmi_state_helper.c:hdmi_generate_hdr_infoframe() {
	...
	infoframe->set = false;

	if (!connector->hdmi.funcs->hdr_drm.write_infoframe)
		return 0;

	if (connector->max_bpc < 10)
		return 0;

	if (!conn_state->hdr_output_metadata)
		return 0;
	...
}

In drm_atomic_sro_readout_and_compare(), the readout state is freshly
zero-allocated via atomic_create_state(), so actual->hdmi.infoframes.*.data
is all zeros when set is false, whereas expected->hdmi.infoframes.*.data
still holds stale bytes from the earlier commit. Comparing data with memcmp()
when set is false will report a mismatch even though the infoframe is
disabled in both states.

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