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

[Maarten Maathuis]

On Mon, Aug 20, 2012 at 11:41 PM, Marcin Slusarz
<marcin.slusarz at gmail.com> wrote:
> 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
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

One thing, i think FIRE_RING should check for available ib slots,
currently it doesn't and that seems a bit dangerous. Just a thought
for Ben :-)

-- 
Far away from the primal instinct, the song seems to fade away, the
river get wider between your thoughts and the things we do and say.