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

[Daniel Vetter]

On Mon, Jan 04, 2016 at 07:18:26PM +0530, Kumar, Shobhit wrote:
> On 12/23/2015 08:22 AM, Kumar, Shobhit wrote:
> >On 12/21/2015 05:39 PM, Daniel Vetter wrote:
> >>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).
> >
> >Agree Daniel, but in our detailed testing this is found to have fixed
> >reliably all known flickers. I recommend to have a detailed comment and
> >*put* the duct-tape. I can have a rerun of detailed tests again to
> >ensure that its indeed fixing everything.
> >
> >>
> >>Imo better to just implement the other workarounds first, then see what's
> >>left.
> >
> >I know there are few known workarounds pending. Lets continue
> >implementing them and more. But with Matt's patch not fixing the issue,
> >lets take this as an immediate short term fix and come back to remove
> >the duct-tape when we have correct fix.
> 
> I experimented and found that reverting this patch but having arbitrated
> display bandwidth based workaround to increase latency values for all levels
> is fixing the flicker. Will be working on this along with Mahesh, unless
> Matt, you have a patch for this already.

Excellent, really happy that we can go ahead with a reasonable looking fix
right away here.

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