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/amdgpu: Implement "color format" DRM property Date: Wed, 25 Mar 2026 06:44:28 +1000 Message-ID: In-Reply-To: <20260324-color-format-v11-9-605559af4fb4@collabora.com> References: <20260324-color-format-v11-0-605559af4fb4@collabora.com> <20260324-color-format-v11-9-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 1. **`PIXEL_ENCODING_UNDEFINED` approach:** When a non-AUTO format can't be satisfied, the code sets `timing_out->pixel_encoding = PIXEL_ENCODING_UNDEFINED`, relying on downstream DC validation to reject it. This is a somewhat indirect error path: ```c + if (connector_state->color_format != DRM_CONNECTOR_COLOR_FORMAT_AUTO) + timing_out->pixel_encoding = PIXEL_ENCODING_UNDEFINED; ``` The `dm_validate_stream_color_format()` later catches the mismatch explicitly, which is good. But the UNDEFINED encoding path could potentially cause unexpected behavior in DC code paths before validation catches it. 2. **`dm_validate_stream_color_format` has redundant AUTO check:** The function first checks `if (!drm_state->color_format)` (AUTO=0), then in the switch has `case DRM_CONNECTOR_COLOR_FORMAT_AUTO:` mapping to RGB encoding. The early return makes the switch case dead code. Not a bug, but confusing. 3. **`force_yuv420_output` and `force_yuv422_output` interaction:** The patch preserves the legacy `force_yuv*_output` flags alongside the new color_format property. If both are set, the `want_420`/`want_422` OR logic means either trigger can activate the format. This could lead to confusing behavior if both are active simultaneously. 4. **Duplicate `supported_colorformats` definition:** Both `amdgpu_dm.c` and `amdgpu_dm_mst_types.c` define identical `supported_colorformats` constants. Consider a shared header definition. --- Generated by Claude Code Patch Reviewer