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/i915: Implement the "color format" DRM property Date: Wed, 25 Mar 2026 06:44:27 +1000 Message-ID: In-Reply-To: <20260324-color-format-v11-8-605559af4fb4@collabora.com> References: <20260324-color-format-v11-0-605559af4fb4@collabora.com> <20260324-color-format-v11-8-605559af4fb4@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 1. **Dropped `has_hdmi_sink` check in HDMI fallback path:** The existing co= de checks `!crtc_state->has_hdmi_sink` before falling back to YCbCr420, but= the new code drops this: ```c - if (crtc_state->sink_format =3D=3D INTEL_OUTPUT_FORMAT_YCBCR420 || - !crtc_state->has_hdmi_sink || - !connector->base.ycbcr_420_allowed || - !drm_mode_is_420_also(info, adjusted_mode)) + if (!connector->base.ycbcr_420_allowed) + return ret; ``` DVI sinks (non-HDMI) cannot do YCbCr420, but the `ycbcr_420_allowed` flag s= hould already prevent this. However, the original code had the `has_hdmi_si= nk` check as an additional safety guard. Removing it is probably fine since= `ycbcr_420_allowed` should never be set for DVI, but worth double-checking. 2. **`drm_mode_is_420_also` changed to `drm_mode_is_420`:** In the HDMI fal= lback, the old code checks `drm_mode_is_420_also()` and the new code uses `= drm_mode_is_420()` (which includes both 420-only and 420-also). Since we al= ready check `crtc_state->sink_format =3D=3D INTEL_OUTPUT_FORMAT_YCBCR420` (= i.e., we're not already in 420 mode), this is a semantic change but correct= =E2=80=94 if the mode is 420-only and we're somehow not already in 420, it= 's still valid to fall back to 420. 3. **Implicit reliance on `!req_fmt` meaning AUTO (value 0):** Throughout t= he i915 HDMI path, `!req_fmt` and `if (req_fmt)` are used to test for AUTO = vs non-AUTO. Since `DRM_CONNECTOR_COLOR_FORMAT_AUTO =3D 0`, this works, but= it's fragile if the enum ever changes. Recommend using explicit comparison= : `req_fmt =3D=3D DRM_CONNECTOR_COLOR_FORMAT_AUTO`. 4. **i915 DP-MST: `actual_format` potentially uninitialized:** In `mst_stre= am_compute_config`, the switch on `pipe_config->output_format`: ```c + switch (pipe_config->output_format) { + case INTEL_OUTPUT_FORMAT_RGB: + actual_format =3D DRM_CONNECTOR_COLOR_FORMAT_RGB444; + break; + case INTEL_OUTPUT_FORMAT_YCBCR444: + actual_format =3D DRM_CONNECTOR_COLOR_FORMAT_YCBCR444; + break; + case INTEL_OUTPUT_FORMAT_YCBCR420: + actual_format =3D DRM_CONNECTOR_COLOR_FORMAT_YCBCR420; + break; + } ``` There is no `default:` case. If `pipe_config->output_format` is an unexpect= ed value, `actual_format` will be used uninitialized. The compiler will lik= ely warn about this. Add a `default: return -EINVAL;` case. 5. **No YCbCr422 support in i915:** The `supported_colorformats` bitmask in= `intel_connector.c` omits `YCBCR422`. This is correct since i915 doesn't s= upport YCbCr422 output, and the property will correctly not list it. --- Generated by Claude Code Patch Reviewer