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 <[email protected]> > > --- > > 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(&pool->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(&pool->bo, dst); > > + /* Drop the extra reference from creation */ > > + pipe_resource_reference(&dst, 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(&pool->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(&pool->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(&pipe->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, &rat_templ);
> >
>
>
signature.asc
Description: This is a digitally signed message part
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
