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.syrj...@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vet...@intel.com>
> > > Cc: Ander Conselvan De Oliveira <ander.conselvan.de.olive...@intel.com>
> > > Signed-off-by: Shashank Sharma <shashank.sha...@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
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
