[Intel-gfx] [PATCH] drm/i915/GLK: Properly handle plane CSC for BT2020 framebuffers

[Sharma, Shashank]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Thursday, May 2, 2019 3:45 PM
> To: Sharma, Shashank <shashank.sharma at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst,
> Maarten <maarten.lankhorst at intel.com>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/GLK: Properly handle plane CSC for
> BT2020 framebuffers
> 
> On Thu, May 02, 2019 at 03:19:42PM +0530, Shashank Sharma wrote:
> > Framebuffer formats P01x are supported by GLK, but the function which
> > handles CSC on plane color control register, still expectes the input
> > buffer to be REC709. This can cause inaccurate output for direct P01x
> > flips.
> >
> > This patch checks if the color_encoding property is set to YCBCR_2020,
> > and enables the corresponding color conversion mode on plane CSC.
> >
> > PS: renamed variable plane_color_ctl to color_ctl for 80 char stuff.
> >
> > Cc: Ville Syrjala <ville.syrjala at intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at intel.com>
> > Cc: Uma Shankar <uma.shankar at intel.com>
> > Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++----------
> >  1 file changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index dd65d7c521c1..2d4d3128bf1f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3868,24 +3868,30 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state
> *crtc_state,
> >  		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);
> > -	u32 plane_color_ctl = 0;
> > +	u32 color_ctl = 0;
> >
> > -	plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
> > -	plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
> > +	color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
> > +	color_ctl |= glk_plane_color_ctl_alpha(plane_state);
> >
> >  	if (fb->format->is_yuv && !icl_is_hdr_plane(dev_priv, plane->id)) {
> > -		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;
> > +		switch (plane_state->base.color_encoding) {
> > +		case DRM_COLOR_YCBCR_BT709:
> > +			color_ctl |=
> PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> > +			break;
> > +		case DRM_COLOR_YCBCR_BT2020:
> > +			color_ctl |=
> PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020;
> > +			break;
> > +		default:
> > +			color_ctl |=
> PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> > +		}
> 
> This isn't going to do anything without adjusting the property supported encodings as
> well.
>
I might have not understood this comment properly, but, AFAIK, if userspace sets this property well, and we set this color_ctl bit properly, driver is setting PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020 bits in GLK color control register. As GLK has a fix function plane CSC, HW will apply a different matrix internally to convert input buffer to RGB_2020 from YCBCR_2020 (earlier this would have been YCBCR_709).  So I think we should see visible changes in output. Do you think otherwise ? 

- Shashank
 
> >
> >  		if (plane_state->base.color_range ==
> DRM_COLOR_YCBCR_FULL_RANGE)
> > -			plane_color_ctl |=
> PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> > +			color_ctl |=
> PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> >  	} else if (fb->format->is_yuv) {
> > -		plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
> > +		color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
> >  	}
> >
> > -	return plane_color_ctl;
> > +	return color_ctl;
> >  }
> >
> >  static int
> > --
> > 2.17.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel