From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Implement pipe CSC based limited range RGB output Date: Fri, 18 Jan 2013 20:08:33 +0200 Message-ID: <20130118180833.GB3503@intel.com> References: <1358529098-31596-1-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id CDA43E5C02 for ; Fri, 18 Jan 2013 10:08:36 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Jan 18, 2013 at 06:37:09PM +0100, Daniel Vetter wrote: > On Fri, Jan 18, 2013 at 6:11 PM, wrote: > > From: Ville Syrj=E4l=E4 > > > > 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=E4l=E4 > > --- > > 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 progra= mmed > > 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 mapp= ed > > 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/i91= 5_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< > #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, _PIP= E_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, _PIP= E_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, _PIP= E_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, _P= IPE_B_CSC_PREOFF_HI) > > +#define PIPE_CSC_PREOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_PREOFF_ME, _P= IPE_B_CSC_PREOFF_ME) > > +#define PIPE_CSC_PREOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_PREOFF_LO, _P= IPE_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/i91= 5/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_crt= c *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 =3D crtc->dev; > > + struct drm_i915_private *dev_priv =3D dev->dev_private; > > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > > + int pipe =3D intel_crtc->pipe; > > + uint16_t coeff =3D 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_RAN= GE) > > + coeff =3D ((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 =3D 0; > > + > > + if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_C= OLOR_RANGE) > > + postoff =3D (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 =3D CSC_MODE_YUV_TO_RGB; > > + > > + if (adjusted_mode->private_flags & INTEL_MODE_LIMITED_C= OLOR_RANGE) > > + mode |=3D 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_P= IPE_CSC_ENABLE); > > POSTING_READ(DSPCNTR(plane)); > > > > ret =3D intel_pipe_set_base(crtc, x, y, fb); > > @@ -6069,6 +6136,8 @@ static void ivb_update_cursor(struct drm_crtc *cr= tc, u32 base) > > cntl &=3D ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); > > cntl |=3D CURSOR_MODE_DISABLE; > > } > > + if (IS_HASWELL(dev)) > > + cntl |=3D CURSOR_PIPE_CSC_ENABLE; > > I915_WRITE(CURCNTR_IVB(pipe), cntl); > > > > intel_crtc->cursor_visible =3D 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 |=3D SPRITE_TRICKLE_FEED_DISABLE; > > sprctl |=3D SPRITE_ENABLE; > > > > + if (IS_HASWELL(dev)) > > + sprctl |=3D 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=E4l=E4 Intel OTC