[Intel-gfx] [PATCH] drm/i915/skl: Increase ddb blocks to support large cursor sizes

Mon Dec 21 04:09:38 PST 2015

On Fri, Dec 18, 2015 at 07:24:19AM -0800, Matt Roper wrote:
> On Fri, Dec 18, 2015 at 07:14:17AM -0800, Matt Roper wrote:
> > On Fri, Dec 18, 2015 at 05:10:12PM +0200, Ville Syrjälä wrote:
> > > On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote:
> > > > On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote:
> > > > > On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada wrote:
> > > > > > Original value of 32 blocks is not sufficient when using cursor size of
> > > > > > 256x256 causing FIFO underruns when the reworked wm
> > > > > > caluclations in
> > > > > > 
> > > > > > commit 024c9045221fe45482863c47c4b4c47d37f97cbf
> > > > > > Author: Matt Roper <matthew.d.roper at intel.com>
> > > > > > Date:   Thu Sep 24 15:53:11 2015 -0700
> > > > > > 
> > > > > >     drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4)
> > > > > 
> > > > > Well that commit is obviously incorrect. It's now using the pipe src
> > > > > width as the plane width for all planes.
> > > > > 
> > > > 
> > > > Yeah, we already noted that bug in another email thread, but decided
> > > > that it was unrelated to the problems Radhakrishna is facing.
> > > > Radhakrishna is only using a cursor (which doesn't use that buggy
> > > > function)
> > > 
> > > Pop quiz: what does it use then?
> > 
> > All non-cursor planes (i.e., primary+sprite).  Cursors use a fixed DDB
> > allocation (currently 32 blocks as suggested by bspec, but
> > Radhakrishna's testing has found this to be too small, so his patch here
> > is bumping that number up.
> > 
> 
> To elaborate on this some more, we have some suspicions about why the
> bspec-suggested 32 blocks might not be enough.  There's some new
> workaround information in the bspec about sometimes needing to disable
> the system agent (SAGV) on non-BXT gen9, depending on the configuration
> and that's not something that's been implemented in our driver code yet.
> There's also another new workaround that checks raw system memory
> bandwidth and adjusts watermark programming that hasn't been implemented
> yet either.  It could be that one of those two missing workarounds is
> the true culprit, but Radhakrishna's patch here looks like a reasonable
> short-term workaround; my main worry is that if he needs to bump the
> hardcoded 32 up to to 52 for the single pipe case, he might also need to
> bump the hardcoded 8 up as well for the multi-pipe case; I don't think
> anyone has tested that yet (and this seems to be a SKL-specific problem,
> so I can't reproduce it on my BXT).

This needs at least a very big comment that we're just hacking around with
duct-tape. Also allocating DDB shouldn't matter for correctness, as long
as we compute the WM values correctly. I wonder how this fixes anything
really (except making it a bit harder to hit perhaps due to the enlarged
buffering it affords for the cursor).

Imo better to just implement the other workarounds first, then see what's
left.
-Daniel

> 
> 
> Matt
> 
> > Primary and sprite planes are supposed to divide up the remaining blocks
> > proportional to their size, but the bug here causes them to all be
> > considered full-screen size.  If you're not actually using sprites and
> > not windowing your primary plane, then the bug has no effect (which is
> > probably why we didn't already catch and fix it).  If you do use a
> > sprite plane or window your primary plane, your proportions are probably
> > wrong and you get non-optimal settings, although even then you usually
> > won't have actual problems.
> > 
> > 
> > Matt
> > 
> > 
> > > 
> > > > and a full-screen primary plane, so the numbers don't actually
> > > > change (for his use case).
> > > > 
> > > > Of course that bug is still worth fixing too; I was planning on writing
> > > > up a patch for it later today.
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > 
> > > > > > 
> > > > > > are used. Increasing the number of blocks to 52 to make cursor plane tolerate
> > > > > > SAGV block time for the maximum possible cursor size.
> > > > > > 
> > > > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > > > > > Signed-off-by: Kalyan Kondapally <kalyan.kondapally at intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index d385d99..137fb68 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> > > > > >  static unsigned int skl_cursor_allocation(const struct intel_wm_config *config)
> > > > > >  {
> > > > > >  	if (config->num_pipes_active == 1)
> > > > > > -		return 32;
> > > > > > +		return 52;
> > > > > >  
> > > > > >  	return 8;
> > > > > >  }
> > > > > > -- 
> > > > > > 1.9.1
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx at lists.freedesktop.org
> > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > > 
> > > > > -- 
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > > > 
> > > > -- 
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > IoTG Platform Enabling & Development
> > > > Intel Corporation
> > > > (916) 356-2795
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch