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

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

[Marcin Slusarz]

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

Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/gpu/drm/nouveau/nouveau_chan.c |    2 +-
 drivers/gpu/drm/nouveau/nouveau_dma.c  |    2 +-
 drivers/gpu/drm/nouveau/nouveau_gem.c  |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c
index fd15ebf..d0c0271 100644
--- a/drivers/gpu/drm/nouveau/nouveau_chan.c
+++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
@@ -318,7 +318,7 @@ nouveau_channel_init(struct nouveau_channel *chan, u32 vram, u32 gart)
 		chan->dma.ib_base =  0x10000 / 4;
 		chan->dma.ib_max  = (0x01000 / 8) - 1;
 		chan->dma.ib_put  = 0;
-		chan->dma.ib_free = chan->dma.ib_max - chan->dma.ib_put;
+		chan->dma.ib_free = chan->dma.ib_max - chan->dma.ib_put + 1;
 		chan->dma.max = chan->dma.ib_base;
 		break;
 	}
diff --git a/drivers/gpu/drm/nouveau/nouveau_dma.c b/drivers/gpu/drm/nouveau/nouveau_dma.c
index 40f91e1..e814fab 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dma.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dma.c
@@ -128,7 +128,7 @@ nv50_dma_push_wait(struct nouveau_channel *chan, int count)
 
 		chan->dma.ib_free = get - chan->dma.ib_put;
 		if (chan->dma.ib_free <= 0)
-			chan->dma.ib_free += chan->dma.ib_max;
+			chan->dma.ib_free += chan->dma.ib_max + 1;
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 6454370..d19c7aa 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -662,7 +662,7 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data,
 	if (unlikely(req->nr_push == 0))
 		goto out_next;
 
-	if (unlikely(req->nr_push > NOUVEAU_GEM_MAX_PUSH)) {
+	if (unlikely(req->nr_push >= NOUVEAU_GEM_MAX_PUSH)) {
 		NV_ERROR(drm, "pushbuf push count exceeds limit: %d max %d\n",
 			 req->nr_push, NOUVEAU_GEM_MAX_PUSH);
 		return nouveau_abi16_put(abi16, -EINVAL);
@@ -718,7 +718,7 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data,
 	}
 
 	if (chan->dma.ib_max) {
-		ret = nouveau_dma_wait(chan, req->nr_push + 1, 16);
+		ret = nouveau_dma_wait(chan, req->nr_push, 16);
 		if (ret) {
 			NV_ERROR(drm, "nv50cal_space: %d\n", ret);
 			goto out;
-- 
1.7.8.6

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

[Maarten Maathuis]

On Sun, Aug 19, 2012 at 11:02 PM, Marcin Slusarz
<marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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
>
> Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/gpu/drm/nouveau/nouveau_chan.c |    2 +-
>  drivers/gpu/drm/nouveau/nouveau_dma.c  |    2 +-
>  drivers/gpu/drm/nouveau/nouveau_gem.c  |    4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c
> index fd15ebf..d0c0271 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_chan.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
> @@ -318,7 +318,7 @@ nouveau_channel_init(struct nouveau_channel *chan, u32 vram, u32 gart)
>                 chan->dma.ib_base =  0x10000 / 4;
>                 chan->dma.ib_max  = (0x01000 / 8) - 1;
>                 chan->dma.ib_put  = 0;
> -               chan->dma.ib_free = chan->dma.ib_max - chan->dma.ib_put;
> +               chan->dma.ib_free = chan->dma.ib_max - chan->dma.ib_put + 1;
>                 chan->dma.max = chan->dma.ib_base;
>                 break;
>         }
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dma.c b/drivers/gpu/drm/nouveau/nouveau_dma.c
> index 40f91e1..e814fab 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dma.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dma.c
> @@ -128,7 +128,7 @@ nv50_dma_push_wait(struct nouveau_channel *chan, int count)
>
>                 chan->dma.ib_free = get - chan->dma.ib_put;
>                 if (chan->dma.ib_free <= 0)
> -                       chan->dma.ib_free += chan->dma.ib_max;
> +                       chan->dma.ib_free += chan->dma.ib_max + 1;
>         }
>
>         return 0;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 6454370..d19c7aa 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -662,7 +662,7 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data,
>         if (unlikely(req->nr_push == 0))
>                 goto out_next;
>
> -       if (unlikely(req->nr_push > NOUVEAU_GEM_MAX_PUSH)) {
> +       if (unlikely(req->nr_push >= NOUVEAU_GEM_MAX_PUSH)) {
>                 NV_ERROR(drm, "pushbuf push count exceeds limit: %d max %d\n",
>                          req->nr_push, NOUVEAU_GEM_MAX_PUSH);
>                 return nouveau_abi16_put(abi16, -EINVAL);
> @@ -718,7 +718,7 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data,
>         }
>
>         if (chan->dma.ib_max) {
> -               ret = nouveau_dma_wait(chan, req->nr_push + 1, 16);
> +               ret = nouveau_dma_wait(chan, req->nr_push, 16);
>                 if (ret) {
>                         NV_ERROR(drm, "nv50cal_space: %d\n", ret);
>                         goto out;
> --
> 1.7.8.6
>

<Tested-by: Maarten Maathuis <madman2003-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

I would however like to voice a concern about the reinterpretation of
MAX_PUSH. Strictly speaking the number needs to go down one as far as
i'm concerned. But i'll Ben decide about the API/ABI concerns :-)

-- 
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.

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

[Ben Skeggs]

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.

> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/gpu/drm/nouveau/nouveau_chan.c |    2 +-
>  drivers/gpu/drm/nouveau/nouveau_dma.c  |    2 +-
>  drivers/gpu/drm/nouveau/nouveau_gem.c  |    4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c
> index fd15ebf..d0c0271 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_chan.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
> @@ -318,7 +318,7 @@ nouveau_channel_init(struct nouveau_channel *chan, u32 vram, u32 gart)
>  		chan->dma.ib_base =  0x10000 / 4;
>  		chan->dma.ib_max  = (0x01000 / 8) - 1;
>  		chan->dma.ib_put  = 0;
> -		chan->dma.ib_free = chan->dma.ib_max - chan->dma.ib_put;
> +		chan->dma.ib_free = chan->dma.ib_max - chan->dma.ib_put + 1;
>  		chan->dma.max = chan->dma.ib_base;
>  		break;
>  	}
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dma.c b/drivers/gpu/drm/nouveau/nouveau_dma.c
> index 40f91e1..e814fab 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dma.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dma.c
> @@ -128,7 +128,7 @@ nv50_dma_push_wait(struct nouveau_channel *chan, int count)
>  
>  		chan->dma.ib_free = get - chan->dma.ib_put;
>  		if (chan->dma.ib_free <= 0)
> -			chan->dma.ib_free += chan->dma.ib_max;
> +			chan->dma.ib_free += chan->dma.ib_max + 1;
>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 6454370..d19c7aa 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -662,7 +662,7 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data,
>  	if (unlikely(req->nr_push == 0))
>  		goto out_next;
>  
> -	if (unlikely(req->nr_push > NOUVEAU_GEM_MAX_PUSH)) {
> +	if (unlikely(req->nr_push >= NOUVEAU_GEM_MAX_PUSH)) {
>  		NV_ERROR(drm, "pushbuf push count exceeds limit: %d max %d\n",
>  			 req->nr_push, NOUVEAU_GEM_MAX_PUSH);
>  		return nouveau_abi16_put(abi16, -EINVAL);
> @@ -718,7 +718,7 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data,
>  	}
>  
>  	if (chan->dma.ib_max) {
> -		ret = nouveau_dma_wait(chan, req->nr_push + 1, 16);
> +		ret = nouveau_dma_wait(chan, req->nr_push, 16);
>  		if (ret) {
>  			NV_ERROR(drm, "nv50cal_space: %d\n", ret);
>  			goto out;
> -- 
> 1.7.8.6
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

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

[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.

Marcin

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

[Ben Skeggs]

[-- Attachment #1.1: Type: text/plain, Size: 1630 bytes --]

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? 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.

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

Also, what happened to instmem being the culprit here?

Marcin
>

[-- Attachment #1.2: Type: text/html, Size: 2126 bytes --]

[-- Attachment #2: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Marcin Slusarz]

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

Re: [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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.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.