On Thu, Jun 15, 2017 at 06:08:43PM +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 6/9/2017 2:03 AM, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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
> >
> > Cc: Jyri Sarha <jsarha@xxxxxx>
> > Cc: "Tang, Jun" <jun.tang@xxxxxxxxx>
> > Reported-by: "Tang, Jun" <jun.tang@xxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > 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 b6d69e289974..ce7c0dc79cf7 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5777,6 +5777,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 */
> > +#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 _SPAGAMC (VLV_DISPLAY_BASE + 0x721f4)
> >
> > #define _SPBCNTR (VLV_DISPLAY_BASE + 0x72280)
> > @@ -5790,6 +5796,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) \
> > @@ -5806,6 +5814,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..2ce3b3c6ffa8 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 con, bri, sh_sin, sh_cos;
> > +
> con = contrast bri = brightness for better reading ?
Sure, why not.
> > + if (format_is_yuv(fb->format->format)) {
> > + /*
> > + * expand limited range to full range.
> > + * contrast is applied first, then brightness
> > + */
> > + con = ((255 << 7) / 219 + 1) >> 1;
> > + bri = -((16 << 1) * 255 / 219 + 1) >> 1;
> > + sh_sin = 0;
> > + sh_cos = (((128 << 8) / 112) + 1) >> 1;
> > + } else {
> > + /* pass-through everything */
> > + con = 1 << 6;
> > + bri = 0;
> > + sh_sin = 0;
> > + sh_cos = 1 << 7;
> > + }
> contrast and brightness are mostly used for color / display correction.
> Would it be better to create a
> plane level property for the same, and attach into color management
> framework ? In this way, userspace
> can also play around.
I did have such plans long ago, but I'm not sure there's much point in
doing that since we would only be able to support these propeerties
on a very limited subset of planes on a very limited subset of platforms.
>
> - Shashank
> > +
> > + /* FIXME these register are single buffered :( */
> > + I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> > + SP_CONTRAST(con) | SP_BRIGHTNESS(bri));
> > + 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