Marcin Slusarz
2012-Aug-19 21:02 UTC
[Nouveau] [PATCH 10/10] drm/nouveau: fix off-by-one bugs related to command submission in IB mode
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 at gmail.com>
---
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
Maarten Maathuis
2012-Aug-19 21:11 UTC
[Nouveau] [PATCH 10/10] drm/nouveau: fix off-by-one bugs related to command submission in IB mode
On Sun, Aug 19, 2012 at 11:02 PM, Marcin Slusarz <marcin.slusarz at gmail.com> 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 at gmail.com> > --- > 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 at gmail.com> 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.
Ben Skeggs
2012-Aug-20 06:33 UTC
[Nouveau] [PATCH 10/10] drm/nouveau: fix off-by-one bugs related to command submission in IB mode
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 impactAlso 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 at gmail.com> > --- > 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 at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/nouveau
Apparently Analagous Threads
- [PATCH drm-misc-next v2] drm/nouveau: uapi: don't pass NO_PREFETCH flag implicitly
- [PATCH drm-misc-next] drm/nouveau: uapi: don't pass NO_PREFETCH flag implicitly
- [PATCH drm-misc-next] drm/nouveau: uapi: don't pass NO_PREFETCH flag implicitly
- [PATCH 0/2] drm/nouveau: Use more standard logging styles
- [PATCH 0/6] drm/nouveau: Support sync FDs and sync objects