Re: [PATCH] drm/i915/GLK: Properly handle plane CSC for BT2020 framebuffers

["Lankhorst, Maarten" <maarten.lankhorst@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4599 bytes --]

tor 2019-05-02 klockan 14:51 +0300 skrev Ville Syrjälä:
> On Thu, May 02, 2019 at 10:40:39AM +0000, Sharma, Shashank wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Thursday, May 2, 2019 3:45 PM
> > > To: Sharma, Shashank <shashank.sharma@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjal
> > > a@intel.com>; Lankhorst,
> > > Maarten <maarten.lankhorst@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@intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@intel.com>
> > > > Cc: Uma Shankar <uma.shankar@intel.com>
> > > > Signed-off-by: Shashank Sharma <shashank.sharma@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 ? 
> 
> The property won't accept the BT2020 value. Or if it does we have a
> bug
> somewhere.
> 
> I guess tests would be nice. Maybe we should extend the kms_plane
> pixel
> format tests to check different YCbCr encodings as well? Or maybe
> Maarten's kms_yuv already tests this?
Not yet, unfortunately we have no way to set CSC in igt yet. :(

Best way to do so would be to add a igt_create_fb_yuv() which would a
igt_create_fb that accepts igt color encoding and range as arguments.

~Maarten

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3282 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx