[RESEND-CI v4 13/15] drm/i915: prepare csc unit for YCBCR HDMI output
Ander Conselvan De Oliveira
conselvan2 at gmail.com
Thu Jun 29 12:08:45 UTC 2017
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 at linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira at intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma at 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
More information about the dri-devel
mailing list