Re: [PATCH] drm/i915: Implement pipe CSC based limited range RGB output

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

On Fri, Jan 18, 2013 at 06:37:09PM +0100, Daniel Vetter wrote:
> On Fri, Jan 18, 2013 at 6:11 PM,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > HSW no longer has the PIPECONF bit for limited range RGB output.
> > Instead the pipe CSC unit must be used to perform that task.
> >
> > The CSC pre offset are set to 0, since the incoming data is full
> > [0:255] range RGB, the coefficients are programmed to compress the
> > data into [0:219] range, and then we use either the CSC_MODE black
> > screen offset bit, or the CSC post offsets to shift the data to
> > the correct [16:235] range.
> >
> > Also have to change the confiuration of all planes so that the
> > data is sent through the pipe CSC unit. For simplicity send the
> > plane data through the pipe CSC unit always, and in case full
> > range output is requested, the pipe CSC unit is set up with an
> > identity transform to pass the plane data through unchanged.
> >
> > I've been told by some hardware people that the use of the pipe
> > CSC unit shouldn't result in any measurable increase in power
> > consumption numbers.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > Note that I haven't actually tested this on HSW. I did test the earlier
> > prototype version on ILK and IVB. The pipe CSC unit on ILK isn't programmed
> > in quite the same as on HSW, but the IVB unit _should_ be identical to HSW.
> >
> > The main risk involves the coefficient registers. If the channel mapping
> > changed for some reason, we could get swapped channels. For some reason
> > reality and documenation didn't seem to agree how the channels are mapped
> > even on ILK and IVB. So I'd like someone to try this out on HSW to make
> > sure the output still looks correct.
> 
> Two questions from the clueless:
> - Since there's some question whether the CSC blows through additional
> power: Can't we just enable it only when we need it? It looks like
> updating the broadcast property requires a full modeset anyway, so we
> don't gain anything by keeping the csc on when not required.

We could, but it's a bit more work since we need to look at the
adjusted_mode private_flags. So we need to pass that somehow to 
all plane code.

For cursors we could pass adjusted_mode from the mode_set path,
and hwmode from the cursor ioctls. And since sprites aren't
currently integrated into the mode_set sequence at all, we
could just look at hwmode there. So I suppose it's doable.

Or we could just set the bit for all planes on the pipe in mode_set,
the same way as it's done for DSPCNTR, and then hope all other code
uses RMW when changing the CNTR regs.

> - I might just be mislead by the 256/219 values, but: How does this
> work for non-8bpc pipe configurations? Or does the CSC thing not care
> and only scale the normalized 0.0 - 1.0 range?

Yeah, it uses normalized numbers. But as stated in the comment I don't
really know what the exact arithmetic inside the HW is, so I can't be
sure if the values I've calculated are the best ones. We might be
slighly over/undershooting the limits.

> Cheers, Daniel
> 
> >
> >  drivers/gpu/drm/i915/i915_reg.h      |   52 ++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_display.c |   71 +++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_sprite.c  |    3 +
> >  3 files changed, 124 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index a2550c5..63ebda8 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2944,6 +2944,7 @@
> >  #define   CURSOR_ENABLE                0x80000000
> >  #define   CURSOR_GAMMA_ENABLE  0x40000000
> >  #define   CURSOR_STRIDE_MASK   0x30000000
> > +#define   CURSOR_PIPE_CSC_ENABLE (1<<24)
> >  #define   CURSOR_FORMAT_SHIFT  24
> >  #define   CURSOR_FORMAT_MASK   (0x07 << CURSOR_FORMAT_SHIFT)
> >  #define   CURSOR_FORMAT_2C     (0x00 << CURSOR_FORMAT_SHIFT)
> > @@ -3005,6 +3006,7 @@
> >  #define   DISPPLANE_RGBA888                    (0xf<<26)
> >  #define   DISPPLANE_STEREO_ENABLE              (1<<25)
> >  #define   DISPPLANE_STEREO_DISABLE             0
> > +#define   DISPPLANE_PIPE_CSC_ENABLE            (1<<24)
> >  #define   DISPPLANE_SEL_PIPE_SHIFT             24
> >  #define   DISPPLANE_SEL_PIPE_MASK              (3<<DISPPLANE_SEL_PIPE_SHIFT)
> >  #define   DISPPLANE_SEL_PIPE_A                 0
> > @@ -3093,6 +3095,7 @@
> >  #define   DVS_FORMAT_RGBX101010        (1<<25)
> >  #define   DVS_FORMAT_RGBX888   (2<<25)
> >  #define   DVS_FORMAT_RGBX161616        (3<<25)
> > +#define   DVS_PIPE_CSC_ENABLE   (1<<24)
> >  #define   DVS_SOURCE_KEY       (1<<22)
> >  #define   DVS_RGB_ORDER_XBGR   (1<<20)
> >  #define   DVS_YUV_BYTE_ORDER_MASK (3<<16)
> > @@ -3160,7 +3163,7 @@
> >  #define   SPRITE_FORMAT_RGBX161616     (3<<25)
> >  #define   SPRITE_FORMAT_YUV444         (4<<25)
> >  #define   SPRITE_FORMAT_XR_BGR101010   (5<<25) /* Extended range */
> > -#define   SPRITE_CSC_ENABLE            (1<<24)
> > +#define   SPRITE_PIPE_CSC_ENABLE       (1<<24)
> >  #define   SPRITE_SOURCE_KEY            (1<<22)
> >  #define   SPRITE_RGB_ORDER_RGBX                (1<<20) /* only for 888 and 161616 */
> >  #define   SPRITE_YUV_TO_RGB_CSC_DISABLE        (1<<19)
> > @@ -4635,4 +4638,51 @@
> >  #define  WM_DBG_DISALLOW_MAXFIFO       (1<<1)
> >  #define  WM_DBG_DISALLOW_SPRITE                (1<<2)
> >
> > +/* pipe CSC */
> > +#define _PIPE_A_CSC_COEFF_RY_GY        0x49010
> > +#define _PIPE_A_CSC_COEFF_BY   0x49014
> > +#define _PIPE_A_CSC_COEFF_RU_GU        0x49018
> > +#define _PIPE_A_CSC_COEFF_BU   0x4901c
> > +#define _PIPE_A_CSC_COEFF_RV_GV        0x49020
> > +#define _PIPE_A_CSC_COEFF_BV   0x49024
> > +#define _PIPE_A_CSC_MODE       0x49028
> > +#define _PIPE_A_CSC_PREOFF_HI  0x49030
> > +#define _PIPE_A_CSC_PREOFF_ME  0x49034
> > +#define _PIPE_A_CSC_PREOFF_LO  0x49038
> > +#define _PIPE_A_CSC_POSTOFF_HI 0x49040
> > +#define _PIPE_A_CSC_POSTOFF_ME 0x49044
> > +#define _PIPE_A_CSC_POSTOFF_LO 0x49048
> > +
> > +#define _PIPE_B_CSC_COEFF_RY_GY        0x49110
> > +#define _PIPE_B_CSC_COEFF_BY   0x49114
> > +#define _PIPE_B_CSC_COEFF_RU_GU        0x49118
> > +#define _PIPE_B_CSC_COEFF_BU   0x4911c
> > +#define _PIPE_B_CSC_COEFF_RV_GV        0x49120
> > +#define _PIPE_B_CSC_COEFF_BV   0x49124
> > +#define _PIPE_B_CSC_MODE       0x49128
> > +#define _PIPE_B_CSC_PREOFF_HI  0x49130
> > +#define _PIPE_B_CSC_PREOFF_ME  0x49134
> > +#define _PIPE_B_CSC_PREOFF_LO  0x49138
> > +#define _PIPE_B_CSC_POSTOFF_HI 0x49140
> > +#define _PIPE_B_CSC_POSTOFF_ME 0x49144
> > +#define _PIPE_B_CSC_POSTOFF_LO 0x49148
> > +
> > +#define CSC_BLACK_SCREEN_OFFSET (1 << 2)
> > +#define CSC_POSITION_BEFORE_GAMMA (1 << 1)
> > +#define CSC_MODE_YUV_TO_RGB (1 << 0)
> > +
> > +#define PIPE_CSC_COEFF_RY_GY(pipe) _PIPE(pipe, _PIPE_A_CSC_COEFF_RY_GY, _PIPE_B_CSC_COEFF_RY_GY)
> > +#define PIPE_CSC_COEFF_BY(pipe) _PIPE(pipe, _PIPE_A_CSC_COEFF_BY, _PIPE_B_CSC_COEFF_BY)
> > +#define PIPE_CSC_COEFF_RU_GU(pipe) _PIPE(pipe, _PIPE_A_CSC_COEFF_RU_GU, _PIPE_B_CSC_COEFF_RU_GU)
> > +#define PIPE_CSC_COEFF_BU(pipe) _PIPE(pipe, _PIPE_A_CSC_COEFF_BU, _PIPE_B_CSC_COEFF_BU)
> > +#define PIPE_CSC_COEFF_RV_GV(pipe) _PIPE(pipe, _PIPE_A_CSC_COEFF_RV_GV, _PIPE_B_CSC_COEFF_RV_GV)
> > +#define PIPE_CSC_COEFF_BV(pipe) _PIPE(pipe, _PIPE_A_CSC_COEFF_BV, _PIPE_B_CSC_COEFF_BV)
> > +#define PIPE_CSC_MODE(pipe) _PIPE(pipe, _PIPE_A_CSC_MODE, _PIPE_B_CSC_MODE)
> > +#define PIPE_CSC_PREOFF_HI(pipe) _PIPE(pipe, _PIPE_A_CSC_PREOFF_HI, _PIPE_B_CSC_PREOFF_HI)
> > +#define PIPE_CSC_PREOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_PREOFF_ME, _PIPE_B_CSC_PREOFF_ME)
> > +#define PIPE_CSC_PREOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_PREOFF_LO, _PIPE_B_CSC_PREOFF_LO)
> > +#define PIPE_CSC_POSTOFF_HI(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_HI, _PIPE_B_CSC_POSTOFF_HI)
> > +#define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
> > +#define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
> > +
> >  #endif /* _I915_REG_H_ */
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f48f698..f22b0a8 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5106,6 +5106,71 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
> >         POSTING_READ(PIPECONF(pipe));
> >  }
> >
> > +/*
> > + * Set up the pipe CSC unit.
> > + *
> > + * Currently only full range RGB to limited range RGB conversion
> > + * is supported, but eventually this should handle various
> > + * RGB<->YCbCr scenarios as well.
> > + */
> > +static void intel_set_pipe_csc(struct drm_crtc *crtc,
> > +                              const struct drm_display_mode *adjusted_mode)
> > +{
> > +       struct drm_device *dev = crtc->dev;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +       int pipe = intel_crtc->pipe;
> > +       uint16_t coeff = 0x7800; /* 1.0 */
> > +
> > +       /*
> > +        * TODO: Check what kind of values actually come out of the pipe
> > +        * with these coeff/postoff values and adjust to get the best
> > +        * accuracy. Perhaps we even need to take the bpc value into
> > +        * consideration.
> > +        */
> > +
> > +       if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
> > +               coeff = ((235 - 16) * (1 << 12) / 255) & 0xff8; /* 0.xxx... */
> > +
> > +       /*
> > +        * GY/GU and RY/RU should be the other way around according
> > +        * to BSpec, but reality doesn't agree. Just set them up in
> > +        * a way that results in the correct picture.
> > +        */
> > +       I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeff << 16);
> > +       I915_WRITE(PIPE_CSC_COEFF_BY(pipe), 0);
> > +
> > +       I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeff);
> > +       I915_WRITE(PIPE_CSC_COEFF_BU(pipe), 0);
> > +
> > +       I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), 0);
> > +       I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeff << 16);
> > +
> > +       I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
> > +       I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
> > +       I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
> > +
> > +       if (INTEL_INFO(dev)->gen > 6) {
> > +               uint16_t postoff = 0;
> > +
> > +               if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
> > +                       postoff = (16 * (1 << 13) / 255) & 0x1fff;
> > +
> > +               I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff);
> > +               I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
> > +               I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
> > +
> > +               I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> > +       } else {
> > +               uint32_t mode = CSC_MODE_YUV_TO_RGB;
> > +
> > +               if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_COLOR_RANGE)
> > +                       mode |= CSC_BLACK_SCREEN_OFFSET;
> > +
> > +               I915_WRITE(PIPE_CSC_MODE(pipe), mode);
> > +       }
> > +}
> > +
> >  static void haswell_set_pipeconf(struct drm_crtc *crtc,
> >                                  struct drm_display_mode *adjusted_mode,
> >                                  bool dither)
> > @@ -5670,8 +5735,10 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> >
> >         haswell_set_pipeconf(crtc, adjusted_mode, dither);
> >
> > +       intel_set_pipe_csc(crtc, adjusted_mode);
> > +
> >         /* Set up the display plane register */
> > -       I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
> > +       I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE | DISPPLANE_PIPE_CSC_ENABLE);
> >         POSTING_READ(DSPCNTR(plane));
> >
> >         ret = intel_pipe_set_base(crtc, x, y, fb);
> > @@ -6069,6 +6136,8 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> >                         cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> >                         cntl |= CURSOR_MODE_DISABLE;
> >                 }
> > +               if (IS_HASWELL(dev))
> > +                       cntl |= CURSOR_PIPE_CSC_ENABLE;
> >                 I915_WRITE(CURCNTR_IVB(pipe), cntl);
> >
> >                 intel_crtc->cursor_visible = visible;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index d7b060e..9dedf68 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -89,6 +89,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
> >         sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
> >         sprctl |= SPRITE_ENABLE;
> >
> > +       if (IS_HASWELL(dev))
> > +               sprctl |= SPRITE_PIPE_CSC_ENABLE;
> > +
> >         /* Sizes are 0 based */
> >         src_w--;
> >         src_h--;
> > --
> > 1.7.8.6
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC