etnaviv: Consolidate buffer references from framebuffers

Submitted by Tomeu Vizoso on Dec. 17, 2018, 8:56 a.m.

Details

Message ID 20181217085600.13974-1-tomeu.vizoso@collabora.com
State New
Headers show
Series "etnaviv: Consolidate buffer references from framebuffers" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Tomeu Vizoso Dec. 17, 2018, 8:56 a.m.
We were leaking surfaces because the references taken in
etna_set_framebuffer_state weren't being released on context destroy.

Instead of just directly releasing those references in
etna_context_destroy, use the util_copy_framebuffer_state helper.

Take the chance to remove the duplicated buffer references in
compiled_framebuffer_state to avoid confusion.

The leak can be reproduced with a client that continuously creates and
destroys contexts.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reported-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
 src/gallium/drivers/etnaviv/etnaviv_context.c  | 10 ++++++----
 src/gallium/drivers/etnaviv/etnaviv_internal.h |  1 -
 src/gallium/drivers/etnaviv/etnaviv_state.c    |  8 +++-----
 3 files changed, 9 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/etnaviv/etnaviv_context.c b/src/gallium/drivers/etnaviv/etnaviv_context.c
index 96cf2f3b59a8..2ecbc7449094 100644
--- a/src/gallium/drivers/etnaviv/etnaviv_context.c
+++ b/src/gallium/drivers/etnaviv/etnaviv_context.c
@@ -60,6 +60,8 @@  etna_context_destroy(struct pipe_context *pctx)
 {
    struct etna_context *ctx = etna_context(pctx);
 
+   util_copy_framebuffer_state(&ctx->framebuffer_s, NULL);
+
    if (ctx->primconvert)
       util_primconvert_destroy(ctx->primconvert);
 
@@ -299,10 +301,10 @@  etna_draw_vbo(struct pipe_context *pctx, const struct pipe_draw_info *info)
    if (DBG_ENABLED(ETNA_DBG_FLUSH_ALL))
       pctx->flush(pctx, NULL, 0);
 
-   if (ctx->framebuffer.cbuf)
-      etna_resource(ctx->framebuffer.cbuf->texture)->seqno++;
-   if (ctx->framebuffer.zsbuf)
-      etna_resource(ctx->framebuffer.zsbuf->texture)->seqno++;
+   if (ctx->framebuffer_s.cbufs[0])
+      etna_resource(ctx->framebuffer_s.cbufs[0]->texture)->seqno++;
+   if (ctx->framebuffer_s.zsbuf)
+      etna_resource(ctx->framebuffer_s.zsbuf->texture)->seqno++;
    if (info->index_size && indexbuf != info->index.resource)
       pipe_resource_reference(&indexbuf, NULL);
 }
diff --git a/src/gallium/drivers/etnaviv/etnaviv_internal.h b/src/gallium/drivers/etnaviv/etnaviv_internal.h
index 3424d8a77153..77214d9ccba1 100644
--- a/src/gallium/drivers/etnaviv/etnaviv_internal.h
+++ b/src/gallium/drivers/etnaviv/etnaviv_internal.h
@@ -182,7 +182,6 @@  struct compiled_viewport_state {
 
 /* Compiled pipe_framebuffer_state */
 struct compiled_framebuffer_state {
-   struct pipe_surface *cbuf, *zsbuf; /* keep reference to surfaces */
    uint32_t GL_MULTI_SAMPLE_CONFIG;
    uint32_t PE_COLOR_FORMAT;
    uint32_t PE_DEPTH_CONFIG;
diff --git a/src/gallium/drivers/etnaviv/etnaviv_state.c b/src/gallium/drivers/etnaviv/etnaviv_state.c
index 87ba10b0dc98..520cc5a775fc 100644
--- a/src/gallium/drivers/etnaviv/etnaviv_state.c
+++ b/src/gallium/drivers/etnaviv/etnaviv_state.c
@@ -37,6 +37,7 @@ 
 #include "etnaviv_surface.h"
 #include "etnaviv_translate.h"
 #include "etnaviv_util.h"
+#include "util/u_framebuffer.h"
 #include "util/u_helpers.h"
 #include "util/u_inlines.h"
 #include "util/u_math.h"
@@ -130,7 +131,6 @@  etna_set_framebuffer_state(struct pipe_context *pctx,
       assert(res->layout & ETNA_LAYOUT_BIT_TILE); /* Cannot render to linear surfaces */
       etna_update_render_resource(pctx, cbuf->base.texture);
 
-      pipe_surface_reference(&cs->cbuf, &cbuf->base);
       cs->PE_COLOR_FORMAT =
          VIVS_PE_COLOR_FORMAT_FORMAT(translate_rs_format(cbuf->base.format)) |
          VIVS_PE_COLOR_FORMAT_COMPONENTS__MASK |
@@ -182,7 +182,6 @@  etna_set_framebuffer_state(struct pipe_context *pctx,
 
       nr_samples_color = cbuf->base.texture->nr_samples;
    } else {
-      pipe_surface_reference(&cs->cbuf, NULL);
       /* Clearing VIVS_PE_COLOR_FORMAT_COMPONENTS__MASK and
        * VIVS_PE_COLOR_FORMAT_OVERWRITE prevents us from overwriting the
        * color target */
@@ -201,7 +200,6 @@  etna_set_framebuffer_state(struct pipe_context *pctx,
 
       etna_update_render_resource(pctx, zsbuf->base.texture);
 
-      pipe_surface_reference(&cs->zsbuf, &zsbuf->base);
       assert(res->layout &ETNA_LAYOUT_BIT_TILE); /* Cannot render to linear surfaces */
 
       uint32_t depth_format = translate_depth_format(zsbuf->base.format);
@@ -252,7 +250,6 @@  etna_set_framebuffer_state(struct pipe_context *pctx,
 
       nr_samples_depth = zsbuf->base.texture->nr_samples;
    } else {
-      pipe_surface_reference(&cs->zsbuf, NULL);
       cs->PE_DEPTH_CONFIG = VIVS_PE_DEPTH_CONFIG_DEPTH_MODE_NONE;
       cs->PE_DEPTH_ADDR.bo = NULL;
       cs->PE_DEPTH_STRIDE = 0;
@@ -325,7 +322,8 @@  etna_set_framebuffer_state(struct pipe_context *pctx,
     */
    cs->PE_LOGIC_OP = VIVS_PE_LOGIC_OP_SINGLE_BUFFER(ctx->specs.single_buffer ? 3 : 0);
 
-   ctx->framebuffer_s = *sv; /* keep copy of original structure */
+   /* keep copy of original structure */
+   util_copy_framebuffer_state(&ctx->framebuffer_s, sv);
    ctx->dirty |= ETNA_DIRTY_FRAMEBUFFER | ETNA_DIRTY_DERIVE_TS;
 }
 

Comments

Am Mo., 17. Dez. 2018 um 09:56 Uhr schrieb Tomeu Vizoso
<tomeu.vizoso@collabora.com>:
>
> We were leaking surfaces because the references taken in
> etna_set_framebuffer_state weren't being released on context destroy.
>
> Instead of just directly releasing those references in
> etna_context_destroy, use the util_copy_framebuffer_state helper.
>
> Take the chance to remove the duplicated buffer references in
> compiled_framebuffer_state to avoid confusion.
>
> The leak can be reproduced with a client that continuously creates and
> destroys contexts.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Reported-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>

> ---
>  src/gallium/drivers/etnaviv/etnaviv_context.c  | 10 ++++++----
>  src/gallium/drivers/etnaviv/etnaviv_internal.h |  1 -
>  src/gallium/drivers/etnaviv/etnaviv_state.c    |  8 +++-----
>  3 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_context.c b/src/gallium/drivers/etnaviv/etnaviv_context.c
> index 96cf2f3b59a8..2ecbc7449094 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_context.c
> +++ b/src/gallium/drivers/etnaviv/etnaviv_context.c
> @@ -60,6 +60,8 @@ etna_context_destroy(struct pipe_context *pctx)
>  {
>     struct etna_context *ctx = etna_context(pctx);
>
> +   util_copy_framebuffer_state(&ctx->framebuffer_s, NULL);
> +
>     if (ctx->primconvert)
>        util_primconvert_destroy(ctx->primconvert);
>
> @@ -299,10 +301,10 @@ etna_draw_vbo(struct pipe_context *pctx, const struct pipe_draw_info *info)
>     if (DBG_ENABLED(ETNA_DBG_FLUSH_ALL))
>        pctx->flush(pctx, NULL, 0);
>
> -   if (ctx->framebuffer.cbuf)
> -      etna_resource(ctx->framebuffer.cbuf->texture)->seqno++;
> -   if (ctx->framebuffer.zsbuf)
> -      etna_resource(ctx->framebuffer.zsbuf->texture)->seqno++;
> +   if (ctx->framebuffer_s.cbufs[0])
> +      etna_resource(ctx->framebuffer_s.cbufs[0]->texture)->seqno++;
> +   if (ctx->framebuffer_s.zsbuf)
> +      etna_resource(ctx->framebuffer_s.zsbuf->texture)->seqno++;
>     if (info->index_size && indexbuf != info->index.resource)
>        pipe_resource_reference(&indexbuf, NULL);
>  }
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_internal.h b/src/gallium/drivers/etnaviv/etnaviv_internal.h
> index 3424d8a77153..77214d9ccba1 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_internal.h
> +++ b/src/gallium/drivers/etnaviv/etnaviv_internal.h
> @@ -182,7 +182,6 @@ struct compiled_viewport_state {
>
>  /* Compiled pipe_framebuffer_state */
>  struct compiled_framebuffer_state {
> -   struct pipe_surface *cbuf, *zsbuf; /* keep reference to surfaces */
>     uint32_t GL_MULTI_SAMPLE_CONFIG;
>     uint32_t PE_COLOR_FORMAT;
>     uint32_t PE_DEPTH_CONFIG;
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_state.c b/src/gallium/drivers/etnaviv/etnaviv_state.c
> index 87ba10b0dc98..520cc5a775fc 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_state.c
> +++ b/src/gallium/drivers/etnaviv/etnaviv_state.c
> @@ -37,6 +37,7 @@
>  #include "etnaviv_surface.h"
>  #include "etnaviv_translate.h"
>  #include "etnaviv_util.h"
> +#include "util/u_framebuffer.h"
>  #include "util/u_helpers.h"
>  #include "util/u_inlines.h"
>  #include "util/u_math.h"
> @@ -130,7 +131,6 @@ etna_set_framebuffer_state(struct pipe_context *pctx,
>        assert(res->layout & ETNA_LAYOUT_BIT_TILE); /* Cannot render to linear surfaces */
>        etna_update_render_resource(pctx, cbuf->base.texture);
>
> -      pipe_surface_reference(&cs->cbuf, &cbuf->base);
>        cs->PE_COLOR_FORMAT =
>           VIVS_PE_COLOR_FORMAT_FORMAT(translate_rs_format(cbuf->base.format)) |
>           VIVS_PE_COLOR_FORMAT_COMPONENTS__MASK |
> @@ -182,7 +182,6 @@ etna_set_framebuffer_state(struct pipe_context *pctx,
>
>        nr_samples_color = cbuf->base.texture->nr_samples;
>     } else {
> -      pipe_surface_reference(&cs->cbuf, NULL);
>        /* Clearing VIVS_PE_COLOR_FORMAT_COMPONENTS__MASK and
>         * VIVS_PE_COLOR_FORMAT_OVERWRITE prevents us from overwriting the
>         * color target */
> @@ -201,7 +200,6 @@ etna_set_framebuffer_state(struct pipe_context *pctx,
>
>        etna_update_render_resource(pctx, zsbuf->base.texture);
>
> -      pipe_surface_reference(&cs->zsbuf, &zsbuf->base);
>        assert(res->layout &ETNA_LAYOUT_BIT_TILE); /* Cannot render to linear surfaces */
>
>        uint32_t depth_format = translate_depth_format(zsbuf->base.format);
> @@ -252,7 +250,6 @@ etna_set_framebuffer_state(struct pipe_context *pctx,
>
>        nr_samples_depth = zsbuf->base.texture->nr_samples;
>     } else {
> -      pipe_surface_reference(&cs->zsbuf, NULL);
>        cs->PE_DEPTH_CONFIG = VIVS_PE_DEPTH_CONFIG_DEPTH_MODE_NONE;
>        cs->PE_DEPTH_ADDR.bo = NULL;
>        cs->PE_DEPTH_STRIDE = 0;
> @@ -325,7 +322,8 @@ etna_set_framebuffer_state(struct pipe_context *pctx,
>      */
>     cs->PE_LOGIC_OP = VIVS_PE_LOGIC_OP_SINGLE_BUFFER(ctx->specs.single_buffer ? 3 : 0);
>
> -   ctx->framebuffer_s = *sv; /* keep copy of original structure */
> +   /* keep copy of original structure */
> +   util_copy_framebuffer_state(&ctx->framebuffer_s, sv);
>     ctx->dirty |= ETNA_DIRTY_FRAMEBUFFER | ETNA_DIRTY_DERIVE_TS;
>  }
>
> --
> 2.20.0
>
> _______________________________________________
> etnaviv mailing list
> etnaviv@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/etnaviv