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: Mon, 25 May 2026 17:26:10 +1000 Message-ID: In-Reply-To: <20260523-color-format-v16-10-24340c5e4732@collabora.com> References: <20260523-color-format-v16-0-24340c5e4732@collabora.com> <20260523-color-format-v16-10-24340c5e4732@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review The main format selection logic in `fill_stream_properties_from_drm_display= _mode()` is well-structured but has an issue worth noting: **Issue:** The final `else` block (lines 6943-6955) can reach `PIXEL_ENCODI= NG_UNDEFINED` for a non-AUTO explicitly requested format that didn't match = any earlier branch. But it can *also* reach `PIXEL_ENCODING_RGB` for AUTO o= n a non-420-only mode. The `PIXEL_ENCODING_UNDEFINED` path is intentional = =E2=80=94 it forces `dm_validate_stream_color_format()` to catch the mismat= ch. However, the logic is subtle: `dm_validate_stream_color_format()` at li= ne 8312 does `if (!drm_state->color_format) return DC_OK` =E2=80=94 since `= DRM_CONNECTOR_COLOR_FORMAT_AUTO =3D=3D 0`, this means AUTO always passes th= e validation, which is correct but relies on the numeric value of the enum.= Worth a `/* AUTO =3D=3D 0 */` comment. **Minor:** `want_420` uses `aconnector->force_yuv420_output` OR `color_form= at =3D=3D YCBCR420`, but the `force_yuv420_output` path is the legacy amdgp= u-specific mechanism. This backward compatibility is fine but the two mecha= nisms could potentially conflict if both are set with different intentions. The `dm_validate_stream_color_format` is a good belt-and-suspenders check. --- Generated by Claude Code Patch Reviewer