From: sashiko-bot@kernel.org
To: "Maxime Ripard" <mripard@kernel.org>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v6 18/28] drm/atomic-state-helper: Provide atomic_compare_state helpers
Date: Thu, 15 Oct 2026 16:50:04 +0000 [thread overview]
Message-ID: <sashiko-outbox-173204@kernel.org> (raw)
In-Reply-To: <20261015-drm-state-readout-v6-18-6cc02cd10998@kernel.org>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] drm: atomic SRO state comparison spams on disabled objects
- [Low] drm/atomic-sro: STATE_CHECK_U32 and STATE_CHECK_BOOL cause static_assert build failures
- [Low] drm/atomic-sro: -Wformat compiler warning in STATE_CHECK_U32_16_16 macro
- [Low] drm: missing base state fields in atomic comparison helpers
--
commit a2faf4570ad763a12f621476faaa22b5502a3a73
Author: Maxime Ripard <mripard@kernel.org>
drm/atomic-state-helper: Provide atomic_compare_state helpers
Now that we have introduced some new infrastructure to compare state
instances, provide helpers for the default states.
> diff --git a/drivers/gpu/drm/drm_atomic_sro_helper.c b/drivers/gpu/drm/drm_atomic_sro_helper.c
> index 692a5a24d762c..46a733e4dcce5 100644
> --- a/drivers/gpu/drm/drm_atomic_sro_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_sro_helper.c
[ ... ]
> @@ -408,6 +409,245 @@ 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);
> +
> + if (expected->fb && actual->fb) {
> + if (!drm_atomic_helper_fb_compare(p, expected->fb, actual->fb))
> + ret = false;
> + } else 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");
> + ret = false;
> + }
> +
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, crtc_x);
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, crtc_y);
[Severity: Medium]
Will this comparison cause false-positive mismatches when the plane is
disabled?
If expected->fb is NULL, the expected state retains the old coordinates but
the actual state readout is zeroed. Since these properties are compared
unconditionally, could this lead to persistent mismatch warnings in the
kernel log?
> + STATE_CHECK_U32(ret, p, plane->name, expected, actual, crtc_w);
> + STATE_CHECK_U32(ret, p, plane->name, expected, actual, crtc_h);
> + STATE_CHECK_U32_16_16(ret, p, plane->name, expected, actual, src_x);
[Severity: Low]
This problem wasn't introduced by this patch, but using STATE_CHECK_U32_16_16
here triggers a -Wformat compiler warning.
The macro uses the %d format specifier for the result of a right shift on a
u32 (sa->f >> 16):
"expected %d.%06u, got %d.%06u", \
sa->f >> 16, ((sa->f & 0xffff) * 15625) >> 10, \
Since sa->f is required to be u32, the shift result is an unsigned int.
Passing an unsigned int to %d violates format string rules. Should this be
changed to %u?
> + STATE_CHECK_U32_16_16(ret, p, plane->name, expected, actual, src_y);
> + STATE_CHECK_U32_16_16(ret, p, plane->name, expected, actual, src_w);
> + STATE_CHECK_U32_16_16(ret, p, plane->name, expected, actual, src_h);
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, hotspot_x);
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, hotspot_y);
> + STATE_CHECK_U16(ret, p, plane->name, expected, actual, alpha);
> + STATE_CHECK_U16(ret, p, plane->name, expected, actual, pixel_blend_mode);
> + STATE_CHECK_U32(ret, p, plane->name, expected, actual, rotation);
> + STATE_CHECK_U32(ret, p, plane->name, expected, actual, zpos);
> + STATE_CHECK_U32(ret, p, plane->name, expected, actual, normalized_zpos);
> + STATE_CHECK_U32(ret, p, plane->name, expected, actual, color_encoding);
> + STATE_CHECK_U32(ret, p, plane->name, expected, actual, color_range);
> +
> + // TODO: damage clips
> +
> + STATE_CHECK_BOOL(ret, p, plane->name, expected, actual, ignore_damage_clips);
[Severity: Low]
This problem wasn't introduced by this patch, but using STATE_CHECK_BOOL here
causes a build failure.
The macro uses static_assert(__same_type(sa->f, true)). In the kernel, true
expands to 1 (an int). Comparing a bool with an int evaluates to false,
unconditionally failing the build. Should the macro compare against a boolean
variable or type instead?
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, src.x1);
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, src.x2);
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, src.y1);
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, src.y2);
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, dst.x1);
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, dst.x2);
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, dst.y1);
> + STATE_CHECK_S32(ret, p, plane->name, expected, actual, dst.y2);
> + STATE_CHECK_BOOL(ret, p, plane->name, expected, actual, visible);
> + STATE_CHECK_U32(ret, p, plane->name, expected, actual, scaling_filter);
> +
[Severity: Low]
Is the color_pipeline property intentionally omitted from the plane state
comparison?
If the hardware state mismatches the software state for this field, it seems
the discrepancy will be silently ignored.
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_plane_compare_state);
> +
[ ... ]
> +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);
> + STATE_CHECK_BOOL(ret, p, crtc->name, expected, actual, no_vblank);
> + STATE_CHECK_U32(ret, p, crtc->name, expected, actual, plane_mask);
> + STATE_CHECK_U32(ret, p, crtc->name, expected, actual, connector_mask);
> + STATE_CHECK_U32(ret, p, crtc->name, expected, actual, encoder_mask);
> +
> + STATE_CHECK_DISPLAY_MODE(ret, p, crtc->name, expected, actual, mode);
> + STATE_CHECK_DISPLAY_MODE(ret, p, crtc->name, expected, actual, adjusted_mode);
[Severity: Medium]
Will this comparison cause false-positive mismatches when the CRTC is inactive?
If expected->active is false, the expected mode may contain previous
user-space values while the actual hardware readout is zeroed. Since these
properties are compared unconditionally, could this cause persistent mismatch
spam in the kernel log?
> + STATE_CHECK_PROPERTY_BLOB(ret, p, crtc->name, expected, actual, mode_blob);
> + STATE_CHECK_PROPERTY_BLOB(ret, p, crtc->name, expected, actual, degamma_lut);
> + STATE_CHECK_PROPERTY_BLOB(ret, p, crtc->name, expected, actual, ctm);
> + STATE_CHECK_PROPERTY_BLOB(ret, p, crtc->name, expected, actual, gamma_lut);
> + STATE_CHECK_BOOL(ret, p, crtc->name, expected, actual, vrr_enabled);
> + STATE_CHECK_BOOL(ret, p, crtc->name, expected, actual, self_refresh_active);
> + STATE_CHECK_U32(ret, p, crtc->name, expected, actual, scaling_filter);
> +
[Severity: Low]
Is the background_color property intentionally omitted from the CRTC state
comparison?
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_crtc_compare_state);
> +
[ ... ]
> +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);
[Severity: Low]
This problem wasn't introduced by this patch, but using STATE_CHECK_U32 on an
enum field causes a build failure.
The STATE_CHECK_U32 macro uses static_assert(__same_type(sa->f, (u32)0)).
In C, __builtin_types_compatible_p considers enum types incompatible with
integer types like u32, so this assertion will unconditionally fail when
checking fields like 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 and max_requested_bpc because it
> + * will typically come from userspace and we can't read it out
> + * from the hardware.
> + */
> +
> + 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);
> + 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);
> + STATE_CHECK_BOOL(ret, p, conn->name, expected, actual, hdmi.is_limited_range);
> + STATE_CHECK_U32(ret, p, conn->name, expected, actual, hdmi.output_bpc);
> + STATE_CHECK_U32(ret, p, conn->name, expected, actual, hdmi.output_format);
> + STATE_CHECK_U64(ret, p, conn->name, expected, actual, hdmi.tmds_char_rate);
> +
[Severity: Low]
Are color_format and hdmi.scrambler_needed intentionally omitted from the
connector state comparison?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=18
next prev parent reply other threads:[~2026-10-15 16:50 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-15 15:08 [PATCH v6 00/28] drm: Implement state readout support Maxime Ripard
2026-10-15 15:08 ` [PATCH v6 01/28] drm/atomic: Fix private obj error handling Maxime Ripard
2026-10-15 15:23 ` sashiko-bot
2026-10-15 15:08 ` [PATCH v6 02/28] drm/atomic: Add drm_private_obj name Maxime Ripard
2026-10-15 15:23 ` sashiko-bot
2026-10-15 15:08 ` [PATCH v6 03/28] drm/bridge: Switch print_state to use bridge name Maxime Ripard
2026-10-15 15:34 ` sashiko-bot
2026-10-15 15:08 ` [PATCH v6 04/28] drm/bridge: Add drm_private_obj_is_bridge() Maxime Ripard
2026-10-15 15:28 ` sashiko-bot
2026-10-15 15:08 ` [PATCH v6 05/28] drm/atomic: Export drm_atomic_*_print_state Maxime Ripard
2026-10-15 15:33 ` sashiko-bot
2026-10-15 15:08 ` [PATCH v6 06/28] drm/atomic_sro: Create drm_atomic_sro_state container Maxime Ripard
2026-10-15 15:41 ` sashiko-bot
2026-10-15 15:08 ` [PATCH v6 07/28] drm/atomic_sro: Create kernel parameter to force or disable readout Maxime Ripard
2026-10-15 15:46 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 08/28] drm/atomic_sro: Add atomic state readout infrastructure Maxime Ripard
2026-10-15 15:52 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 09/28] drm/atomic_sro: Add function to install state into drm objects Maxime Ripard
2026-10-15 15:56 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 10/28] drm/atomic_sro: Create documentation Maxime Ripard
2026-10-15 15:59 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 11/28] drm/bridge: Handle bridges with hardware state readout Maxime Ripard
2026-10-15 16:05 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 12/28] drm/mode-config: Read out hardware state in drm_mode_config_create_state Maxime Ripard
2026-10-15 16:14 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 13/28] drm/atomic_sro: Provide helpers to implement hardware state readout Maxime Ripard
2026-10-15 16:21 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 14/28] drm/mode-config: Introduce hw_reset hook Maxime Ripard
2026-10-15 15:09 ` [PATCH v6 15/28] drm/modeset-helper: Add drm_mode_config_helper_hw_reset() Maxime Ripard
2026-10-15 15:09 ` [PATCH v6 16/28] drm/atomic_helper: Compare actual and readout states once the commit is done Maxime Ripard
2026-10-15 16:31 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 17/28] drm/atomic-state-helper: Provide comparison macros Maxime Ripard
2026-10-15 16:39 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 18/28] drm/atomic-state-helper: Provide atomic_compare_state helpers Maxime Ripard
2026-10-15 16:50 ` sashiko-bot [this message]
2026-10-15 15:09 ` [PATCH v6 19/28] drm/atomic-helper: Document State Read-Out as a state initialization approach Maxime Ripard
2026-10-15 16:53 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 20/28] drm/encoder: Create atomic_sro_get_current_crtc hook Maxime Ripard
2026-10-15 15:09 ` [PATCH v6 21/28] drm/bridge: display-connector: Implement readout support Maxime Ripard
2026-10-15 16:59 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 22/28] drm/bridge_connector: Implement hw readout for connector Maxime Ripard
2026-10-15 16:59 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 23/28] drm/tidss: crtc: Add atomic_print_state implementation Maxime Ripard
2026-10-15 15:09 ` [PATCH v6 24/28] drm/tidss: dispc: Improve mode checking logs Maxime Ripard
2026-10-15 17:02 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 25/28] drm/tidss: Implement hw_reset Maxime Ripard
2026-10-15 17:09 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 26/28] drm/tidss: Implement readout support Maxime Ripard
2026-10-15 17:13 ` sashiko-bot
2026-10-15 15:09 ` [PATCH v6 27/28] drm/tidss: encoder: Implement atomic_sro_get_current_crtc Maxime Ripard
2026-10-15 15:09 ` [PATCH v6 28/28] drm/bridge: sii902x: Implement hw state readout Maxime Ripard
2026-10-15 17:20 ` sashiko-bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sashiko-outbox-173204@kernel.org \
--to=sashiko-bot@kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=mripard@kernel.org \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.