[Nouveau] [PATCH] nouveau: synchronize "scratch runout" destruction with the command stream

[Ilia Mirkin]

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
<marcin.slusarz at gmail.com> 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
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau