>-----Original Message----- >From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] >Sent: Wednesday, October 24, 2018 4:18 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >Subject: Re: [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB >Conversion > >Op 23-10-18 om 22:11 schreef Uma Shankar: >> Plane input CSC needs to be enabled to convert frambuffers from YUV to >> RGB. This is needed for bottom 3 planes on ICL, rest of the planes >> have hardcoded conversion and taken care by the legacy code. >> >> This patch defines the plane input csc registers and co-efficient >> values for YUV to RGB conversion in BT709 and BT601 formats. It >> programs the coefficients and enables the plane input csc unit in >> hardware. >> >> Note: This is currently untested and floated to get an early feedback >> on the design and implementation for this feature. In parallel, I will >> test this on actual ICL hardware and confirm with planar formats. >> >> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 243 >+++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++- >> 2 files changed, 313 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h >> b/drivers/gpu/drm/i915/i915_reg.h index 8bd61f9..f0f6f7c 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -6553,6 +6553,7 @@ enum { >> #define _PLANE_COLOR_CTL_3_A 0x703CC /* GLK+ */ >> #define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30) /* >Pre-ICL */ >> #define PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE (1 << 28) >> +#define PLANE_COLOR_INPUT_CSC_ENABLE (1 << 20) /* Pre-ICL */ >> #define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23) /* Pre-ICL */ >> #define PLANE_COLOR_CSC_MODE_BYPASS (0 << 17) >> #define PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709 (1 << >17) >> @@ -6569,6 +6570,248 @@ enum { >> #define _PLANE_NV12_BUF_CFG_1_A 0x70278 >> #define _PLANE_NV12_BUF_CFG_2_A 0x70378 >> >> +/* Input CSC Register Definitions */ >> +#define _PLANE_INPUT_CSC_RY_GY_1_A 0x701E0 >> +#define _PLANE_INPUT_CSC_RY_GY_2_A 0x702E0 >> +#define _PLANE_INPUT_CSC_RY_GY_3_A 0x703E0 >> + >> +#define _PLANE_INPUT_CSC_RY_GY_1_B 0x711E0 >> +#define _PLANE_INPUT_CSC_RY_GY_2_B 0x712E0 >> +#define _PLANE_INPUT_CSC_RY_GY_3_B 0x713E0 >> + >> +#define _PLANE_INPUT_CSC_RY_GY_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_RY_GY_1_A, \ >> + _PLANE_INPUT_CSC_RY_GY_1_B) >> +#define _PLANE_INPUT_CSC_RY_GY_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_RY_GY_2_A, \ >> + _PLANE_INPUT_CSC_RY_GY_2_B) >> +#define PLANE_INPUT_CSC_RY_GY(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \ >> + _PLANE_INPUT_CSC_RY_GY_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_BY_1_A 0x701E4 >> +#define _PLANE_INPUT_CSC_BY_2_A 0x702E4 >> +#define _PLANE_INPUT_CSC_BY_3_A 0x703E4 >> + >> +#define _PLANE_INPUT_CSC_BY_1_B 0x711E4 >> +#define _PLANE_INPUT_CSC_BY_2_B 0x712E4 >> +#define _PLANE_INPUT_CSC_BY_3_B 0x713E4 >> + >> +#define _PLANE_INPUT_CSC_BY_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_1_A, \ >> + _PLANE_INPUT_CSC_BY_1_B) >> +#define _PLANE_INPUT_CSC_BY_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_BY_2_A, \ >> + _PLANE_INPUT_CSC_BY_2_B) >> +#define PLANE_INPUT_CSC_BY(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \ >> + _PLANE_INPUT_CSC_BY_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_RU_GU_1_A 0x701E8 >> +#define _PLANE_INPUT_CSC_RU_GU_2_A 0x702E8 >> +#define _PLANE_INPUT_CSC_RU_GU_3_A 0x703E8 >> + >> +#define _PLANE_INPUT_CSC_RU_GU_1_B 0x711E8 >> +#define _PLANE_INPUT_CSC_RU_GU_2_B 0x712E8 >> +#define _PLANE_INPUT_CSC_RU_GU_3_B 0x713E8 >> + >> +#define _PLANE_INPUT_CSC_RU_GU_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_1_A, \ >> + _PLANE_INPUT_CSC_RU_GU_1_B) >> +#define _PLANE_INPUT_CSC_RU_GU_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_2_A, \ >> + _PLANE_INPUT_CSC_RU_GU_2_B) >> +#define PLANE_INPUT_CSC_RU_GU(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \ >> + _PLANE_INPUT_CSC_RU_GU_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_BU_1_A 0x701EC >> +#define _PLANE_INPUT_CSC_BU_2_A 0x702EC >> +#define _PLANE_INPUT_CSC_BU_3_A 0x703EC >> + >> +#define _PLANE_INPUT_CSC_BU_1_B 0x711EC >> +#define _PLANE_INPUT_CSC_BU_2_B 0x712EC >> +#define _PLANE_INPUT_CSC_BU_3_B 0x713EC >> + >> +#define _PLANE_INPUT_CSC_BU_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_1_A, \ >> + _PLANE_INPUT_CSC_BU_1_B) >> +#define _PLANE_INPUT_CSC_BU_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_BU_2_A, \ >> + _PLANE_INPUT_CSC_BU_2_B) >> +#define PLANE_INPUT_CSC_BU(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \ >> + _PLANE_INPUT_CSC_BU_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_RV_GV_1_A 0x701F0 >> +#define _PLANE_INPUT_CSC_RV_GV_2_A 0x702F0 >> +#define _PLANE_INPUT_CSC_RV_GV_3_A 0x703F0 >> + >> +#define _PLANE_INPUT_CSC_RV_GV_1_B 0x711F0 >> +#define _PLANE_INPUT_CSC_RV_GV_2_B 0x712F0 >> +#define _PLANE_INPUT_CSC_RV_GV_3_B 0x713F0 >> + >> +#define _PLANE_INPUT_CSC_RV_GV_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_1_A, \ >> + _PLANE_INPUT_CSC_RV_GV_1_B) >> +#define _PLANE_INPUT_CSC_RV_GV_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_2_A, \ >> + _PLANE_INPUT_CSC_RV_GV_2_B) >> +#define PLANE_INPUT_CSC_RV_GV(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \ >> + _PLANE_INPUT_CSC_RV_GV_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_BV_1_A 0x701F4 >> +#define _PLANE_INPUT_CSC_BV_2_A 0x702F4 >> +#define _PLANE_INPUT_CSC_BV_3_A 0x703F4 >> + >> +#define _PLANE_INPUT_CSC_BV_1_B 0x711F4 >> +#define _PLANE_INPUT_CSC_BV_2_B 0x712F4 >> +#define _PLANE_INPUT_CSC_BV_3_B 0x713F4 >> + >> +#define _PLANE_INPUT_CSC_BV_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_1_A, \ >> + _PLANE_INPUT_CSC_BV_1_B) >> +#define _PLANE_INPUT_CSC_BV_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_BV_2_A, \ >> + _PLANE_INPUT_CSC_BV_2_B) >> +#define PLANE_INPUT_CSC_BV(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \ >> + _PLANE_INPUT_CSC_BV_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_A 0x701F8 >> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_A 0x702F8 >> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_A 0x703F8 >> + >> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_B 0x711F8 >> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_B 0x712F8 >> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_B 0x713F8 >> + >> +#define _PLANE_INPUT_CSC_PREOFF_HI_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_HI_1_A, \ >> + _PLANE_INPUT_CSC_PREOFF_HI_1_B) >> +#define _PLANE_INPUT_CSC_PREOFF_HI_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_HI_2_A, \ >> + _PLANE_INPUT_CSC_PREOFF_HI_2_B) >> +#define PLANE_INPUT_CSC_PREOFF_HI(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe), \ >> + _PLANE_INPUT_CSC_PREOFF_HI_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_A 0x701FC >> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_A 0x702FC >> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_A 0x703FC >> + >> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_B 0x711FC >> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_B 0x712FC >> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_B 0x713FC >> + >> +#define _PLANE_INPUT_CSC_PREOFF_ME_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_1_A, \ >> + _PLANE_INPUT_CSC_PREOFF_ME_1_B) >> +#define _PLANE_INPUT_CSC_PREOFF_ME_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_2_A, \ >> + _PLANE_INPUT_CSC_PREOFF_ME_2_B) >> +#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \ >> + _PLANE_INPUT_CSC_PREOFF_ME_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_A 0x70200 >> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_A 0x70300 >> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_A 0x70400 >> + >> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_B 0x71200 >> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_B 0x71300 >> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_B 0x71400 >> + >> +#define _PLANE_INPUT_CSC_PREOFF_LO_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_1_A, \ >> + _PLANE_INPUT_CSC_PREOFF_LO_1_B) >> +#define _PLANE_INPUT_CSC_PREOFF_LO_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_2_A, \ >> + _PLANE_INPUT_CSC_PREOFF_LO_2_B) >> +#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe), \ >> + _PLANE_INPUT_CSC_PREOFF_LO_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A 0x70204 >> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A 0x70304 >> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_A 0x70404 >> + >> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B 0x71204 >> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B 0x71304 >> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_B 0x71404 >> + >> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_HI_1_A, \ >> + _PLANE_INPUT_CSC_POSTOFF_HI_1_B) >> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_HI_2_A, \ >> + _PLANE_INPUT_CSC_POSTOFF_HI_2_B) >> +#define PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \ >> + _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_A 0x70208 >> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_A 0x70308 >> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_A 0x70408 >> + >> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_B 0x71208 >> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_B 0x71308 >> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_B 0x71408 >> + >> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_1_A, \ >> + _PLANE_INPUT_CSC_POSTOFF_ME_1_B) >> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_2_A, \ >> + _PLANE_INPUT_CSC_POSTOFF_ME_2_B) >> +#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \ >> + _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe)) >> + >> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_A 0x7020C >> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_A 0x7030C >> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_A 0x7040C >> + >> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_B 0x7120C >> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_B 0x7130C >> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_B 0x7140C >> + >> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_1_A, \ >> + _PLANE_INPUT_CSC_POSTOFF_LO_1_B) >> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe) \ >> + _PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_2_A, \ >> + _PLANE_INPUT_CSC_POSTOFF_LO_2_B) >> +#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane) \ >> + _MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \ >> + _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe)) >Would probably be best to keep those separate. You mean in a separate patch ? If yes, I can do that and segregate the register macro definitions in a separate patch. >> +/* >> + * These values are direct register values specified in the Bspec, >> + * for YUV->RGB Full Range conversion matrix (colorspace BT709) */ >> +#define CSC_BT709_YUV_TO_RGB_RY_GY 0x7C987800 >> +#define CSC_BT709_YUV_TO_RGB_BY 0x0 >> +#define CSC_BT709_YUV_TO_RGB_RU_GU 0x9EF87800 >> +#define CSC_BT709_YUV_TO_RGB_BU 0xABF8 >> +#define CSC_BT709_YUV_TO_RGB_RV_GV 0x7800 >> +#define CSC_BT709_YUV_TO_RGB_BV 0x7ED8 >> + >> +/* >> + * These values are direct register values specified in the Bspec, >> + * for YUV->RGB Full Range conversion matrix (colorspace BT601) */ >> +#define CSC_BT601_YUV_TO_RGB_RY_GY 0x7AF87800 >> +#define CSC_BT601_YUV_TO_RGB_BY 0x0 >> +#define CSC_BT601_YUV_TO_RGB_RU_GU 0x8B287800 >> +#define CSC_BT601_YUV_TO_RGB_BU 0x9AC0 >> +#define CSC_BT601_YUV_TO_RGB_RV_GV 0x7800 >> +#define CSC_BT601_YUV_TO_RGB_BV 0x7DD8 >> + >> +/* Preoffset values for YUV to RGB Conversion */ >> +#define PREOFF_YUV_TO_RGB_HI 0x800 >> +#define PREOFF_YUV_TO_RGB_ME 0xF00 >> +#define PREOFF_YUV_TO_RGB_LO 0x800 >> >> #define _PLANE_CTL_1_B 0x71180 >> #define _PLANE_CTL_2_B 0x71280 >Probably move those coefficients to intel_color.c ? Sure, I will do that. >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index fc7e3b0..38b41ed 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -3689,12 +3689,66 @@ u32 skl_plane_ctl(const struct intel_crtc_state >*crtc_state, >> return plane_ctl; >> } >> >> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state, >> + const struct intel_plane_state *plane_state) { >> + struct drm_i915_private *dev_priv = >> + to_i915(plane_state->base.plane->dev); >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >> + enum pipe pipe = crtc->pipe; >> + struct intel_plane *intel_plane = >> + to_intel_plane(plane_state->base.plane); >> + enum plane_id plane = intel_plane->id; >> + >> + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) { >> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane), >> + CSC_BT709_YUV_TO_RGB_RY_GY); >> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane), >> + CSC_BT709_YUV_TO_RGB_BY); >> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane), >> + CSC_BT709_YUV_TO_RGB_RU_GU); >> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane), >> + CSC_BT709_YUV_TO_RGB_BU); >> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane), >> + CSC_BT709_YUV_TO_RGB_RV_GV); >> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane), >> + CSC_BT709_YUV_TO_RGB_BV); >> + } else { >> + I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane), >> + CSC_BT601_YUV_TO_RGB_RY_GY); >> + I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane), >> + CSC_BT601_YUV_TO_RGB_BY); >> + I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane), >> + CSC_BT601_YUV_TO_RGB_RU_GU); >> + I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane), >> + CSC_BT601_YUV_TO_RGB_BU); >> + I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane), >> + CSC_BT601_YUV_TO_RGB_RV_GV); >> + I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane), >> + CSC_BT601_YUV_TO_RGB_BV); >> + } >Considering there's going to be BT.601, BT.709 and perhaps newer colorspaces >with limited vs full range, would it make more sense to generate the values? These are all floating point coefficient values and driver has restrictions on the floating math, so would be tough to generate them. Currently only BT709 and BT601 was supported. Later BT2020 co-eficients can be added. Other way can be to expose this as a property and get these values from userspace (I would not want to go that path as it will be an extra ABI burden and will require a userspace). Personally I feel, defining 6 macro value for a new colorspace would be an easier option. >> + >> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_HI(pipe, plane), >> + PREOFF_YUV_TO_RGB_HI); >> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_ME(pipe, plane), >> + PREOFF_YUV_TO_RGB_ME); >> + I915_WRITE(PLANE_INPUT_CSC_PREOFF_LO(pipe, plane), >> + PREOFF_YUV_TO_RGB_LO); >> + >> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane), 0x0); >> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane), 0x0); >> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane), 0x0); } >We already have some support code for CSC matrices in intel_color.c, so I think it >makes sense to program this from there.. Since for the non HDR planes CSC bits are programmed here, it would be good if all planes are handled at one place. I will move the function to intel_color.c to keep all the color related functions in one file. >> u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state, >> const struct intel_plane_state *plane_state) { >> struct drm_i915_private *dev_priv = >> to_i915(plane_state->base.plane->dev); >> const struct drm_framebuffer *fb = plane_state->base.fb; >> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane); >> + enum plane_id plane_id = plane->id; >> + >> u32 plane_color_ctl = 0; >> >> if (INTEL_GEN(dev_priv) < 11) { >> @@ -3705,13 +3759,23 @@ u32 glk_plane_color_ctl(const struct >intel_crtc_state *crtc_state, >> plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state); >> >> if (fb->format->is_yuv) { >> - if (plane_state->base.color_encoding == >DRM_COLOR_YCBCR_BT709) >> - plane_color_ctl |= >PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; >> - else >> - plane_color_ctl |= >PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; >> + if (INTEL_GEN(dev_priv) < 11 || plane_id > 2) { >We now have icl_is_hdr_plane(), it just landed :) >> + if (plane_state->base.color_encoding == >> + DRM_COLOR_YCBCR_BT709) >> + plane_color_ctl |= >> + > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709; >> + else >> + plane_color_ctl |= >> + > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709; >> >> - if (plane_state->base.color_range == >DRM_COLOR_YCBCR_FULL_RANGE) >> - plane_color_ctl |= >PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; >> + if (plane_state->base.color_range == >> + DRM_COLOR_YCBCR_FULL_RANGE) >> + plane_color_ctl |= >> + > PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE; >> + } else { >> + icl_program_input_csc_coeff(crtc_state, plane_state); >> + plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE; >The coefficients are likely different for full vs limited range, and I don't see that >handled at the moment. :( I have supported the FULL Range YUV to RGB conversion in this patch. Will add limited range along with BT2020 with a later patch. This should unblock all the current YUV Full Range planar format implementation. Hope this is ok ? Thanks for the review Maarten. Will send out the updated patch based on your recommendations. Regards, Uma Shankar >> + } >> } >> >> return plane_color_ctl; > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
