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 <[email protected]> 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/[email protected]?part=18
