| Message ID | 1498041253-16426-13-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote: > To support ycbcr HDMI output, we need a pipe CSC block to > do the RGB->YCBCR conversion, as the blender output is in RGB. > > 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, to perform RGB->YCBCR conversion > as per recommended spec values. Please do a full reference to the "spec", including name, version and relevant section. > > V2: Rebase > V3: Rebase > V4: Rebase > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_color.c | 47 +++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++ > 2 files changed, 78 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > index 306c6b0..12d5f21 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -41,6 +41,19 @@ > > #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 > + > +/* Direct spec values for RGB->YUV conversion matrix */ > +#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 > + Not a big fan or hardcoding this in the register format. We already have the code for converting a number to the right format for the register in i915_load_csc_matrix(). I think it would make more sense to extract the code that actually writes the matrix out of that function, so it would just unconditionally use a matrix and coefficients passed as arguments. Then the values above would be defined in the format expected for this new function. > /* > * 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 +104,35 @@ 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); > + > + /* We don't use high values for conversion */ > + I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0); > + I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0); > + I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0); > + > + /* Program direct spec values for RGB to YCBCR conversion matrix */ > + 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); > + > + /* Spec postoffset values */ > + 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); > + > + /* CSC mode before gamma */ > + 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 +143,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->hdmi_output > DRM_HDMI_OUTPUT_DEFAULT_RGB) { > + i9xx_load_ycbcr_conversion_matrix(intel_crtc); > + return; > + } else if (crtc_state->ctm) { Hmm, I'm not sure this is the right place to check for this condition. I mean, we shouldn't allow the modeset to happen in this case. > 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 71fd19e..96ff2a0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6261,6 +6261,29 @@ static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state) > ilk_pipe_pixel_rate(crtc_state); > } > > +static int intel_crtc_ycbcr_config(struct intel_crtc_state *state) > +{ > + struct drm_crtc_state *drm_state = &state->base; > + struct drm_i915_private *dev_priv = to_i915(drm_state->crtc->dev); > + > + /* YCBCR420 is supported only in HDMI 2.0 controllers */ > + if ((state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420) && > + !IS_GEMINILAKE(dev_priv)) { > + DRM_ERROR("YCBCR420 output is not supported\n"); > + return -EINVAL; > + } > + > + /* We need CSC for output conversion from RGB->YCBCR */ > + if (drm_state->ctm) { > + DRM_ERROR("YCBCR output and CTM is not possible together\n"); > + return -EINVAL; > + } Hmm, ok, that's checked here. > + > + DRM_DEBUG_DRIVER("Output %s can be supported\n", > + drm_get_hdmi_output_name(state->hdmi_output)); > + return 0; > +} > + > static int intel_crtc_compute_config(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config) > { > @@ -6290,6 +6313,14 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, > return -EINVAL; > } > > + /* YCBCR output check */ > + if (pipe_config->hdmi_output > DRM_HDMI_OUTPUT_DEFAULT_RGB) { > + if (intel_crtc_ycbcr_config(pipe_config)) { You could combine both ifs. Or, even better, just move the first check into the caller. I think it would be better to keep it all in one place. I was a bit unsure if function did the right thing until I saw how it was called. > + DRM_ERROR("Cant enable HDMI YCBCR output\n"); > + return -EINVAL; > + } > + } > + > /* > * Pipe horizontal size must be even in: > * - DVO ganged mode > @@ -11658,6 +11689,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > DRM_DEBUG_KMS("Encoder config failure\n"); > goto fail; > } > + No need for this. Ander > } > > /* Set default port clock if not overwritten by the encoder. Needs to be
Regards Shashank On 6/29/2017 5:38 PM, Ander Conselvan De Oliveira wrote: > On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote: >> To support ycbcr HDMI output, we need a pipe CSC block to >> do the RGB->YCBCR conversion, as the blender output is in RGB. >> >> 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, to perform RGB->YCBCR conversion >> as per recommended spec values. > Please do a full reference to the "spec", including name, version and relevant > section. Sure, will add more details. >> V2: Rebase >> V3: Rebase >> V4: Rebase >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_color.c | 47 +++++++++++++++++++++++++++++++++++- >> drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++ >> 2 files changed, 78 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c >> index 306c6b0..12d5f21 100644 >> --- a/drivers/gpu/drm/i915/intel_color.c >> +++ b/drivers/gpu/drm/i915/intel_color.c >> @@ -41,6 +41,19 @@ >> >> #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 >> + >> +/* Direct spec values for RGB->YUV conversion matrix */ >> +#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 >> + > Not a big fan or hardcoding this in the register format. We already have the > code for converting a number to the right format for the register in > i915_load_csc_matrix(). I think it would make more sense to extract the code > that actually writes the matrix out of that function, so it would just > unconditionally use a matrix and coefficients passed as arguments. Then the > values above would be defined in the format expected for this new function. Actually I had a small discussion on this with Ville, and we think that the current CSC multiplication code is not correct. So if CTM and limited color range is applied together, we might not be getting the right values. So not planning to reuse that code. I think while sending the BT2020 patches, we will add a fix for that part too, but right now not working on it. >> /* >> * 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 +104,35 @@ 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); >> + >> + /* We don't use high values for conversion */ >> + I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0); >> + I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0); >> + I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0); >> + >> + /* Program direct spec values for RGB to YCBCR conversion matrix */ >> + 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); >> + >> + /* Spec postoffset values */ >> + 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); >> + >> + /* CSC mode before gamma */ >> + 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 +143,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->hdmi_output > DRM_HDMI_OUTPUT_DEFAULT_RGB) { >> + i9xx_load_ycbcr_conversion_matrix(intel_crtc); >> + return; >> + } else if (crtc_state->ctm) { > Hmm, I'm not sure this is the right place to check for this condition. I mean, > we shouldn't allow the modeset to happen in this case. If you please have a look at the intel_crtc_compute_ycbcr_config check added in this patch, that's doing the same. Its not allowing HDMI output conversion and pipe level CTM at the same time. > >> 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 71fd19e..96ff2a0 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6261,6 +6261,29 @@ static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state) >> ilk_pipe_pixel_rate(crtc_state); >> } >> >> +static int intel_crtc_ycbcr_config(struct intel_crtc_state *state) >> +{ >> + struct drm_crtc_state *drm_state = &state->base; >> + struct drm_i915_private *dev_priv = to_i915(drm_state->crtc->dev); >> + >> + /* YCBCR420 is supported only in HDMI 2.0 controllers */ >> + if ((state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420) && >> + !IS_GEMINILAKE(dev_priv)) { >> + DRM_ERROR("YCBCR420 output is not supported\n"); >> + return -EINVAL; >> + } >> + >> + /* We need CSC for output conversion from RGB->YCBCR */ >> + if (drm_state->ctm) { >> + DRM_ERROR("YCBCR output and CTM is not possible together\n"); >> + return -EINVAL; >> + } > Hmm, ok, that's checked here. :-) >> + >> + DRM_DEBUG_DRIVER("Output %s can be supported\n", >> + drm_get_hdmi_output_name(state->hdmi_output)); >> + return 0; >> +} >> + >> static int intel_crtc_compute_config(struct intel_crtc *crtc, >> struct intel_crtc_state *pipe_config) >> { >> @@ -6290,6 +6313,14 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, >> return -EINVAL; >> } >> >> + /* YCBCR output check */ >> + if (pipe_config->hdmi_output > DRM_HDMI_OUTPUT_DEFAULT_RGB) { >> + if (intel_crtc_ycbcr_config(pipe_config)) { > You could combine both ifs. Or, even better, just move the first check into the > caller. I think it would be better to keep it all in one place. I was a bit > unsure if function did the right thing until I saw how it was called. Ok, sure. > >> + DRM_ERROR("Cant enable HDMI YCBCR output\n"); >> + return -EINVAL; >> + } >> + } >> + >> /* >> * Pipe horizontal size must be even in: >> * - DVO ganged mode >> @@ -11658,6 +11689,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, >> DRM_DEBUG_KMS("Encoder config failure\n"); >> goto fail; >> } >> + > No need for this. Got it. > > > Ander > >> } >> >> /* Set default port clock if not overwritten by the encoder. Needs to be
On Fri, 2017-06-30 at 11:33 +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 6/29/2017 5:38 PM, Ander Conselvan De Oliveira wrote: > > On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote: > > > To support ycbcr HDMI output, we need a pipe CSC block to > > > do the RGB->YCBCR conversion, as the blender output is in RGB. > > > > > > 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, to perform RGB->YCBCR conversion > > > as per recommended spec values. > > > > Please do a full reference to the "spec", including name, version and relevant > > section. > > Sure, will add more details. > > > V2: Rebase > > > V3: Rebase > > > V4: Rebase > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_color.c | 47 +++++++++++++++++++++++++++++++++++- > > > drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++ > > > 2 files changed, 78 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c > > > index 306c6b0..12d5f21 100644 > > > --- a/drivers/gpu/drm/i915/intel_color.c > > > +++ b/drivers/gpu/drm/i915/intel_color.c > > > @@ -41,6 +41,19 @@ > > > > > > #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 > > > + > > > +/* Direct spec values for RGB->YUV conversion matrix */ > > > +#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 > > > + > > > > Not a big fan or hardcoding this in the register format. We already have the > > code for converting a number to the right format for the register in > > i915_load_csc_matrix(). I think it would make more sense to extract the code > > that actually writes the matrix out of that function, so it would just > > unconditionally use a matrix and coefficients passed as arguments. Then the > > values above would be defined in the format expected for this new function. > > Actually I had a small discussion on this with Ville, and we think that > the current CSC multiplication code is not correct. > So if CTM and limited color range is applied together, we might not be > getting the right values. So not planning to > reuse that code. I think while sending the BT2020 patches, we will add a > fix for that part too, but right now not working on it. I wasn't talking about the matrix multiplication, but creating a function to write any given matrix into the CSC. That way, the above could be hardcoded in a more human readable format. This issue is independent from fixing the matrix multiplication. I'm talking specifically about the code below: /* * Convert fixed point S31.32 input to format supported by the * hardware. */ for (i = 0; i < ARRAY_SIZE(coeffs); i++) { uint64_t abs_coeff = ((1ULL << 63) - 1) & input[i]; /* * Clamp input value to min/max supported by * hardware. */ abs_coeff = clamp_val(abs_coeff, 0, CTM_COEFF_4_0 - 1); /* sign bit */ if (CTM_COEFF_NEGATIVE(input[i])) coeffs[i] |= 1 << 15; if (abs_coeff < CTM_COEFF_0_125) coeffs[i] |= (3 << 12) | I9XX_CSC_COEFF_FP(abs_coeff, 12); else if (abs_coeff < CTM_COEFF_0_25) coeffs[i] |= (2 << 12) | I9XX_CSC_COEFF_FP(abs_coeff, 11); else if (abs_coeff < CTM_COEFF_0_5) coeffs[i] |= (1 << 12) | I9XX_CSC_COEFF_FP(abs_coeff, 10); else if (abs_coeff < CTM_COEFF_1_0) coeffs[i] |= I9XX_CSC_COEFF_FP(abs_coeff, 9); else if (abs_coeff < CTM_COEFF_2_0) coeffs[i] |= (7 << 12) | I9XX_CSC_COEFF_FP(abs_coeff, 8); else coeffs[i] |= (6 << 12) | I9XX_CSC_COEFF_FP(abs_coeff, 7); } I'd like to see that and the code that writes the registers reused instead of duplicated. Ander > > > /* > > > * 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 +104,35 @@ 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); > > > + > > > + /* We don't use high values for conversion */ > > > + I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0); > > > + I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0); > > > + I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0); > > > + > > > + /* Program direct spec values for RGB to YCBCR conversion matrix */ > > > + 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); > > > + > > > + /* Spec postoffset values */ > > > + 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); > > > + > > > + /* CSC mode before gamma */ > > > + 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 +143,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->hdmi_output > DRM_HDMI_OUTPUT_DEFAULT_RGB) { > > > + i9xx_load_ycbcr_conversion_matrix(intel_crtc); > > > + return; > > > + } else if (crtc_state->ctm) { > > > > Hmm, I'm not sure this is the right place to check for this condition. I mean, > > we shouldn't allow the modeset to happen in this case. > > If you please have a look at the intel_crtc_compute_ycbcr_config check > added in this patch, that's doing the same. > Its not allowing HDMI output conversion and pipe level CTM at the same > time. > > > > > 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 71fd19e..96ff2a0 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -6261,6 +6261,29 @@ static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state) > > > ilk_pipe_pixel_rate(crtc_state); > > > } > > > > > > +static int intel_crtc_ycbcr_config(struct intel_crtc_state *state) > > > +{ > > > + struct drm_crtc_state *drm_state = &state->base; > > > + struct drm_i915_private *dev_priv = to_i915(drm_state->crtc->dev); > > > + > > > + /* YCBCR420 is supported only in HDMI 2.0 controllers */ > > > + if ((state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420) && > > > + !IS_GEMINILAKE(dev_priv)) { > > > + DRM_ERROR("YCBCR420 output is not supported\n"); > > > + return -EINVAL; > > > + } > > > + > > > + /* We need CSC for output conversion from RGB->YCBCR */ > > > + if (drm_state->ctm) { > > > + DRM_ERROR("YCBCR output and CTM is not possible together\n"); > > > + return -EINVAL; > > > + } > > > > Hmm, ok, that's checked here. > > :-) > > > + > > > + DRM_DEBUG_DRIVER("Output %s can be supported\n", > > > + drm_get_hdmi_output_name(state->hdmi_output)); > > > + return 0; > > > +} > > > + > > > static int intel_crtc_compute_config(struct intel_crtc *crtc, > > > struct intel_crtc_state *pipe_config) > > > { > > > @@ -6290,6 +6313,14 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, > > > return -EINVAL; > > > } > > > > > > + /* YCBCR output check */ > > > + if (pipe_config->hdmi_output > DRM_HDMI_OUTPUT_DEFAULT_RGB) { > > > + if (intel_crtc_ycbcr_config(pipe_config)) { > > > > You could combine both ifs. Or, even better, just move the first check into the > > caller. I think it would be better to keep it all in one place. I was a bit > > unsure if function did the right thing until I saw how it was called. > > Ok, sure. > > > > > + DRM_ERROR("Cant enable HDMI YCBCR output\n"); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > /* > > > * Pipe horizontal size must be even in: > > > * - DVO ganged mode > > > @@ -11658,6 +11689,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > > > DRM_DEBUG_KMS("Encoder config failure\n"); > > > goto fail; > > > } > > > + > > > > No need for this. > > Got it. > > > > > > Ander > > > > > } > > > > > > /* Set default port clock if not overwritten by the encoder. Needs to be > >
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index 306c6b0..12d5f21 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -41,6 +41,19 @@ #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 + +/* Direct spec values for RGB->YUV conversion matrix */ +#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 +104,35 @@ 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); + + /* We don't use high values for conversion */ + I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0); + I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0); + I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0); + + /* Program direct spec values for RGB to YCBCR conversion matrix */ + 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); + + /* Spec postoffset values */ + 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); + + /* CSC mode before gamma */ + 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 +143,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->hdmi_output > DRM_HDMI_OUTPUT_DEFAULT_RGB) { + 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 71fd19e..96ff2a0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6261,6 +6261,29 @@ static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state) ilk_pipe_pixel_rate(crtc_state); } +static int intel_crtc_ycbcr_config(struct intel_crtc_state *state) +{ + struct drm_crtc_state *drm_state = &state->base; + struct drm_i915_private *dev_priv = to_i915(drm_state->crtc->dev); + + /* YCBCR420 is supported only in HDMI 2.0 controllers */ + if ((state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420) && + !IS_GEMINILAKE(dev_priv)) { + DRM_ERROR("YCBCR420 output is not supported\n"); + return -EINVAL; + } + + /* We need CSC for output conversion from RGB->YCBCR */ + if (drm_state->ctm) { + DRM_ERROR("YCBCR output and CTM is not possible together\n"); + return -EINVAL; + } + + DRM_DEBUG_DRIVER("Output %s can be supported\n", + drm_get_hdmi_output_name(state->hdmi_output)); + return 0; +} + static int intel_crtc_compute_config(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { @@ -6290,6 +6313,14 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, return -EINVAL; } + /* YCBCR output check */ + if (pipe_config->hdmi_output > DRM_HDMI_OUTPUT_DEFAULT_RGB) { + if (intel_crtc_ycbcr_config(pipe_config)) { + DRM_ERROR("Cant enable HDMI YCBCR output\n"); + return -EINVAL; + } + } + /* * Pipe horizontal size must be even in: * - DVO ganged mode @@ -11658,6 +11689,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, DRM_DEBUG_KMS("Encoder config failure\n"); goto fail; } + } /* Set default port clock if not overwritten by the encoder. Needs to be
To support ycbcr HDMI output, we need a pipe CSC block to do the RGB->YCBCR conversion, as the blender output is in RGB. 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, to perform RGB->YCBCR conversion as per recommended spec values. V2: Rebase V3: Rebase V4: Rebase Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/intel_color.c | 47 +++++++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-)