>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Wednesday, October 24, 2018 8:10 PM >To: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten ><maarten.lankhorst@xxxxxxxxx> >Subject: Re: [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB >Conversion > >On Wed, Oct 24, 2018 at 03:42:34PM +0200, Maarten Lankhorst wrote: >> Op 24-10-18 om 15:12 schreef Shankar, Uma: >> > >> >> -----Original Message----- >> >> From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] >> >> Sent: Wednesday, October 24, 2018 5:37 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 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? >> > All the planar formats will be up sampled before conversion. So >> > standard YUV to RGB conversion matrix should work even for P01X and Y21X >formats. >> Not opposed to hardcoding then, but would be nice to have those values in nice >static const u16[3][3] array instead of a bunch of defines.. > >Just like chv_update_csc() already does it. Sure, got it. Will update the patch accordingly. >I'd actually like to see a tool in igt that would be capable of calculating the >coefficients for each hardware generation with a different matrix. We have >igt_color_encoding so it should be mostly a matter of converting the resulting >matrix to the correct fixed point (or whatever) precision. I will try to create something on these lines. Atleast a mechanism to get the values which matches to bspec register expectations. Thanks & Regards, Uma Shankar >-- >Ville Syrjälä >Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
