Re: [PATCH 04/26] drm/i915: ValleyView watermark support

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, 23 Mar 2012 10:51:03 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Mar 22, 2012 at 08:29:30PM -0700, Ben Widawsky wrote:
> > On Thu, Mar 22, 2012 at 02:38:46PM -0700, Jesse Barnes wrote:
> > > Add support for ValleyView watermark handling.  It's like Cantiga
> > > with a few small differences (big FIFO mode and different WM
> > > limits).
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h      |    9 +++++
> > >  drivers/gpu/drm/i915/intel_display.c |   65
> > > ++++++++++++++++++++++++++++++++++ 2 files changed, 74
> > > insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h index f3609f2..0540099 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1043,6 +1043,12 @@
> > >  #define RAMCLK_GATE_D		0x6210		/*
> > > CRL only */ #define DEUC
> > > 0x6214          /* CRL only */ 
> > > +#define FW_BLC_SELF_VLV		0x6500
> > > +#define  FW_CSPWRDWNEN		(1<<15)
> > > +#define MI_ARB_VLV		0x6504
> > > +#define  DISP_TRICKLE_FEED_DIS	(1<<2)
> > > +#define CZCLK_CDCLK_FREQ_RATIO_VLV	0x6508
> > > +
> > >  /*
> > >   * Palette regs
> > >   */
> > 
> > Seems like the bottom 3 of these aren't used anywhere.
> 
> I honestly don't mind a few extra #defines, as long as someone has
> bothered to cross check them with bspec (to avoid another MI_WTF). At
> least if they're sounding somewhat relevant.
> -Daniel

Okay, seriously - fix mutt so it actually responds To: me.

The reason I'm only acking most of the patches thus far is I can't find
the register definitions. Jesse said some exist in some doc, and
others were just told to him by the design team. OTOH, anything with my
r-b has been crossed checked with the bspec.

Also, since we got into the discussion RIGHT BEFORE the vlv patches
about how messy our code is, I was being extra diligent about cutting
out stuff we don't use, since the odds of someone doing it after the
fact are slim to none. Furthermore, these defines are so terribly
named, the odds of someone creating another set of defines for the same
purpose are greater than 0.

~Ben

> 
> > 
> > > @@ -2495,6 +2501,7 @@
> > >  #define I915_FIFO_LINE_SIZE	64
> > >  #define I830_FIFO_LINE_SIZE	32
> > >  
> > > +#define VALLEYVIEW_FIFO_SIZE	255
> > >  #define G4X_FIFO_SIZE		127
> > >  #define I965_FIFO_SIZE		512
> > >  #define I945_FIFO_SIZE		127
> > > @@ -2502,6 +2509,7 @@
> > >  #define I855GM_FIFO_SIZE	127 /* In cachelines */
> > >  #define I830_FIFO_SIZE		95
> > >  
> > > +#define VALLEYVIEW_MAX_WM	0xff
> > >  #define G4X_MAX_WM		0x3f
> > >  #define I915_MAX_WM		0x3f
> > >  
> > > @@ -2516,6 +2524,7 @@
> > >  #define PINEVIEW_CURSOR_DFT_WM	0
> > >  #define PINEVIEW_CURSOR_GUARD_WM	5
> > >  
> > > +#define VALLEYVIEW_CURSOR_MAX_WM 64
> > >  #define I965_CURSOR_FIFO	64
> > >  #define I965_CURSOR_MAX_WM	32
> > >  #define I965_CURSOR_DFT_WM	8
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c index de1ba19..daa8853
> > > 100644 --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3606,6 +3606,20 @@ static const struct intel_watermark_params
> > > g4x_cursor_wm_info = { 2,
> > >  	G4X_FIFO_LINE_SIZE,
> > >  };
> > > +static const struct intel_watermark_params valleyview_wm_info = {
> > > +	VALLEYVIEW_FIFO_SIZE,
> > > +	VALLEYVIEW_MAX_WM,
> > > +	VALLEYVIEW_MAX_WM,
> > > +	2,
> > > +	G4X_FIFO_LINE_SIZE,
> > > +};
> > > +static const struct intel_watermark_params
> > > valleyview_cursor_wm_info = {
> > > +	I965_CURSOR_FIFO,
> > > +	VALLEYVIEW_CURSOR_MAX_WM,
> > > +	I965_CURSOR_DFT_WM,
> > > +	2,
> > > +	G4X_FIFO_LINE_SIZE,
> > > +};
> > >  static const struct intel_watermark_params i965_cursor_wm_info =
> > > { I965_CURSOR_FIFO,
> > >  	I965_CURSOR_MAX_WM,
> > > @@ -4130,6 +4144,55 @@ static bool g4x_compute_srwm(struct
> > > drm_device *dev, 
> > >  #define single_plane_enabled(mask) is_power_of_2(mask)
> > >  
> > > +static void valleyview_update_wm(struct drm_device *dev)
> > > +{
> > > +	static const int sr_latency_ns = 12000;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
> > > +	int plane_sr, cursor_sr;
> > > +	unsigned int enabled = 0;
> > > +
> > > +	if (g4x_compute_wm0(dev, 0,
> > > +			    &valleyview_wm_info, latency_ns,
> > > +			    &valleyview_cursor_wm_info,
> > > latency_ns,
> > > +			    &planea_wm, &cursora_wm))
> > > +		enabled |= 1;
> > > +
> > > +	if (g4x_compute_wm0(dev, 1,
> > > +			    &valleyview_wm_info, latency_ns,
> > > +			    &valleyview_cursor_wm_info,
> > > latency_ns,
> > > +			    &planeb_wm, &cursorb_wm))
> > > +		enabled |= 2;
> > > +
> > > +	plane_sr = cursor_sr = 0;
> > > +	if (single_plane_enabled(enabled) &&
> > > +	    g4x_compute_srwm(dev, ffs(enabled) - 1,
> > > +			     sr_latency_ns,
> > > +			     &valleyview_wm_info,
> > > +			     &valleyview_cursor_wm_info,
> > > +			     &plane_sr, &cursor_sr))
> > > +		I915_WRITE(FW_BLC_SELF_VLV, FW_CSPWRDWNEN);
> > > +	else
> > > +		I915_WRITE(FW_BLC_SELF_VLV,
> > > +			   I915_READ(FW_BLC_SELF_VLV) &
> > > ~FW_CSPWRDWNEN); +
> > > +	DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d,
> > > cursor=%d, B: plane=%d, cursor=%d, SR: plane=%d, cursor=%d\n",
> > > +		      planea_wm, cursora_wm,
> > > +		      planeb_wm, cursorb_wm,
> > > +		      plane_sr, cursor_sr);
> > > +
> > > +	I915_WRITE(DSPFW1,
> > > +		   (plane_sr << DSPFW_SR_SHIFT) |
> > > +		   (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> > > +		   (planeb_wm << DSPFW_PLANEB_SHIFT) |
> > > +		   planea_wm);
> > > +	I915_WRITE(DSPFW2,
> > > +		   (I915_READ(DSPFW2) & DSPFW_CURSORA_MASK) |
> > > +		   (cursora_wm << DSPFW_CURSORA_SHIFT));
> > > +	I915_WRITE(DSPFW3,
> > > +		   (I915_READ(DSPFW3) | (cursor_sr <<
> > > DSPFW_CURSOR_SR_SHIFT))); +}
> > > +
> > >  static void g4x_update_wm(struct drm_device *dev)
> > >  {
> > >  	static const int sr_latency_ns = 12000;
> > > @@ -8917,6 +8980,8 @@ static void intel_init_display(struct
> > > drm_device *dev) dev_priv->display.write_eld = ironlake_write_eld;
> > >  		} else
> > >  			dev_priv->display.update_wm = NULL;
> > > +	} else if (IS_VALLEYVIEW(dev)) {
> > > +		dev_priv->display.update_wm =
> > > valleyview_update_wm; } else if (IS_PINEVIEW(dev)) {
> > >  		if (!intel_get_cxsr_latency(IS_PINEVIEW_G(dev),
> > >  					    dev_priv->is_ddr3,
> > 
> > Aside from the extraneous #defines
> > Acked-by: Ben Widawsky <ben@bwidawsk.net>
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>