Looks good to me too. Reviewed-by: Charmaine Lee <[email protected]> ________________________________________ From: mesa-dev <[email protected]> on behalf of Jose Fonseca <[email protected]> Sent: Tuesday, April 12, 2016 9:24 AM To: Brian Paul; [email protected] Cc: 11.2 Subject: Re: [Mesa-dev] [PATCH] st/mesa: fix memleak in glDrawPixels cache code
On 12/04/16 16:15, Brian Paul wrote: > If the glDrawPixels size changed, we leaked the previously cached > texture, if there was one. This patch fixes the reference counting, > adds a refcount assertion check, and better handles potential malloc() > failures. > > Tested with a modified version of the drawpix Mesa demo which changed > the image size for each glDrawPixels call. > > Cc: "11.2" <[email protected]> > --- > src/mesa/state_tracker/st_cb_drawpixels.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c > b/src/mesa/state_tracker/st_cb_drawpixels.c > index 01ed544..3c7bc0c 100644 > --- a/src/mesa/state_tracker/st_cb_drawpixels.c > +++ b/src/mesa/state_tracker/st_cb_drawpixels.c > @@ -384,7 +384,7 @@ make_texture(struct st_context *st, > struct gl_context *ctx = st->ctx; > struct pipe_context *pipe = st->pipe; > mesa_format mformat; > - struct pipe_resource *pt; > + struct pipe_resource *pt = NULL; > enum pipe_format pipeFormat; > GLenum baseInternalFormat; > > @@ -403,10 +403,18 @@ make_texture(struct st_context *st, > unpack->SkipRows == 0 && > unpack->SwapBytes == GL_FALSE && > st->drawpix_cache.image) { > + assert(st->drawpix_cache.texture); > + > /* check if the pixel data is the same */ > if (memcmp(pixels, st->drawpix_cache.image, width * height * bpp) == > 0) { > /* OK, re-use the cached texture */ > - return st->drawpix_cache.texture; > + pipe_resource_reference(&pt, st->drawpix_cache.texture); > + /* refcount of returned texture should be at least two here. One > + * reference for the cache to hold on to, one for the caller (which > + * it will release), and possibly more held by the driver. > + */ > + assert(pt->reference.count >= 2); > + return pt; > } > } > > @@ -525,8 +533,14 @@ make_texture(struct st_context *st, > st->drawpix_cache.image = malloc(width * height * bpp); > if (st->drawpix_cache.image) { > memcpy(st->drawpix_cache.image, pixels, width * height * bpp); > + pipe_resource_reference(&st->drawpix_cache.texture, pt); > + } > + else { > + /* out of memory, free/disable cached texture */ > + st->drawpix_cache.width = 0; > + st->drawpix_cache.height = 0; > + pipe_resource_reference(&st->drawpix_cache.texture, NULL); > } > - st->drawpix_cache.texture = pt; > } > #endif > > @@ -1160,9 +1174,8 @@ st_DrawPixels(struct gl_context *ctx, GLint x, GLint y, > if (num_sampler_view > 1) > pipe_sampler_view_reference(&sv[1], NULL); > > -#if !USE_DRAWPIXELS_CACHE > + /* free the texture (but may persist in the cache) */ > pipe_resource_reference(&pt, NULL); > -#endif > } > > Looks good AFAICT. Reviewed-by: Jose Fonseca <[email protected]> _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
