Re: [PATCH 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jun 15, 2017 at 06:52:53PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Add support for the COLOR_ENCODING plane property which selects
> > the matrix coefficients used for the YCbCr->RGB conversion. Our
> > hardware can generally handle BT.601 and BT.709.
> >
> > CHV pipe B sprites have a fully programmable matrix, so in theory
> > we could handle anything, but it doesn't seem all that useful to
> > expose anything beyond BT.601 and BT.709 at this time.
> >
> > GLK can supposedly do BT.2020, but let's leave enabling that for
> > the future as well.
> >
> > Cc: Jyri Sarha <jsarha@ti.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_reg.h      |  3 ++
> >   drivers/gpu/drm/i915/intel_display.c | 19 +++++++++--
> >   drivers/gpu/drm/i915/intel_sprite.c  | 61 +++++++++++++++++++++++++++---------
> >   3 files changed, 66 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index f4f51e7a3e83..bac3ec378b6e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5612,6 +5612,7 @@ enum {
> >   #define   DVS_PIPE_CSC_ENABLE   (1<<24)
> >   #define   DVS_SOURCE_KEY	(1<<22)
> >   #define   DVS_RGB_ORDER_XBGR	(1<<20)
> > +#define   DVS_YUV_CSC_FORMAT_BT709	(1<<18)
> >   #define   DVS_YUV_BYTE_ORDER_MASK (3<<16)
> >   #define   DVS_YUV_ORDER_YUYV	(0<<16)
> >   #define   DVS_YUV_ORDER_UYVY	(1<<16)
> > @@ -5758,6 +5759,7 @@ enum {
> >   #define   SP_FORMAT_RGBA8888		(0xf<<26)
> >   #define   SP_ALPHA_PREMULTIPLY		(1<<23) /* CHV pipe B */
> >   #define   SP_SOURCE_KEY			(1<<22)
> > +#define   SP_YUV_CSC_FORMAT_BT709		(1<<18)
> >   #define   SP_YUV_BYTE_ORDER_MASK	(3<<16)
> >   #define   SP_YUV_ORDER_YUYV		(0<<16)
> >   #define   SP_YUV_ORDER_UYVY		(1<<16)
> > @@ -5876,6 +5878,7 @@ enum {
> >   #define   PLANE_CTL_KEY_ENABLE_DESTINATION	(  2 << 21)
> >   #define   PLANE_CTL_ORDER_BGRX			(0 << 20)
> >   #define   PLANE_CTL_ORDER_RGBX			(1 << 20)
> > +#define   PLANE_CTL_YUV_CSC_FORMAT_BT709	(1 << 18)
> Should we call it PLANE_CTL_CSC_YUV_FORMAT_BT709 (or similar) so that it 
> would be clear that source is BT709 format.
> Right now, it feels like CSC(destination) format is BT709.

I just copy pasted it from the IVB sprite plane definition.

I believe the spec calls it:
g4x+: "YUV Format"
ivb+: "YUV to RGB CSC Format"

I guess I could try to line up the definitions to match the spec more
closely.


> >   #define   PLANE_CTL_YUV422_ORDER_MASK		(0x3 << 16)
> >   #define   PLANE_CTL_YUV422_YUYV			(  0 << 16)
> >   #define   PLANE_CTL_YUV422_UYVY			(  1 << 16)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e2aaaf60a969..810b53b0128c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3321,6 +3321,9 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >   			PLANE_CTL_PIPE_GAMMA_ENABLE |
> >   			PLANE_CTL_PIPE_CSC_ENABLE |
> >   			PLANE_CTL_PLANE_GAMMA_DISABLE;
> > +
> > +		if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > +			plane_ctl |= PLANE_CTL_YUV_CSC_FORMAT_BT709;
> >   	}
> >   
> >   	plane_ctl |= skl_plane_ctl_format(fb->format->format);
> > @@ -3344,8 +3347,12 @@ u32 glk_color_ctl(const struct intel_plane_state *plane_state)
> >   		PLANE_COLOR_PIPE_CSC_ENABLE |
> >   		PLANE_COLOR_PLANE_GAMMA_DISABLE;
> >   
> > -	if (intel_format_is_yuv(fb->format->format))
> > -		color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> > +	if (intel_format_is_yuv(fb->format->format)) {
> > +		if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > +			color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> > +		else
> > +			color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> > +	}
> >   
> >   	return color_ctl;
> >   }
> > @@ -13819,6 +13826,14 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> >   						   DRM_MODE_ROTATE_0,
> >   						   supported_rotations);
> >   
> > +	if (INTEL_GEN(dev_priv) >= 9)
> I am seeing few if (INTEL_GEN(dev_priv) >= 9)  checks above, would it 
> make sense to reuse any of those, instead of adding
> a new one ?

I wanted to keep the steps clearly separate. First initialize the plane,
then add the rotation property, and finally add the color properties.

> 
> - Shashank
> > +		drm_plane_create_color_properties(&primary->base,
> > +						  BIT(DRM_COLOR_YCBCR_BT601) |
> > +						  BIT(DRM_COLOR_YCBCR_BT709),
> > +						  BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> > +						  DRM_COLOR_YCBCR_BT601,
> > +						  DRM_COLOR_YCBCR_LIMITED_RANGE);
> > +
> >   	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> >   
> >   	return primary;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index cc07157f2eb6..c21f43d64b00 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -328,30 +328,45 @@ chv_update_csc(const struct intel_plane_state *plane_state)
> >   	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >   	const struct drm_framebuffer *fb = plane_state->base.fb;
> >   	enum plane_id plane_id = plane->id;
> > +	/*
> > +	 * |r|   | c0 c1 c2 |   |cr|
> > +	 * |g| = | c3 c4 c5 | x |y |
> > +	 * |b|   | c6 c7 c8 |   |cb|
> > +	 *
> > +	 * Coefficients are s3.12.
> > +	 *
> > +	 * Cb and Cr apparently come in as signed already, and
> > +	 * we always get full range data in on account of CLRC0/1.
> > +	 */
> > +	static const s16 csc_matrix[][9] = {
> > +		/* BT.601 full range YCbCr -> full range RGB */
> > +		[DRM_COLOR_YCBCR_BT601] = {
> > +			 5743, 4096,     0,
> > +			-2925, 4096, -1410,
> > +			    0, 4096,  7258,
> > +		},
> > +		/* BT.709 full range YCbCr -> full range RGB */
> > +		[DRM_COLOR_YCBCR_BT709] = {
> > +			 6450, 4096,     0,
> > +			-1917, 4096,  -767,
> > +			    0, 4096,  7601,
> > +		},
> > +	};
> > +	const s16 *csc = csc_matrix[plane_state->base.color_encoding];
> >   
> >   	/* Seems RGB data bypasses the CSC always */
> >   	if (!intel_format_is_yuv(fb->format->format))
> >   		return;
> >   
> > -	/*
> > -	 * BT.601 full range YCbCr -> full range RGB
> > -	 *
> > -	 * |r|   | 5743 4096     0|   |cr|
> > -	 * |g| = |-2925 4096 -1410| x |y |
> > -	 * |b|   |    0 4096  7258|   |cb|
> > -	 *
> > -	 * Cb and Cr apparently come in as signed already,
> > -	 * and we get full range data in on account of CLRC0/1
> > -	 */
> >   	I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >   	I915_WRITE_FW(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >   	I915_WRITE_FW(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >   
> > -	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4096) | SPCSC_C0(5743));
> > -	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-2925) | SPCSC_C0(0));
> > -	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1410) | SPCSC_C0(4096));
> > -	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4096) | SPCSC_C0(0));
> > -	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(7258));
> > +	I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(csc[1]) | SPCSC_C0(csc[0]));
> > +	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(csc[3]) | SPCSC_C0(csc[2]));
> > +	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(csc[5]) | SPCSC_C0(csc[4]));
> > +	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(csc[7]) | SPCSC_C0(csc[6]));
> > +	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(csc[8]));
> >   
> >   	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> >   	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> > @@ -445,6 +460,9 @@ static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >   		return 0;
> >   	}
> >   
> > +	if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > +		sprctl |= SP_YUV_CSC_FORMAT_BT709;
> > +
> >   	if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> >   		sprctl |= SP_TILED;
> >   
> > @@ -578,6 +596,9 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >   		return 0;
> >   	}
> >   
> > +	if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > +		sprctl |= SPRITE_YUV_CSC_FORMAT_BT709;
> > +
> >   	if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> >   		sprctl |= SPRITE_TILED;
> >   
> > @@ -715,6 +736,9 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >   		return 0;
> >   	}
> >   
> > +	if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > +		dvscntr |= DVS_YUV_CSC_FORMAT_BT709;
> > +
> >   	if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> >   		dvscntr |= DVS_TILED;
> >   
> > @@ -1221,6 +1245,13 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> >   					   DRM_MODE_ROTATE_0,
> >   					   supported_rotations);
> >   
> > +	drm_plane_create_color_properties(&intel_plane->base,
> > +					  BIT(DRM_COLOR_YCBCR_BT601) |
> > +					  BIT(DRM_COLOR_YCBCR_BT709),
> > +					  BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> > +					  DRM_COLOR_YCBCR_BT601,
> > +					  DRM_COLOR_YCBCR_LIMITED_RANGE);
> > +
> >   	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> >   
> >   	return intel_plane;

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx