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/amdgpu: Implement "color format" DRM property
Date: Sun, 12 Apr 2026 11:01:41 +1000
Message-ID: <review-patch8-20260409-color-format-v12-8-ce84e1817a27@collabora.com>
In-Reply-To: <20260409-color-format-v12-8-ce84e1817a27@collabora.com>
References: <20260409-color-format-v12-0-ce84e1817a27@collabora.com> <20260409-color-format-v12-8-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

Major rework of the pixel encoding logic in `fill_stream_properties_from_drm_display_mode()`. The new logic:

```c
want_420 = (aconnector && aconnector->force_yuv420_output) ||
	   (connector_state->color_format == DRM_CONNECTOR_COLOR_FORMAT_YCBCR420);
want_422 = (aconnector && aconnector->force_yuv422_output) ||
	   (connector_state->color_format == DRM_CONNECTOR_COLOR_FORMAT_YCBCR422);
```

**Notes**:

1. The fallback `else` branch is complex:
```c
} else {
	if (connector_state->color_format != DRM_CONNECTOR_COLOR_FORMAT_AUTO)
		timing_out->pixel_encoding = PIXEL_ENCODING_UNDEFINED;
	else if (drm_mode_is_420_only(info, mode_in))
		timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
	else
		timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
}
```
Setting `PIXEL_ENCODING_UNDEFINED` to signal an error is an unusual pattern. It relies on `dm_validate_stream_color_format()` catching this later. This works but could be made more explicit with a comment explaining the intentional error-signaling mechanism.

2. `dm_validate_stream_color_format()` has a subtle issue:
```c
if (!drm_state->color_format)
	return DC_OK;
```
Since `DRM_CONNECTOR_COLOR_FORMAT_AUTO = 0`, this correctly short-circuits for AUTO. But it relies on the numeric value of AUTO being 0, which is an implicit coupling. A named constant comparison would be clearer:
```c
if (drm_state->color_format == DRM_CONNECTOR_COLOR_FORMAT_AUTO)
```

3. MST connectors are correctly excluded via the `!aconnector->mst_root` check. Good.

4. The `supported_colorformats` mask includes all four formats (RGB444|YCBCR444|YCBCR422|YCBCR420), which is appropriate for amdgpu.

**Verdict**: Acceptable, but the `!drm_state->color_format` check (line 2238) should use the named enum value. The PIXEL_ENCODING_UNDEFINED error-signaling mechanism deserves a comment.

---

---
Generated by Claude Code Patch Reviewer