Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Logic Error / Fatal Initialization Error: Unhandled color block in `intel_color_pipeline_plane_add_colorop`. - [High] Hardware Configuration Error: `PLANE_COLOR_INPUT_CSC_ENABLE` is ignored for RGB framebuffers when `csc_ff_enable` is true. - [Low] Compilation Failure: Undeclared enum value `INTEL_PLANE_CB_CSC_FF`. - [Low] Compilation Failure: Undeclared struct member `csc_ff_enable`. --
commit 1c6f477c5724be8b1a87f5ec970f9c13fe79be33 Author: Chaitanya Kumar Borah <[email protected]> drm/i915/color: Add YUV buffer support on HDR planes This commit adds the INTEL_PLANE_CB_CSC_FF color block as the first stage for HDR planes to enable YUV-to-RGB color space conversion. It also updates icl_program_input_csc() to account for color pipeline programming. > diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c > b/drivers/gpu/drm/i915/display/intel_color_pipeline.c > index 6cf8080ee8000..71451a196e165 100644 > --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c > +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c [ ... ] > @@ -31,6 +31,7 @@ static const struct drm_colorop_funcs intel_colorop_funcs = > { > * the pipeline totally unusable. > */ > static const enum intel_color_block xe3plpd_primary_plane_pipeline[] = { > + INTEL_PLANE_CB_CSC_FF, [Severity: Low] Is INTEL_PLANE_CB_CSC_FF defined in this tree? It appears to be an undeclared enum value here, which will cause a compilation failure. > INTEL_PLANE_CB_PRE_CSC_LUT, > INTEL_PLANE_CB_CSC, [ ... ] > @@ -38,6 +39,7 @@ static const enum intel_color_block > xe3plpd_primary_plane_pipeline[] = { > }; > > static const enum intel_color_block hdr_plane_pipeline[] = { > + INTEL_PLANE_CB_CSC_FF, [Severity: High] How is this new color block handled during pipeline creation? Looking at intel_color_pipeline_plane_add_colorop(), there doesn't seem to be a case for INTEL_PLANE_CB_CSC_FF, meaning it will hit the default path: intel_color_pipeline_plane_add_colorop() { ... default: drm_err(plane->dev, "Invalid colorop id [%d]", id); ret = -EINVAL; ... } Will this cause pipeline creation to immediately abort and return -EINVAL when encountering this new block? > INTEL_PLANE_CB_PRE_CSC_LUT, > INTEL_PLANE_CB_CSC, [ ... ] > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > index 5cda1ab90e40f..c6ec77c38cc50 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c [ ... ] > @@ -1616,7 +1616,8 @@ icl_plane_update_noarm(struct intel_dsb *dsb, > intel_de_write_dsb(display, dsb, PLANE_COLOR_CTL(pipe, plane_id), > plane_color_ctl); > > - if (fb->format->is_yuv && icl_is_hdr_plane(display, plane_id)) > + if (icl_is_hdr_plane(display, plane_id) && > + (fb->format->is_yuv || plane_state->hw.csc_ff_enable)) [Severity: Low] Does struct intel_plane_state contain csc_ff_enable in its hw member? This looks like an undeclared struct member that will break the build. [Severity: High] When csc_ff_enable is explicitly requested for RGB buffers, will the hardware actually enable the CSC block? In glk_plane_color_ctl(), the enable bit is only set if the framebuffer format is YUV: glk_plane_color_ctl() { ... } else if (fb->format->is_yuv) { plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE; ... } Does this mean the fixed-function CSC hardware block remains disabled in PLANE_COLOR_CTL when we have an RGB framebuffer with csc_ff_enable set to true? > icl_program_input_csc(dsb, plane, plane_state); > > skl_write_plane_wm(dsb, plane, crtc_state); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
