Re: [Mesa-dev] [PATCH 1/2] eg/compute: Use reference counting to handle compute memory pool.

[Jan Vesely]

On Mon, 2018-05-07 at 15:10 +0200, Nicolai Hähnle wrote:
> On 04.05.2018 08:34, Jan Vesely wrote:
> > The original bug/corruption was by util_unreference_framebuffer_state,
> > trying to drop reference on cbuf[0] (global AS for OCL).
> > Adding reference counting to set_rat uncovered problems with acessing the 
> > pool->bo.
> > 
> > Also drops leaked memory from 7,4kB to 1,7Kb in single run of 
> > conformance_bruteforce trunc -w.
> > 
> > Signed-off-by: Jan Vesely 
> > ---
> >   src/gallium/drivers/r600/compute_memory_pool.c | 18 ++
> >   src/gallium/drivers/r600/evergreen_compute.c   |  3 ++-
> >   2 files changed, 8 insertions(+), 13 deletions(-)
> > 
> > diff --git a/src/gallium/drivers/r600/compute_memory_pool.c 
> > b/src/gallium/drivers/r600/compute_memory_pool.c
> > index bcda155c71..155fefe54f 100644
> > --- a/src/gallium/drivers/r600/compute_memory_pool.c
> > +++ b/src/gallium/drivers/r600/compute_memory_pool.c
> > @@ -91,10 +91,7 @@ void compute_memory_pool_delete(struct 
> > compute_memory_pool* pool)
> >   {
> > COMPUTE_DBG(pool->screen, "* compute_memory_pool_delete()\n");
> > free(pool->shadow);
> > -   if (pool->bo) {
> > -   pool->screen->b.b.resource_destroy((struct pipe_screen *)
> > -   pool->screen, (struct pipe_resource *)pool->bo);
> > -   }
> > +   pipe_resource_reference(>bo, NULL);
> > /* In theory, all of the items were freed in compute_memory_free.
> >  * Just delete the list heads
> >  */
> > @@ -213,11 +210,9 @@ int compute_memory_grow_defrag_pool(struct 
> > compute_memory_pool *pool,
> >   
> > compute_memory_defrag(pool, src, dst, pipe);
> >   
> > -   pool->screen->b.b.resource_destroy(
> > -   (struct pipe_screen *)pool->screen,
> > -   src);
> > -
> > -   pool->bo = temp;
> > +   pipe_resource_reference(>bo, dst);
> > +   /* Drop the extra reference from creation */
> > +   pipe_resource_reference(, NULL);
> 
> I believe you could just say
> 
>   pool->bo = dst;
>   dst = NULL;
> 
> here, and save two atomic ops.

I think the above would leak old pool->bo (if present). I could do:

pipe_resource_reference(>bo, NULL);
pool->bo = temp;

to save one reference op, but I don't think that's critical since we
just copied and defragged all compute buffers. On the other hand it'd
be consistent with the code below if you prefer.

> 
> > pool->size_in_dw = new_size_in_dw;
> > }
> > else {
> > @@ -230,9 +225,8 @@ int compute_memory_grow_defrag_pool(struct 
> > compute_memory_pool *pool,
> > return -1;
> >   
> > pool->size_in_dw = new_size_in_dw;
> > -   pool->screen->b.b.resource_destroy(
> > -   (struct pipe_screen *)pool->screen,
> > -   (struct pipe_resource *)pool->bo);
> > +   pipe_resource_reference(>bo, NULL);
> > +   /* We already have one reference */
> 
> I find this comment confusing.
> 
> The rest looks good to me.

thanks,
Jan

> 
> Cheers,
> Nicolai
> 
> 
> > pool->bo = r600_compute_buffer_alloc_vram(pool->screen, 
> > pool->size_in_dw * 4);
> > compute_memory_shadow(pool, pipe, 0);
> >   
> > diff --git a/src/gallium/drivers/r600/evergreen_compute.c 
> > b/src/gallium/drivers/r600/evergreen_compute.c
> > index 6bec1a333f..58626a3114 100644
> > --- a/src/gallium/drivers/r600/evergreen_compute.c
> > +++ b/src/gallium/drivers/r600/evergreen_compute.c
> > @@ -122,7 +122,8 @@ static void evergreen_set_rat(struct r600_pipe_compute 
> > *pipe,
> > rat_templ.u.tex.first_layer = 0;
> > rat_templ.u.tex.last_layer = 0;
> >   
> > -   /* Add the RAT the list of color buffers */
> > +   /* Add the RAT the list of color buffers. Drop the old buffer first. */
> > +   pipe_surface_reference(>ctx->framebuffer.state.cbufs[id], NULL);
> > pipe->ctx->framebuffer.state.cbufs[id] = pipe->ctx->b.b.create_surface(
> > (struct pipe_context *)pipe->ctx,
> > (struct pipe_resource *)bo, _templ);
> > 
> 
> 


signature.asc
Description: This is a digitally signed message part
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] eg/compute: Use reference counting to handle compute memory pool.

[Nicolai Hähnle]

On 04.05.2018 08:34, Jan Vesely wrote:

The original bug/corruption was by util_unreference_framebuffer_state,
trying to drop reference on cbuf[0] (global AS for OCL).
Adding reference counting to set_rat uncovered problems with acessing the 
pool->bo.

Also drops leaked memory from 7,4kB to 1,7Kb in single run of 
conformance_bruteforce trunc -w.

Signed-off-by: Jan Vesely 
---
  src/gallium/drivers/r600/compute_memory_pool.c | 18 ++
  src/gallium/drivers/r600/evergreen_compute.c   |  3 ++-
  2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/src/gallium/drivers/r600/compute_memory_pool.c 
b/src/gallium/drivers/r600/compute_memory_pool.c
index bcda155c71..155fefe54f 100644
--- a/src/gallium/drivers/r600/compute_memory_pool.c
+++ b/src/gallium/drivers/r600/compute_memory_pool.c
@@ -91,10 +91,7 @@ void compute_memory_pool_delete(struct compute_memory_pool* 
pool)
  {
COMPUTE_DBG(pool->screen, "* compute_memory_pool_delete()\n");
free(pool->shadow);
-   if (pool->bo) {
-   pool->screen->b.b.resource_destroy((struct pipe_screen *)
-   pool->screen, (struct pipe_resource *)pool->bo);
-   }
+   pipe_resource_reference(>bo, NULL);
/* In theory, all of the items were freed in compute_memory_free.
 * Just delete the list heads
 */
@@ -213,11 +210,9 @@ int compute_memory_grow_defrag_pool(struct 
compute_memory_pool *pool,
  
  			compute_memory_defrag(pool, src, dst, pipe);
  
-			pool->screen->b.b.resource_destroy(

-   (struct pipe_screen *)pool->screen,
-   src);
-
-   pool->bo = temp;
+   pipe_resource_reference(>bo, dst);
+   /* Drop the extra reference from creation */
+   pipe_resource_reference(, NULL);


I believe you could just say

pool->bo = dst;
dst = NULL;

here, and save two atomic ops.



pool->size_in_dw = new_size_in_dw;
}
else {
@@ -230,9 +225,8 @@ int compute_memory_grow_defrag_pool(struct 
compute_memory_pool *pool,
return -1;
  
  			pool->size_in_dw = new_size_in_dw;

-   pool->screen->b.b.resource_destroy(
-   (struct pipe_screen *)pool->screen,
-   (struct pipe_resource *)pool->bo);
+   pipe_resource_reference(>bo, NULL);
+   /* We already have one reference */


I find this comment confusing.

The rest looks good to me.

Cheers,
Nicolai



pool->bo = r600_compute_buffer_alloc_vram(pool->screen, 
pool->size_in_dw * 4);
compute_memory_shadow(pool, pipe, 0);
  
diff --git a/src/gallium/drivers/r600/evergreen_compute.c b/src/gallium/drivers/r600/evergreen_compute.c

index 6bec1a333f..58626a3114 100644
--- a/src/gallium/drivers/r600/evergreen_compute.c
+++ b/src/gallium/drivers/r600/evergreen_compute.c
@@ -122,7 +122,8 @@ static void evergreen_set_rat(struct r600_pipe_compute 
*pipe,
rat_templ.u.tex.first_layer = 0;
rat_templ.u.tex.last_layer = 0;
  
-	/* Add the RAT the list of color buffers */

+   /* Add the RAT the list of color buffers. Drop the old buffer first. */
+   pipe_surface_reference(>ctx->framebuffer.state.cbufs[id], NULL);
pipe->ctx->framebuffer.state.cbufs[id] = pipe->ctx->b.b.create_surface(
(struct pipe_context *)pipe->ctx,
(struct pipe_resource *)bo, _templ);




--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] eg/compute: Use reference counting to handle compute memory pool.

[Jan Vesely]

The original bug/corruption was by util_unreference_framebuffer_state,
trying to drop reference on cbuf[0] (global AS for OCL).
Adding reference counting to set_rat uncovered problems with acessing the 
pool->bo.

Also drops leaked memory from 7,4kB to 1,7Kb in single run of 
conformance_bruteforce trunc -w.

Signed-off-by: Jan Vesely 
---
 src/gallium/drivers/r600/compute_memory_pool.c | 18 ++
 src/gallium/drivers/r600/evergreen_compute.c   |  3 ++-
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/src/gallium/drivers/r600/compute_memory_pool.c 
b/src/gallium/drivers/r600/compute_memory_pool.c
index bcda155c71..155fefe54f 100644
--- a/src/gallium/drivers/r600/compute_memory_pool.c
+++ b/src/gallium/drivers/r600/compute_memory_pool.c
@@ -91,10 +91,7 @@ void compute_memory_pool_delete(struct compute_memory_pool* 
pool)
 {
COMPUTE_DBG(pool->screen, "* compute_memory_pool_delete()\n");
free(pool->shadow);
-   if (pool->bo) {
-   pool->screen->b.b.resource_destroy((struct pipe_screen *)
-   pool->screen, (struct pipe_resource *)pool->bo);
-   }
+   pipe_resource_reference(>bo, NULL);
/* In theory, all of the items were freed in compute_memory_free.
 * Just delete the list heads
 */
@@ -213,11 +210,9 @@ int compute_memory_grow_defrag_pool(struct 
compute_memory_pool *pool,
 
compute_memory_defrag(pool, src, dst, pipe);
 
-   pool->screen->b.b.resource_destroy(
-   (struct pipe_screen *)pool->screen,
-   src);
-
-   pool->bo = temp;
+   pipe_resource_reference(>bo, dst);
+   /* Drop the extra reference from creation */
+   pipe_resource_reference(, NULL);
pool->size_in_dw = new_size_in_dw;
}
else {
@@ -230,9 +225,8 @@ int compute_memory_grow_defrag_pool(struct 
compute_memory_pool *pool,
return -1;
 
pool->size_in_dw = new_size_in_dw;
-   pool->screen->b.b.resource_destroy(
-   (struct pipe_screen *)pool->screen,
-   (struct pipe_resource *)pool->bo);
+   pipe_resource_reference(>bo, NULL);
+   /* We already have one reference */
pool->bo = r600_compute_buffer_alloc_vram(pool->screen, 
pool->size_in_dw * 4);
compute_memory_shadow(pool, pipe, 0);
 
diff --git a/src/gallium/drivers/r600/evergreen_compute.c 
b/src/gallium/drivers/r600/evergreen_compute.c
index 6bec1a333f..58626a3114 100644
--- a/src/gallium/drivers/r600/evergreen_compute.c
+++ b/src/gallium/drivers/r600/evergreen_compute.c
@@ -122,7 +122,8 @@ static void evergreen_set_rat(struct r600_pipe_compute 
*pipe,
rat_templ.u.tex.first_layer = 0;
rat_templ.u.tex.last_layer = 0;
 
-   /* Add the RAT the list of color buffers */
+   /* Add the RAT the list of color buffers. Drop the old buffer first. */
+   pipe_surface_reference(>ctx->framebuffer.state.cbufs[id], NULL);
pipe->ctx->framebuffer.state.cbufs[id] = pipe->ctx->b.b.create_surface(
(struct pipe_context *)pipe->ctx,
(struct pipe_resource *)bo, _templ);
-- 
2.17.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev