From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm: Add CRTC background color property Date: Wed, 04 Mar 2026 06:54:47 +1000 Message-ID: In-Reply-To: <20260303-rk3588-bgcolor-v8-2-fee377037ad1@collabora.com> References: <20260303-rk3588-bgcolor-v8-0-fee377037ad1@collabora.com> <20260303-rk3588-bgcolor-v8-2-fee377037ad1@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Status: Looks good** This is the core patch and it's well done. **Property creation and plumbing**: The property is created as a standard property in `drm_mode_create_standard_properties()` with range `[0, U64_MAX]`, and attached to individual CRTCs via `drm_crtc_attach_background_color_property()`. This follows the established DRM pattern (similar to color management properties). The get/set in `drm_atomic_uapi.c` and state dump in `drm_atomic.c` are correct. **Default initialization**: Setting the default to opaque black in `__drm_atomic_helper_crtc_state_reset()` is correct: ```c + crtc_state->background_color = DRM_ARGB64_PREP(0xffff, 0, 0, 0); ``` **ARGB64 macros**: The helper design is well thought out: - `DRM_ARGB64_PREP()` / `DRM_ARGB64_PREP_BPC()` for packing - `DRM_ARGB64_GET*()` / `DRM_ARGB64_GET*_BPC()` / `DRM_ARGB64_GET*_BPCS()` for extraction - The BPC variants use proper rounding (`DIV_ROUND_CLOSEST`) for accuracy - The BPCS variants use simple right-shift for performance The scaling math is correct: `DIV_ROUND_CLOSEST(c * 0xFFFF, (2^bpc - 1))` properly maps the input range to 16-bit. I verified the intermediate multiplication won't overflow since the `__u16` operands promote to at least `unsigned long` when multiplied with `__GENMASK(15, 0)`. **Documentation**: The updated kerneldoc in `drm_blend.c` clearly describes the property semantics, including the CRTC degamma/CSC/gamma pipeline placement and the note about alpha behavior. Good. Minor: the state dump format `%llx` works but `0x%016llx` would produce more consistently readable output for a 64-bit ARGB value. --- --- Generated by Claude Code Patch Reviewer