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 new general DRM property "color format" Date: Wed, 25 Mar 2026 06:44:26 +1000 Message-ID: In-Reply-To: <20260324-color-format-v11-3-605559af4fb4@collabora.com> References: <20260324-color-format-v11-0-605559af4fb4@collabora.com> <20260324-color-format-v11-3-605559af4fb4@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 This is the core UAPI patch. A few observations: 1. **Redundant definitions:** `hdmi_colorformats` and `dp_colorformats` are defined identically: ```c +static const u32 hdmi_colorformats = + BIT(DRM_OUTPUT_COLOR_FORMAT_RGB444) | + BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR444) | + BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR422) | + BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR420); + +static const u32 dp_colorformats = + BIT(DRM_OUTPUT_COLOR_FORMAT_RGB444) | + BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR444) | + BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR422) | + BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR420); ``` These could be a single definition. If they're intended to diverge in the future, a comment explaining that would help. 2. **Implicit reliance on enum value 0 for AUTO:** The `drm_atomic_connector_set_property` uses `val > INT_MAX` check, which is fine since `val` is `uint64_t`, but the `drm_connector_color_format_valid()` validator already covers this. The double-check is harmless but redundant. 3. **`drm_connector_attach_color_format_property` naming for enum list:** The function uses `drm_hdmi_connector_get_output_format_name(fmt)` to name the enum entries, but that function takes `enum hdmi_colorspace` on current drm-next (takes `enum drm_output_color_format` after the base the series depends on). Worth ensuring the dependency is clear. 4. The `connector_type` validation switch in `drm_connector_attach_color_format_property` has no `default:` case, so other connector types can register arbitrary format sets without validation. This seems intentional (other connector types are free to support whatever they declare), but is worth noting. --- Generated by Claude Code Patch Reviewer