| Message ID | 1499960000-9232-13-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 13, 2017 at 09:03:18PM +0530, Shashank Sharma wrote: > To support ycbcr output, we need a pipe CSC block to do > RGB->YCBCR conversion. > > Current Intel platforms have only one pipe CSC unit, so > we can either do color correction using it, or we can perform > RGB->YCBCR conversion. > > This function adds a csc handler, which uses recommended bspec > values to perform RGB->YCBCR conversion (target color space BT709) > > V2: Rebase > V3: Rebase > V4: Rebase > V5: Addressed review comments from Ander > - Remove extra line added in the patch > - Add the spec details in the commit message > - Combine two if(cond) while calling intel_crtc_compute_config > V6: Handle YCBCR420 outputs only (Ville) > V7: Addressed review comments from Ville: > - Add description about target colorspace > - Remove the comments from CSC function > - DRM_DEBUG->DEBUG_KMS for atomic failure due to CSC unit busy > - Remove unnecessary debug message about YCBCR420 possibe > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_color.c | 46 +++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++ > 2 files changed, 60 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > index f85d575..8698691 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -41,6 +41,22 @@ > > #define LEGACY_LUT_LENGTH (sizeof(struct drm_color_lut) * 256) > > +/* Post offset values for RGB->YCBCR conversion */ > +#define POSTOFF_RGB_TO_YUV_HI 0x800 > +#define POSTOFF_RGB_TO_YUV_ME 0x100 > +#define POSTOFF_RGB_TO_YUV_LO 0x800 > + > +/* > + * These values are direct register values specified in the Bspec, > + * for RGB->YUV conversion matrix (colorspace BT709) > + */ > +#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8 > +#define CSC_RGB_TO_YUV_BU 0x37e80000 > +#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0 > +#define CSC_RGB_TO_YUV_BY 0xb5280000 > +#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 > +#define CSC_RGB_TO_YUV_BV 0x1e080000 > + > /* > * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point > * format). This macro takes the coefficient we want transformed and the > @@ -91,6 +107,31 @@ static void ctm_mult_by_limited(uint64_t *result, int64_t *input) > } > } > > +void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc) > +{ > + int pipe = intel_crtc->pipe; > + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev); > + > + > + I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0); > + I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0); > + I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0); > + > + I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), CSC_RGB_TO_YUV_RU_GU); > + I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU); > + > + I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), CSC_RGB_TO_YUV_RY_GY); > + I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY); > + > + I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), CSC_RGB_TO_YUV_RV_GV); > + I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV); > + > + I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI); > + I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME); > + I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO); > + I915_WRITE(PIPE_CSC_MODE(pipe), 0); > +} > + > /* Set up the pipe CSC unit. */ > static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) > { > @@ -101,7 +142,10 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) > uint16_t coeffs[9] = { 0, }; > struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state); > > - if (crtc_state->ctm) { > + if (intel_crtc_state->ycbcr420) { > + i9xx_load_ycbcr_conversion_matrix(intel_crtc); > + return; > + } else if (crtc_state->ctm) { > struct drm_color_ctm *ctm = > (struct drm_color_ctm *)crtc_state->ctm->data; > uint64_t input[9] = { 0, }; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 1a23ec0..9c6a1ed 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6283,6 +6283,21 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, > return -EINVAL; > } > > + /* YCBCR420 feasibility check */ Not a useful comment. > + if (pipe_config->ycbcr420) { > + struct drm_crtc_state *drm_state = &pipe_config->base; Having aliasing pointer for the same thing is just annoying. We do have quite a lot of that going around, but IMO we need to clean that all up at some point. So pls just do this instead: if (pipe_config->ycbcr420 && pipe_config->base.ctm) { ... } > + > + /* > + * There is only one pipe CSC unit per pipe, and we need that > + * for output conversion from RGB->YCBCR. So if CTM is already > + * applied we can't support YCBCR420 output. > + */ > + if (drm_state->ctm) { > + DRM_DEBUG_KMS("YCBCR420 and CTM is not possible\n"); > + return -EINVAL; > + } > + } > + > /* > * Pipe horizontal size must be even in: > * - DVO ganged mode > -- > 2.7.4
Regards Shashank On 7/15/2017 12:06 AM, Ville Syrjälä wrote: > On Thu, Jul 13, 2017 at 09:03:18PM +0530, Shashank Sharma wrote: >> To support ycbcr output, we need a pipe CSC block to do >> RGB->YCBCR conversion. >> >> Current Intel platforms have only one pipe CSC unit, so >> we can either do color correction using it, or we can perform >> RGB->YCBCR conversion. >> >> This function adds a csc handler, which uses recommended bspec >> values to perform RGB->YCBCR conversion (target color space BT709) >> >> V2: Rebase >> V3: Rebase >> V4: Rebase >> V5: Addressed review comments from Ander >> - Remove extra line added in the patch >> - Add the spec details in the commit message >> - Combine two if(cond) while calling intel_crtc_compute_config >> V6: Handle YCBCR420 outputs only (Ville) >> V7: Addressed review comments from Ville: >> - Add description about target colorspace >> - Remove the comments from CSC function >> - DRM_DEBUG->DEBUG_KMS for atomic failure due to CSC unit busy >> - Remove unnecessary debug message about YCBCR420 possibe >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_color.c | 46 +++++++++++++++++++++++++++++++++++- >> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++ >> 2 files changed, 60 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c >> index f85d575..8698691 100644 >> --- a/drivers/gpu/drm/i915/intel_color.c >> +++ b/drivers/gpu/drm/i915/intel_color.c >> @@ -41,6 +41,22 @@ >> >> #define LEGACY_LUT_LENGTH (sizeof(struct drm_color_lut) * 256) >> >> +/* Post offset values for RGB->YCBCR conversion */ >> +#define POSTOFF_RGB_TO_YUV_HI 0x800 >> +#define POSTOFF_RGB_TO_YUV_ME 0x100 >> +#define POSTOFF_RGB_TO_YUV_LO 0x800 >> + >> +/* >> + * These values are direct register values specified in the Bspec, >> + * for RGB->YUV conversion matrix (colorspace BT709) >> + */ >> +#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8 >> +#define CSC_RGB_TO_YUV_BU 0x37e80000 >> +#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0 >> +#define CSC_RGB_TO_YUV_BY 0xb5280000 >> +#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 >> +#define CSC_RGB_TO_YUV_BV 0x1e080000 >> + >> /* >> * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point >> * format). This macro takes the coefficient we want transformed and the >> @@ -91,6 +107,31 @@ static void ctm_mult_by_limited(uint64_t *result, int64_t *input) >> } >> } >> >> +void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc) >> +{ >> + int pipe = intel_crtc->pipe; >> + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev); >> + >> + >> + I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0); >> + I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0); >> + I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0); >> + >> + I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), CSC_RGB_TO_YUV_RU_GU); >> + I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU); >> + >> + I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), CSC_RGB_TO_YUV_RY_GY); >> + I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY); >> + >> + I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), CSC_RGB_TO_YUV_RV_GV); >> + I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV); >> + >> + I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI); >> + I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME); >> + I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO); >> + I915_WRITE(PIPE_CSC_MODE(pipe), 0); >> +} >> + >> /* Set up the pipe CSC unit. */ >> static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) >> { >> @@ -101,7 +142,10 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) >> uint16_t coeffs[9] = { 0, }; >> struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state); >> >> - if (crtc_state->ctm) { >> + if (intel_crtc_state->ycbcr420) { >> + i9xx_load_ycbcr_conversion_matrix(intel_crtc); >> + return; >> + } else if (crtc_state->ctm) { >> struct drm_color_ctm *ctm = >> (struct drm_color_ctm *)crtc_state->ctm->data; >> uint64_t input[9] = { 0, }; >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 1a23ec0..9c6a1ed 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6283,6 +6283,21 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, >> return -EINVAL; >> } >> >> + /* YCBCR420 feasibility check */ > Not a useful comment. Got it. >> + if (pipe_config->ycbcr420) { >> + struct drm_crtc_state *drm_state = &pipe_config->base; > Having aliasing pointer for the same thing is just annoying. > We do have quite a lot of that going around, but IMO we need > to clean that all up at some point. > > So pls just do this instead: > > if (pipe_config->ycbcr420 && pipe_config->base.ctm) { > ... > } Most of the time its done to cover for 80 char limit, and to make code look good. but let me see if I can accommodate this one. - Shashank > >> + >> + /* >> + * There is only one pipe CSC unit per pipe, and we need that >> + * for output conversion from RGB->YCBCR. So if CTM is already >> + * applied we can't support YCBCR420 output. >> + */ >> + if (drm_state->ctm) { >> + DRM_DEBUG_KMS("YCBCR420 and CTM is not possible\n"); >> + return -EINVAL; >> + } >> + } >> + >> /* >> * Pipe horizontal size must be even in: >> * - DVO ganged mode >> -- >> 2.7.4
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index f85d575..8698691 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -41,6 +41,22 @@ #define LEGACY_LUT_LENGTH (sizeof(struct drm_color_lut) * 256) +/* Post offset values for RGB->YCBCR conversion */ +#define POSTOFF_RGB_TO_YUV_HI 0x800 +#define POSTOFF_RGB_TO_YUV_ME 0x100 +#define POSTOFF_RGB_TO_YUV_LO 0x800 + +/* + * These values are direct register values specified in the Bspec, + * for RGB->YUV conversion matrix (colorspace BT709) + */ +#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8 +#define CSC_RGB_TO_YUV_BU 0x37e80000 +#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0 +#define CSC_RGB_TO_YUV_BY 0xb5280000 +#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 +#define CSC_RGB_TO_YUV_BV 0x1e080000 + /* * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point * format). This macro takes the coefficient we want transformed and the @@ -91,6 +107,31 @@ static void ctm_mult_by_limited(uint64_t *result, int64_t *input) } } +void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc) +{ + int pipe = intel_crtc->pipe; + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev); + + + I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0); + I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0); + I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0); + + I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), CSC_RGB_TO_YUV_RU_GU); + I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU); + + I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), CSC_RGB_TO_YUV_RY_GY); + I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY); + + I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), CSC_RGB_TO_YUV_RV_GV); + I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV); + + I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI); + I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME); + I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO); + I915_WRITE(PIPE_CSC_MODE(pipe), 0); +} + /* Set up the pipe CSC unit. */ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) { @@ -101,7 +142,10 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) uint16_t coeffs[9] = { 0, }; struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state); - if (crtc_state->ctm) { + if (intel_crtc_state->ycbcr420) { + i9xx_load_ycbcr_conversion_matrix(intel_crtc); + return; + } else if (crtc_state->ctm) { struct drm_color_ctm *ctm = (struct drm_color_ctm *)crtc_state->ctm->data; uint64_t input[9] = { 0, }; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1a23ec0..9c6a1ed 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6283,6 +6283,21 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, return -EINVAL; } + /* YCBCR420 feasibility check */ + if (pipe_config->ycbcr420) { + struct drm_crtc_state *drm_state = &pipe_config->base; + + /* + * There is only one pipe CSC unit per pipe, and we need that + * for output conversion from RGB->YCBCR. So if CTM is already + * applied we can't support YCBCR420 output. + */ + if (drm_state->ctm) { + DRM_DEBUG_KMS("YCBCR420 and CTM is not possible\n"); + return -EINVAL; + } + } + /* * Pipe horizontal size must be even in: * - DVO ganged mode
To support ycbcr output, we need a pipe CSC block to do RGB->YCBCR conversion. Current Intel platforms have only one pipe CSC unit, so we can either do color correction using it, or we can perform RGB->YCBCR conversion. This function adds a csc handler, which uses recommended bspec values to perform RGB->YCBCR conversion (target color space BT709) V2: Rebase V3: Rebase V4: Rebase V5: Addressed review comments from Ander - Remove extra line added in the patch - Add the spec details in the commit message - Combine two if(cond) while calling intel_crtc_compute_config V6: Handle YCBCR420 outputs only (Ville) V7: Addressed review comments from Ville: - Add description about target colorspace - Remove the comments from CSC function - DRM_DEBUG->DEBUG_KMS for atomic failure due to CSC unit busy - Remove unnecessary debug message about YCBCR420 possibe Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/intel_color.c | 46 +++++++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-)