The big issue I have with this patch is that it's a little dirty to have the bos thing like that. I had a long comment about a way to mitigate that, but in the end, it may not be worth it... this scratch system is complex and it's insufficiently necessary to muck with it. Not a huge fan of creating a new fence either...
If you *are* interested in trying to fix it all up, moving all the scratch stuff off to the side and then attaching a fence to it (updated in the various kick methods) and then freeing when that fence is hit -- that seems like it'd be a bit cleaner. But not enough so that I'd ask or even recommend doing it. On Sun, Mar 29, 2015 at 12:24 PM, Marcin Ślusarz <[email protected]> wrote: > When nvc0_push_vbo calls nouveau_scratch_done it does not mean > scratch buffers can be freed immediately. It means "when hardware > advances to this place in the command stream the scratch buffers > can be freed". > > The bug existed for a very long time. Nobody noticed, because > "scratch runout" code path is rarely executed. > > Fixes "Serious Sam 3" on nve7/gk107. > --- > src/gallium/drivers/nouveau/nouveau_buffer.c | 33 > +++++++++++++++++++++++----- > 1 file changed, 28 insertions(+), 5 deletions(-) > > diff --git a/src/gallium/drivers/nouveau/nouveau_buffer.c > b/src/gallium/drivers/nouveau/nouveau_buffer.c > index 49ff100..e649752 100644 > --- a/src/gallium/drivers/nouveau/nouveau_buffer.c > +++ b/src/gallium/drivers/nouveau/nouveau_buffer.c > @@ -846,19 +846,42 @@ nouveau_scratch_bo_alloc(struct nouveau_context *nv, > struct nouveau_bo **pbo, > 4096, size, NULL, pbo); > } > > +struct bos > +{ > + unsigned nr; > + struct nouveau_bo **arr; > +}; > + > +static void > +unref_bos(void *d) > +{ > + struct bos *b = d; > + int i; > + > + for (i = 0; i < b->nr; ++i) > + nouveau_bo_ref(NULL, &b->arr[i]); > + > + FREE(b->arr); > + FREE(b); > +} > + > void > nouveau_scratch_runout_release(struct nouveau_context *nv) > { > if (!nv->scratch.nr_runout) > return; > - do { > - --nv->scratch.nr_runout; > - nouveau_bo_ref(NULL, &nv->scratch.runout[nv->scratch.nr_runout]); > - } while (nv->scratch.nr_runout); > > - FREE(nv->scratch.runout); > + struct bos *b = MALLOC(sizeof(*b)); Let's not crash if this fails... I'd be totally fine with just leaking the bo's, or alternatively stalling until the fence is hit. > + b->arr = nv->scratch.runout; > + b->nr = nv->scratch.nr_runout; > + > + struct nouveau_fence *fence = NULL; Unnecessary (the = NULL bit). > + nouveau_fence_new(nv->screen, &fence, TRUE); > + nouveau_fence_work(fence, unref_bos, b); > + > nv->scratch.end = 0; > nv->scratch.runout = NULL; > + nv->scratch.nr_runout = 0; > } > > /* Allocate an extra bo if we can't fit everything we need simultaneously. > -- > 2.1.0 > > _______________________________________________ > Nouveau mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/nouveau _______________________________________________ Nouveau mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/nouveau
