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

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));
+   b->arr = nv->scratch.runout;
+   b->nr  = nv->scratch.nr_runout;
+
+   struct nouveau_fence *fence = NULL;
+   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

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