Date:	Fri, 29 Jul 2016 15:46:07 -0400
From:	Lyude <cpaul@...hat.com>
To:	Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:	Daniel Vetter <daniel.vetter@...el.com>,
	intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	dri-devel@...ts.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

On Fri, 2016-07-29 at 22:26 +0300, Ville Syrjälä wrote:
> On Fri, Jul 29, 2016 at 02:48:09PM -0400, Lyude wrote:
> > 
> > So I've been working on trying to fix this entirely again (e.g.
> > writing
> > the ddb properly), since from bug reports it still doesn't sound
> > like
> > we've got enough workarounds to make this tolerable. I've shown
> > this to
> > matt roper, but I should probably post what I've been trying to do
> > for
> > you as well.
> > 
> > So the approach I came up with is here
> > 
> > https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
> > 
> > My approach differs a little bit from what the bspec recommends,
> > but I
> > think it's going to be a bit easier to implement. At the end of all
> > the
> > changes I'm attempting it should look like this:
> > 
> >  * We no longer have a global watermark update for skl
> >  * A new hook called "update_ddbs" is added to i915_display_funcs.
> > This
> >    gets called in intel_atomic_commit_tail() after we've disabled
> > any
> >    CRTCs that needed disabling, and before we begin
> > enabling/updating
> >    CRTCs
> >  * Because pipe ddb allocations (not the inner-pipe ddb allocations
> >    that apply to each pipe's planes) only change while
> >    enabling/disabling crtcs:
> >     * Pass 1: Find which pipe's new ddb allocation won't overlap
> > with
> >       another pipe's previous allocation, and update that pipe
> > first
> >     * Pass 2: Update the allocation of the remaining pipe
> > 
> > Here's an illustration of what this looks like. Parts of the ddb
> > not
> > being used by any CRTCs are marked out with 'x's:
> > 
> > With pipe A enabled, we enable pipe B:
> > Initial DDB:    |           A           |
> > update_ddbs:    |     A     |xxxxxxxxxxx| Pass 1
> > Enable pipe B:  |     A     |     B     |
> > 
> > With pipes B and A active, we enable pipe C:
> > 
> > Initial DDB:    |     B     |     A     |
> > update_ddbs:    |   B   |xxx|     A     | Pass 1
> > update_ddbs:    |   B   |   A   |xxxxxxx| Pass 2
> > Enable pipe C:  |   B   |   A   |   C   |
> > 
> > With pipes A, B, and C active, we disable B:
> > Initial DDB:    |   A   |   B   |   C   |
> > Disable pipe B: |   A   |xxxxxxx|   C   |
> > update_ddbs:    |     A     |     C     | Pass 1
> > Since neither pipe's new allocation overlapped, we skip pass 2
> > 
> > Another allocation with A, B, and C active, disabling A:
> > Initial DDB:    |   A   |   B   |   C   |
> > Disable pipe A: |xxxxxxx|   B   |   C   |
> > update_ddbs:    |     B     |xxx|   C   | Pass 1
> > update_ddbs:    |     B     |     C     | Pass 2
> > 
> > This should ensure we can always move around the allocations of
> > pipes
> > without them ever overlapping and exploding.
> 
> That's what the current flush thing does, or at least that what it
> used
> to do at least. Not sure it's really doing it anymore, but I'm pretty
> sure the order in which it did things was sound at some point.
> 
> > 
> > 
> > This branch doesn't entirely fix underrun issues, but I'm mostly
> > sure
> > that's the fault of still not having removed the global wm update
> > hook
> > yet (which is leading to additional pipe flushes in places they
> > shouldn't be):
> 
> Well it should basically boil down to s/update_wm/update_ddb/
> Only DDB reallocation really warrants a global hook. Everything else
> should be handled via per-crtc hooks, on all platforms.
> 
> > 
> > 
> > https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
> > 
> > As for updating inner-pipe ddb allocations for each plane on a
> > pipe, we
> > should be able to just do that at the same time we update each
> > pipe's
> > watermarks
> 
> Yes.
> 
> None of that changes what I said before though. Either you need to
> lock down everything when the DDB needs to be repartitioned, or you
> do what I outlined in the previous mail. Otherwise a plane update
> etc. happening in parallel will still blow up on account of the DDB
> state changing underneath the plane update somewhere between compute
> and commit. I can't really think of third option that would work.
> 
Bleh! I didn't even realize plane updates could happen in parallel like
that. Suddenly your proposal makes a lot more sense...

Anyway, your method definitely sounds like the right one. Unless Matt
thinks there's something that could go wrong there, I'm going to start
working that into the driver and repost the patchset once I've added
that into the driver.

Cheers,
	Lyude 
> > 
> > 
> > Let me know what you think
> > 	Lyude
> > 
> > On Fri, 2016-07-29 at 12:39 +0300, Ville Syrjälä wrote:
> > > 
> > > On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> > > > 
> > > > 
> > > > This is completely untested (and probably horribly
> > > > broken/buggy),
> > > > but
> > > > here's a quick mockup of the general approach I was thinking
> > > > for
> > > > ensuring DDB & WM's can be updated together while ensuring the
> > > > three-step pipe flushing process is honored:
> > > > 
> > > >         https://github.com/mattrope/kernel/commits/experimental
> > > > /lyu
> > > > de_ddb
> > > > 
> > > > Basically the idea is to take note of what's happening to the
> > > > pipe's DDB
> > > > allocation (shrinking, growing, unchanged, etc.) during the
> > > > atomic
> > > > check
> > > > phase;
> > > 
> > > Didn't look too closely, but I think you can't actually do that
> > > unless
> > > you lock all the crtcs whenever the number of active pipes is
> > > goind
> > > to
> > > change. Meaning we'd essentially be back to the one-big-modeset-
> > > lock
> > > apporach, which will cause missed flips and whanot on the other
> > > pipes.
> > > 
> > > The alternative I think would consist of:
> > > - make sure level 0 watermark never exceeds
> > > total_ddb_size/max_pipes,
> > >   so that a modeset doesn't have to care about the wms for the
> > > other
> > >   pipes not fitting in
> > > - level 1+ watermarks would be checked against total_ddb_size
> > > - protect the plane/pipe commit with the wm mutex whenever the
> > > wms
> > >   need to be reprogrammed
> > > - keep the flush_wm thing around for the case when ddb size does
> > > get
> > >   changed, protect it with the wm lock
> > > - when programming wms, we will first filter out any level that
> > >   doesn't fit in with the current ddb size, and then program the
> > >   rest in
> > > - potentially introduce per-pipe wm locks if the one big lock
> > > looks
> > >   like an issue, which it might if the flush_wm holds it all the
> > > way
> > >   through
> > > 
> > > > 
> > > > 
> > > > then during the commit phase, we loop over the CRTC's three
> > > > times
> > > > instead of just once, but only operate on a subset of the
> > > > CRTC's in
> > > > each
> > > > loop.  While operating on each CRTC, the plane, WM, and DDB all
> > > > get
> > > > programmed together and have a single flush for all three.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
> > > > > 
> > > > > 
> > > > > Latest version of https://lkml.org/lkml/2016/7/26/290 .
> > > > > Resending
> > > > > the whole
> > > > > thing to keep it in one place.
> > > > > 
> > > > > Lyude (5):
> > > > >   drm/i915/skl: Add support for the SAGV, fix underrun hangs
> > > > >   drm/i915/skl: Only flush pipes when we change the ddb
> > > > > allocation
> > > > >   drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
> > > > >   drm/i915/skl: Update plane watermarks atomically during
> > > > > plane
> > > > > updates
> > > > >   drm/i915/skl: Always wait for pipes to update after a flush
> > > > > 
> > > > > Matt Roper (1):
> > > > >   drm/i915/gen9: Only copy WM results for changed pipes to
> > > > > skl_hw
> > > > > 
> > > > >  drivers/gpu/drm/i915/i915_drv.h      |   3 +
> > > > >  drivers/gpu/drm/i915/i915_reg.h      |   5 +
> > > > >  drivers/gpu/drm/i915/intel_display.c |  24 ++++
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> > > > >  drivers/gpu/drm/i915/intel_pm.c      | 240
> > > > > +++++++++++++++++++++++++++++++----
> > > > >  drivers/gpu/drm/i915/intel_sprite.c  |   2 +
> > > > >  6 files changed, 255 insertions(+), 23 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@...ts.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > -- 
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > IoTG Platform Enabling & Development
> > > > Intel Corporation
> > > > (916) 356-2795
> > > 
> > -- 
> > Cheers,
> > 	Lyude
> 
-- 
Cheers,
	Lyude

Powered by blists - more mailing lists