Re: [PATCH 2/2] drm/i915: repin bound framebuffers on resume

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: intel-gfx@lists.freedesktop.org, Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH 2/2] drm/i915: repin bound framebuffers on resume
Date: Wed, 12 Jun 2013 23:41:08 +0100	[thread overview]
Message-ID: <20130612224108.GH32648@cantiga.alporthouse.com> (raw)
In-Reply-To: <20130612150651.428903ce@jbarnes-desktop>

On Wed, Jun 12, 2013 at 03:06:51PM -0700, Jesse Barnes wrote:
> On Wed, 12 Jun 2013 00:48:25 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Tue, Jun 11, 2013 at 04:01:21PM -0700, Stéphane Marchesin wrote:
> > > 
> > > On Tue, Jun 11, 2013 at 3:57 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > On Tue, Jun 11, 2013 at 03:49:27PM -0700, Stéphane Marchesin wrote:
> > > >> During suspend all fences are reset, including their pin_count which
> > > >> is reset to 0. However a framebuffer can be bound across
> > > >> suspend/resume, which means that when the buffer is unbound after
> > > >> resume, the pin count for the buffer will be negative. Since the
> > > >> fence pin count is now negative when available and zero when in use,
> > > >> the buffer's fence will get recycled when the fence is in use which
> > > >> is the opposite of what we want. The visible effect is that since the
> > > >> fence is recycled the tiling mode goes away while the buffer is being
> > > >> displayed and we get lines/screens of garbage.
> > > >>
> > > >> To fix this, we repin the fences for all bound fbs on resume, which
> > > >> ensures the pin count is right.
> > > >
> > > > Yikes. So why do we not just keep the fences alive during suspend (not
> > > > touching their pin_count), and then just iterate over the list of fences
> > > > rewriting the register as required upon resume? That would seem less
> > > > error prone than trying to reconstruct the lost pin_count.
> > > 
> > > I suspect they'd need to be saved/restored at the hw level as well,
> > > which AFAICS isn't happening today...
> > 
> > Ugh, I introduced this bug 30 months ago - saved by the VT switch on
> > resume. But we can restore the fences from dev_priv->fence_regs...
> > Actually we have a very similar problem after a GPU reset where we
> > should restore fences for pinned objects (i.e. the scanout). The patch
> > to fix both looks fairly straightforward.
> 
> To be clear, this only affects gen3 right?  For gen4+ we don't need the
> fences for scanout since we have a bit in the plane control...

True, you will only get scanout corruption from lack of a fence on gen2/3.
FBC will also be more broken than before.

> Or are we failing to fault on a previously mapped scanout too?  If so,
> we'd need to cover more than just scanout here.

They all get faulted in again, and all will grab a fence again. Only
scanouts pinned at the time of resume will believe that they hold an
additional reference to the fence, and so unpin it once too often. If we
do that enough times, we will starve ourselves of fences. And a gen3
scanout runs the risk of losing its fence at any time.

So the impact is far less severe than it appears at first glance. Unless
I've missed something.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre