From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm: Add new general DRM property "color format"
Date: Sun, 12 Apr 2026 11:01:40 +1000
Message-ID: <review-patch3-20260409-color-format-v12-3-ce84e1817a27@collabora.com>
In-Reply-To: <20260409-color-format-v12-3-ce84e1817a27@collabora.com>
References: <20260409-color-format-v12-0-ce84e1817a27@collabora.com> <20260409-color-format-v12-3-ce84e1817a27@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

The core patch. Introduces the userspace-facing enum:

```c
enum drm_connector_color_format {
	DRM_CONNECTOR_COLOR_FORMAT_AUTO = 0,
	DRM_CONNECTOR_COLOR_FORMAT_RGB444,
	DRM_CONNECTOR_COLOR_FORMAT_YCBCR444,
	DRM_CONNECTOR_COLOR_FORMAT_YCBCR422,
	DRM_CONNECTOR_COLOR_FORMAT_YCBCR420,
	DRM_CONNECTOR_COLOR_FORMAT_COUNT,
};
```

And extends `drm_output_color_format` with `DRM_OUTPUT_COLOR_FORMAT_COUNT`.

**Notes**:

1. The property validation in `drm_atomic_uapi.c` uses a `val > INT_MAX` check before calling `drm_connector_color_format_valid()`, which is a belt-and-suspenders approach since val is u64. Fine.

2. The `drm_connector_attach_color_format_property()` function validates supported formats against connector type. However:
```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 are **identical**. Either collapse them into one constant or add a comment explaining why they're kept separate (future divergence). As-is, `dp_colorformats` is dead weight.

3. The `drm_connector_color_format_valid()` inline function is marked `__pure`, which is correct -- it has no side effects and depends only on its input.

4. In `drm_atomic_helper_check_modeset()`, `color_format` changes trigger `connectors_changed = true`, matching the pattern used by `max_requested_bpc`. Good.

5. The documentation is thorough, including both the enum-level kerneldoc and the drm-kms.rst integration.

R-b from Maxime Ripard.

**Verdict**: Good. Consider unifying the identical colorformat masks.

---

---
Generated by Claude Code Patch Reviewer