Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Hardware YUV-to-RGB conversion (Input CSC) is incorrectly disabled for HDR planes when a color pipeline is active. - [Medium] For SDR planes, when a color pipeline is active, the driver fails to set the PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE bit for full-range YCbCr-to-RGB matrix conversions. --
--- Patch [6]: [CI v4 06/14] drm/i915/display: Program input CSC on SDR planes --- commit 840c01156b413fde2fac882f08e842c50840876f Author: Chaitanya Kumar Borah <[email protected]> Subject: drm/i915/display: Program input CSC on SDR planes This commit programs the SDR plane fixed-function CSC when a color pipeline is active. It also ensures the legacy color_encoding/color_range properties remain mutually exclusive with the color pipeline uAPI. > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > index da2df5be9848d..0e951b36cccad 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c [ ... ] > @@ -1240,33 +1240,63 @@ static u32 glk_plane_color_ctl_crtc(const struct > intel_crtc_state *crtc_state) [ ... ] > static u32 glk_plane_color_ctl_input_csc(const struct intel_plane_state > *plane_state) > { > struct intel_display *display = to_intel_display(plane_state); > const struct drm_framebuffer *fb = plane_state->hw.fb; > struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); > + bool color_pipeline = plane_state->uapi.state && > + plane_state->uapi.state->plane_color_pipeline; > u32 ctl = 0; > > - if (!fb->format->is_yuv) > - return 0; > + if (!color_pipeline) { > + if (!fb->format->is_yuv) > + return 0; > > - if (!icl_is_hdr_plane(display, plane->id)) { > - switch (plane_state->hw.color_encoding) { > - case DRM_COLOR_YCBCR_BT709: > - ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; > - break; > - case DRM_COLOR_YCBCR_BT2020: > - ctl |= PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020; > - break; > - default: > - ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB601; > + if (!icl_is_hdr_plane(display, plane->id)) { > + switch (plane_state->hw.color_encoding) { [ ... ] > + default: > + ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB601; > + } > + } else { > + ctl |= PLANE_COLOR_INPUT_CSC_ENABLE; > } > - } else { > - ctl |= PLANE_COLOR_INPUT_CSC_ENABLE; > - } > > - if (plane_state->hw.color_range == DRM_COLOR_YCBCR_FULL_RANGE) > - ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; > + if (plane_state->hw.color_range == DRM_COLOR_YCBCR_FULL_RANGE) > + ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; [Severity: High] Does this logic inadvertently disable hardware YUV-to-RGB conversion for HDR planes when a color pipeline is active? When color_pipeline is true for an HDR plane, the logic in glk_plane_color_ctl_input_csc() skips the (!color_pipeline) branch entirely. However, the else-if block below only handles SDR planes, which causes the function to return 0 and fail to set the PLANE_COLOR_INPUT_CSC_ENABLE bit for HDR planes. Since the hardware coefficients might still be programmed in icl_plane_update_noarm(): if (fb->format->is_yuv && icl_is_hdr_plane(display, plane_id)) icl_program_input_csc(dsb, plane, plane_state); Could this allow raw YUV data to flow directly into the RGB pipeline without conversion, causing visual corruption when a YUV framebuffer is assigned to an HDR plane? > + } else if (!icl_is_hdr_plane(display, plane->id)) { > + if (plane_state->hw.csc_ff_enable) > + ctl |= > intel_csc_ff_type_to_csc_mode(plane_state->hw.csc_ff_type); [Severity: Medium] For SDR planes with an active color pipeline, does this miss setting the PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE bit for full-range YCbCr-to-RGB conversions? If full-range YCbCr input is processed without this bit, would the hardware's default limited-to-full range correction be applied before the full-range matrix, causing color distortion? Even if this path is currently unreachable in practice because the color pipeline property isn't exposed on SDR planes, should the disable bit still be explicitly set when appropriate to prevent future issues? > + } > > return ctl; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
