[Mesa-dev] [PATCH 1/2] llvmpipe: fix "leaking" textures

[Jose Fonseca]

Good catch.

One cosmetic request.

On 15/01/16 02:30, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
>
> This was not really a leak per se, but we were referencing the textures for
> longer than intended. If textures were set via llvmpipe_set_sampler_views()
> (for fs) and then picked up by lp_setup_set_fragment_sampler_views(), they
> were referenced in the setup state. However, the only way to unreference them
> was by replacing them with another texture, and not when the texture slot
> was replaced with a NULL sampler view. (They were then further also referenced
> by the scene too which might have additional minor side effects as we limit
> the memory size which is allowed to be referenced by a scene in a rather crude
> way.) Only setup destruction (at context destruction time) then finally would
> get rid of the references.
> Fix this by noting the number of textures the last time, and unreference
> things if the new view is NULL (avoiding having to unreference things
> always up to PIPE_MAX_SHADER_SAMPLER_VIEWS which would also have worked).
> Found by code inspection, no test...
> ---
>   src/gallium/drivers/llvmpipe/lp_setup.c         | 10 ++++++++--
>   src/gallium/drivers/llvmpipe/lp_setup_context.h |  1 +
>   2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c b/src/gallium/drivers/llvmpipe/lp_setup.c
> index bd85051..b8c4d52 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
> @@ -796,13 +796,15 @@ lp_setup_set_fragment_sampler_views(struct lp_setup_context *setup,
>                                       unsigned num,
>                                       struct pipe_sampler_view **views)
>   {
> -   unsigned i;
> +   unsigned i, max_tex_num;
>
>      LP_DBG(DEBUG_SETUP, "%s\n", __FUNCTION__);
>
>      assert(num <= PIPE_MAX_SHADER_SAMPLER_VIEWS);
>
> -   for (i = 0; i < PIPE_MAX_SHADER_SAMPLER_VIEWS; i++) {
> +   max_tex_num = MAX2(num, setup->fs.tex_num_last);
> +
> +   for (i = 0; i < max_tex_num; i++) {
>         struct pipe_sampler_view *view = i < num ? views[i] : NULL;
>
>         if (view) {
> @@ -922,7 +924,11 @@ lp_setup_set_fragment_sampler_views(struct lp_setup_context *setup,
>               assert(jit_tex->base);
>            }
>         }
> +      else {
> +         pipe_resource_reference(&setup->fs.current_tex[i], NULL);
> +      }
>      }
> +   setup->fs.tex_num_last = num;
>
>      setup->dirty |= LP_SETUP_NEW_FS;
>   }
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup_context.h b/src/gallium/drivers/llvmpipe/lp_setup_context.h
> index 80acd74..6cef55f 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup_context.h
> +++ b/src/gallium/drivers/llvmpipe/lp_setup_context.h
> @@ -133,6 +133,7 @@ struct lp_setup_context
>         const struct lp_rast_state *stored; /**< what's in the scene */
>         struct lp_rast_state current;  /**< currently set state */
>         struct pipe_resource *current_tex[PIPE_MAX_SHADER_SAMPLER_VIEWS];
> +      unsigned tex_num_last;

I think `current_tex_num` might be more self-documenting -- making it 
clear it refers current_tex array.

No need for `last`.

>      } fs;
>
>      /** fragment shader constants */
>

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>

Jose