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: Mon, 25 May 2026 18:48:53 +1000 Message-ID: In-Reply-To: <20260522-color-format-v15-3-21fb136c9df2@collabora.com> References: <20260522-color-format-v15-0-21fb136c9df2@collabora.com> <20260522-color-format-v15-3-21fb136c9df2@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 Core patch. A few observations: The two separate enums (`drm_output_color_format` for internal use, `drm_co= nnector_color_format` for the property with AUTO) make sense and avoid poll= uting the existing enum. The `drm_connector_attach_color_format_property` function has connector-typ= e validation: ```c switch (connector->connector_type) { case DRM_MODE_CONNECTOR_HDMIA: case DRM_MODE_CONNECTOR_HDMIB: if (supported_color_formats & ~hdmi_colorformats) { ... } break; case DRM_MODE_CONNECTOR_DisplayPort: case DRM_MODE_CONNECTOR_eDP: if (supported_color_formats & ~dp_colorformats) { ... } break; } ``` Note that connector types not listed (e.g. DSI, LVDS) silently pass through= with no validation. This is presumably intentional to allow future expansi= on, but it means a driver could accidentally register unsupported formats o= n those connector types. A comment noting this is deliberate would help. Currently `hdmi_colorformats` and `dp_colorformats` are identical bitmasks.= If they're always going to be the same, they could be one constant =E2=80= =94 though keeping them separate makes sense if they'll diverge in the futu= re. The `drm_hdmi_connector_get_output_format_name()` call for the enum_list na= mes is clever reuse, but it means the property string names come from a fun= ction originally designed for debug output format names. Worth confirming t= hese are the exact strings documented ("RGB", "YUV 4:4:4", etc.). Minor: the `enum_list` array is stack-allocated at `DRM_CONNECTOR_COLOR_FOR= MAT_COUNT` (6 elements) which is fine, but the index `i` is pre-incremented= in the loop (`enum_list[++i]`), which always skips index 0 (used for AUTO)= . This is correct but somewhat unusual =E2=80=94 a comment would clarify. --- Generated by Claude Code Patch Reviewer