Op 24-10-18 om 13:19 schreef Shankar, Uma: > >> -----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. This would work for NV12, but what about P01X or the Y21X formats? Do they need slightly different coefficients for each? ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
