Re: [PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV

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

On Tue, Jun 20, 2017 at 08:13:42PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 6/20/2017 8:04 PM, Ville Syrjälä wrote:
> > On Tue, Jun 20, 2017 at 07:27:54PM +0530, Sharma, Shashank wrote:
> >> Regards
> >> Shashank
> >>
> >> On 6/20/2017 7:02 PM, ville.syrjala@linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Turns out the VLV/CHV fixed function sprite CSC expects full range
> >>> data as input. We've been feeding it limited range data to it all
> >>> along. To expand the data out to full range we'll use the color
> >>> correction registers (brightness, contrast, and saturation).
> >>>
> >>> On CHV pipe B we were actually doing the right thing already because we
> >>> progammed the custom CSC matrix to do expect limited range input. Now
> >>> that well pre-expand the data out with the color correction unit, we
> >>> need to change the CSC matrix to operate with full range input instead.
> >>>
> >>> This should make the sprite output of the other pipes match the sprite
> >>> output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> >>> there can be a slight difference in the output, but as I don't know
> >>> the formula used by the fixed function CSC of the other pipes, I don't
> >>> think it's worth the effort to try to match the output exactly. It
> >>> might not even be possible due to difference in internal precision etc.
> >>>
> >>> One slight caveat here is that the color correction registers are single
> >>> bufferred, so we should really be updating them during vblank, but we
> >>> still don't have a mechanism for that, so just toss in another FIXME.
> >>>
> >>> v2: Rebase
> >>> v3: s/bri/brightness/ s/con/contrast/ (Shashank)
> >>>
> >>> Cc: Shashank Sharma <shashank.sharma@intel.com>
> >>> Cc: Jyri Sarha <jsarha@ti.com>
> >>> Cc: "Tang, Jun" <jun.tang@intel.com>
> >>> Reported-by: "Tang, Jun" <jun.tang@intel.com>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_reg.h     | 10 +++++
> >>>    drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
> >>>    2 files changed, 66 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>> index c8647cfa81ba..290322588f56 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -5974,6 +5974,12 @@ enum {
> >>>    #define _SPATILEOFF		(VLV_DISPLAY_BASE + 0x721a4)
> >>>    #define _SPACONSTALPHA		(VLV_DISPLAY_BASE + 0x721a8)
> >>>    #define   SP_CONST_ALPHA_ENABLE		(1<<31)
> >>> +#define _SPACLRC0		(VLV_DISPLAY_BASE + 0x721d0)
> >>> +#define   SP_CONTRAST(x)		((x) << 18) /* u3.6 */
> >> protection for higher reserved bits ? From 27-31 ? something like ((x &
> >> 1FF) << 18) ?
> > It's unsigned, so no need.
> Humm .. doesn't stops me to pass a bigger value, which will be shifted 
> towards reserved bits ?

We don't hand hold developers quite that much. I did have some ideas in
the past that we might want to have some optional debug checks in these
things to trigger WARNs if a value overflows its bits. But I never got
as far as implementing anything like that.

> >
> >>> +#define   SP_BRIGHTNESS(x)		((x) & 0xff) /* s8 */
> >>> +#define _SPACLRC1		(VLV_DISPLAY_BASE + 0x721d4)
> >>> +#define   SP_SH_SIN(x)			(((x) & 0x7ff) << 16) /* s4.7 */
> >>> +#define   SP_SH_COS(x)			(x) /* u3.7 */
> >>>    
> >> #define   SP_SH_COS(x)			(x & 3FF) /* u3.7 */ ?
> > Also unsigned
> >
> >>> #define _SPAGAMC		(VLV_DISPLAY_BASE + 0x721f4)
> >>>    
> >>>    #define _SPBCNTR		(VLV_DISPLAY_BASE + 0x72280)
> >>> @@ -5987,6 +5993,8 @@ enum {
> >>>    #define _SPBKEYMAXVAL		(VLV_DISPLAY_BASE + 0x722a0)
> >>>    #define _SPBTILEOFF		(VLV_DISPLAY_BASE + 0x722a4)
> >>>    #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
> >>> +#define _SPBCLRC0		(VLV_DISPLAY_BASE + 0x722d0)
> >>> +#define _SPBCLRC1		(VLV_DISPLAY_BASE + 0x722d4)
> >>>    #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
> >>>    
> >>>    #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> >>> @@ -6003,6 +6011,8 @@ enum {
> >>>    #define SPKEYMAXVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> >>>    #define SPTILEOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
> >>>    #define SPCONSTALPHA(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> >>> +#define SPCLRC0(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> >>> +#define SPCLRC1(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
> >>>    #define SPGAMC(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
> >>>    
> >>>    /*
> >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>> index 0c650c2cbca8..4462408cc835 100644
> >>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>> @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
> >>>    }
> >>>    
> >>>    static void
> >>> -chv_update_csc(struct intel_plane *plane, uint32_t format)
> >>> +chv_update_csc(const struct intel_plane_state *plane_state)
> >>>    {
> >>> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >>>    	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;
> >>>    
> >>>    	/* Seems RGB data bypasses the CSC always */
> >>> -	if (!format_is_yuv(format))
> >>> +	if (!format_is_yuv(fb->format->format))
> >>>    		return;
> >>>    
> >>>    	/*
> >>> -	 * BT.601 limited range YCbCr -> full range RGB
> >>> +	 * BT.601 full range YCbCr -> full range RGB
> >>>    	 *
> >>> -	 * |r|   | 6537 4769     0|   |cr  |
> >>> -	 * |g| = |-3330 4769 -1605| x |y-64|
> >>> -	 * |b|   |    0 4769  8263|   |cb  |
> >>> +	 * |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, so no
> >>> -	 * need for any offset. For Y we need to remove the offset.
> >>> +	 * 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(-64));
> >>> +	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(4769) | SPCSC_C0(6537));
> >>> -	I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> >>> -	I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> >>> -	I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> >>> -	I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> >>> +	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(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> >>> -	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> >>> -	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> >>> +	I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> >>> +	I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> >>> +	I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> >>>    
> >>>    	I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >>>    	I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >>>    	I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >>>    }
> >>>    
> >>> +static void
> >>> +vlv_update_clrc(const struct intel_plane_state *plane_state)
> >>> +{
> >>> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >>> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >>> +	const struct drm_framebuffer *fb = plane_state->base.fb;
> >>> +	enum pipe pipe = plane->pipe;
> >>> +	enum plane_id plane_id = plane->id;
> >>> +	int contrast, brightness, sh_sin, sh_cos;
> >>> +
> >>> +	if (format_is_yuv(fb->format->format)) {
> >>> +		/*
> >>> +		 * expand limited range to full range.
> >>> +		 * contrast is applied first, then brightness
> >>> +		 */
> >> I would be happy to see some comment explaining the Brightness/Contrast
> >> calculation magic nos, or may be a hint for other developers.
> > Y: 16-235 * contrast + brightness -> ~0->255
> > CbCr: no hue adjustemnt -> 0 degree angle
> >        scale to expand CbCr range from -112-112 to -128-128
> >        sh_sin = sin(0) * 128/112 = 0
> >        sh_cos = cos(0) * 128/112 = whatever
> Yep, (fortunately I am aware of this formula :-)), this same in form of 
> a comment ?

Perhaps. If I can write it in a way that doesn't just confuse people
more :P

> With of without this comment, feel free to use:
> R-B: Shashank Sharma <shashank.sharma@intel.com>
> >>> +		contrast = ((255 << 7) / 219 + 1) >> 1;
> >>> +		brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
> >>> +		sh_sin = 0;
> >>> +		sh_cos = (((128 << 8) / 112) + 1) >> 1;
> >>> +	} else {
> >>> +		/* pass-through everything */
> >>> +		contrast = 1 << 6;
> >>> +		brightness = 0;
> >>> +		sh_sin = 0;
> >>> +		sh_cos = 1 << 7;
> >>> +	}
> >>> +
> >>> +	/* FIXME these register are single buffered :( */
> >>> +	I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> >>> +		      SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
> >>> +	I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> >>> +		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> >>> +}
> >>> +
> >>>    static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >>>    			  const struct intel_plane_state *plane_state)
> >>>    {
> >>> @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
> >>>    
> >>>    	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >>>    
> >>> +	vlv_update_clrc(plane_state);
> >>> +
> >>>    	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> >>> -		chv_update_csc(plane, fb->format->format);
> >>> +		chv_update_csc(plane_state);
> >>>    
> >>>    	if (key->flags) {
> >>>    		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);

-- 
Ville Syrjälä
Intel OTC