Re: [PATCH 10/10] drm/nouveau: fix off-by-one bugs related to command submission in IB mode

[Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Tue, Aug 21, 2012 at 07:13:23AM +1000, Ben Skeggs wrote:
>    Am Dienstag, 21. August 2012 schrieb Marcin Slusarz :
> 
>      On Mon, Aug 20, 2012 at 04:33:24PM +1000, Ben Skeggs wrote:
>      > On Sun, Aug 19, 2012 at 11:02:00PM +0200, Marcin Slusarz wrote:
>      > > Commit "drm/nouveau: port all engines to new engine module
>      format" changed
>      > > IB size calculation to be less wasteful, but didn't take into
>      account already
>      > > existing off-by-one bugs :).
>      > >
>      > > So:
>      > > - ib_max is the last entry, so we need to +1 when calculating
>      number of
>      > >   free entries
>      > > - nv50_dma_wait already does +1 (for FIRE_RING), so we don't
>      need another +1
>      > >   on nouveau_gem_ioctl_pushbuf side
>      > > - there are 512 allocated IB entries (and it needs to be round
>      number), so we
>      > >   can accept at most 511 entries from userspace (we need one for
>      FIRE_RING) -
>      > >   fortunately userspace already flushes at 511, so nr_push check
>      change won't
>      > >   have any impact
>      > Also skipped this patch for now as I have work in progress that
>      will improve this and
>      > related code.  I should have it in the tree soon.
>      Maybe I should have written this in the changelog, but this patch
>      fixes total
>      mayhem seen in warsow and nexuiz (at least). It's better to
>      integrate it sooner
>      than later.
> 
>    I presume bumping the IB size to 8KiB also helps? 

Yes, my first patch did exactly this and it worked.

However, Maarten can still reproduce this bug, so my patch is not completely
correct. I discovered that RING_SPACE calls nouveau_dma_wait with slots=1 and
nv50_dma_wait increases it again, so we need to move +1 from nv50_dma_wait
to nouveau_gem_ioctl_pushbuf. But once I do this everything explodes, because
we do 2 FIRE_RING's (one from fence_emit, and one from nv50_dma_wait), so...

>    I prefer this in the
>    meantime, and it's probably best to do anyway, I didn't take into
>    account what we allow userspace to do in one shot.  If this works too,
>    i'll push that change this morning.

... it's probably a good idea to do this and forget about the problem.

>    I did try and reproduce on a couple of cards with warsow and wasn't
>    able to.

Nexuiz is even better. It draws artifacts immediately after demo start.

>    Also, what happened to instmem being the culprit here?

It was bad bisection. Once I knew what is the problem, I verified
"drm/nouveau: port all engines to new engine module format" is the first
bad commit.

Marcin