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 18:48:55 +1000 Message-ID: In-Reply-To: <20260522-color-format-v15-10-21fb136c9df2@collabora.com> References: <20260522-color-format-v15-0-21fb136c9df2@collabora.com> <20260522-color-format-v15-10-21fb136c9df2@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 implementation in `fill_stream_properties_from_drm_display_mode` is the most complex driver integration. A concern: ```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 trigger an error bubble-up is creative but could be fragile. If any downstream code doesn't handle UNDEFINED gracefully, it could cause unexpected behavior. The validation in `dm_validate_stream_color_format` should catch this, but the path between setting UNDEFINED and the validation check is worth auditing. In `dm_validate_stream_color_format`: ```c if (!drm_state->color_format) return DC_OK; ``` This checks if `color_format` is falsy (== 0), which is `DRM_CONNECTOR_COLOR_FORMAT_AUTO`. But the switch below *also* has `case DRM_CONNECTOR_COLOR_FORMAT_AUTO` mapping to `PIXEL_ENCODING_RGB`. The early return means the AUTO case in the switch is dead code. This is functionally fine (AUTO returns OK early) but the dead switch case is misleading. The property registration excludes MST connectors (`if (!aconnector->mst_root)`), which is documented in the cover letter as intentional. --- Generated by Claude Code Patch Reviewer