Re: [PATCH 48/62] drm/i915/bdw: Add Broadwell display FIFO limits

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

On Mon, Nov 04, 2013 at 11:39:56AM +0200, Jani Nikula wrote:
> On Sun, 03 Nov 2013, Ben Widawsky <benjamin.widawsky@intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Broadwell has bigger display FIFOs than Haswell. Otherwise the
> > two are very similar.
> >
> > v2: Fix FBC WM_LP shift for BDW
> >
> > v3: Rebase on top of the big Haswell wm rework.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> (v2)
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++---------
> >  2 files changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 6f834b3..2a65f92 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3366,6 +3366,7 @@
> >  #define  WM1_LP_LATENCY_MASK	(0x7f<<24)
> >  #define  WM1_LP_FBC_MASK	(0xf<<20)
> >  #define  WM1_LP_FBC_SHIFT	20
> > +#define  WM1_LP_FBC_SHIFT_BDW	19
> >  #define  WM1_LP_SR_MASK		(0x7ff<<8)
> 
> Meh, the above _MASKs would need some love too. WM1_LP_SR_MASK is wrong
> for HSW already I think. But nobody's using them, so not a blocker for
> this patch.
> 
> >  #define  WM1_LP_SR_SHIFT	8
> >  #define  WM1_LP_CURSOR_MASK	(0xff)
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 68dc363..6d14182 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2291,7 +2291,9 @@ static uint32_t ilk_compute_fbc_wm(const struct hsw_pipe_wm_parameters *params,
> >  
> >  static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
> >  {
> > -	if (INTEL_INFO(dev)->gen >= 7)
> > +	if (INTEL_INFO(dev)->gen >= 8)
> > +		return 3072;
> 
> It's probably just me, but I couldn't find this in the spec, so can't
> verify.

Looks like it's not spelled out there anymore. But you can figure it out
by looking at the single pipe primary:sprite 1:1 split numbers
(1536 * 2 = 3072) or the multi pipe primary only numbers (1024 * 3 = 3072).

> Apart from that,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> ...but read on, some non-blocking bikeshedding below.
> 
> > +	else if (INTEL_INFO(dev)->gen >= 7)
> >  		return 768;
> >  	else
> >  		return 512;
> > @@ -2336,7 +2338,9 @@ static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
> >  	}
> >  
> >  	/* clamp to max that the registers can hold */
> > -	if (INTEL_INFO(dev)->gen >= 7)
> > +	if (INTEL_INFO(dev)->gen >= 8)
> > +		max = level == 0 ? 255 : 2047;
> 
> Not having looked at the WM stuff before, it took me a while to find the
> registers and check the maximums. Which made me wonder why we don't fix
> the masks and use them here, so it would be bloody obvious what we're
> referring to?
> 
> if (level)
> 	max = WM1_LP_SR_MASK_BDW >> WM1_LP_SR_SHIFT_BDW;
> else
> 	max = WM0_PIPE_PLANE_MASK_BDW >> WM0_PIPE_PLANE_SHIFT;

I just extended the masks to cover all platforms. That makes hw
state readout a bit simpler since I don't need to worry about the
differences between generations there. But that means the masks
don't necessarily correspond to any specific platform, and so we
can't use them here. I could define per-platforms masks too, but
that seems rather pointless since there would be but one user.

> 
> > +	else if (INTEL_INFO(dev)->gen >= 7)
> >  		/* IVB/HSW primary/sprite plane watermarks */
> >  		max = level == 0 ? 127 : 1023;
> >  	else if (!is_sprite)
> > @@ -2366,10 +2370,13 @@ static unsigned int ilk_cursor_wm_max(const struct drm_device *dev,
> >  }
> >  
> >  /* Calculate the maximum FBC watermark */
> > -static unsigned int ilk_fbc_wm_max(void)
> > +static unsigned int ilk_fbc_wm_max(struct drm_device *dev)
> >  {
> >  	/* max that registers can hold */
> > -	return 15;
> > +	if (INTEL_INFO(dev)->gen >= 8)
> > +		return 31;
> > +	else
> > +		return 15;
> >  }
> >  
> >  static void ilk_compute_wm_maximums(struct drm_device *dev,
> > @@ -2381,7 +2388,7 @@ static void ilk_compute_wm_maximums(struct drm_device *dev,
> >  	max->pri = ilk_plane_wm_max(dev, level, config, ddb_partitioning, false);
> >  	max->spr = ilk_plane_wm_max(dev, level, config, ddb_partitioning, true);
> >  	max->cur = ilk_cursor_wm_max(dev, level, config);
> > -	max->fbc = ilk_fbc_wm_max();
> > +	max->fbc = ilk_fbc_wm_max(dev);
> >  }
> >  
> >  static bool ilk_validate_wm_level(int level,
> > @@ -2722,10 +2729,18 @@ static void hsw_compute_wm_results(struct drm_device *dev,
> >  		if (!r->enable)
> >  			break;
> >  
> > -		results->wm_lp[wm_lp - 1] = HSW_WM_LP_VAL(level * 2,
> > -							  r->fbc_val,
> > -							  r->pri_val,
> > -							  r->cur_val);
> 
> This leaves HSW_WM_LP_VAL() macro unused.

Yeah. I should just kill it.

> 
> > +		results->wm_lp[wm_lp - 1] = WM3_LP_EN |
> > +			((level * 2) << WM1_LP_LATENCY_SHIFT) |
> > +			(r->pri_val << WM1_LP_SR_SHIFT) |
> > +			r->cur_val;
> > +
> > +		if (INTEL_INFO(dev)->gen >= 8)
> > +			results->wm_lp[wm_lp - 1] |=
> > +				r->fbc_val << WM1_LP_FBC_SHIFT_BDW;
> > +		else
> > +			results->wm_lp[wm_lp - 1] |=
> > +				r->fbc_val << WM1_LP_FBC_SHIFT;
> > +
> >  		results->wm_lp_spr[wm_lp - 1] = r->spr_val;
> >  	}
> >  
> > -- 
> > 1.8.4.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC