On 16/10/17 02:21 PM, Thomas Hellstrom wrote:
>
> On 10/16/2017 12:53 PM, Thomas Hellstrom wrote:
>> Hi, Michel,
>>
>> On 10/16/2017 12:35 PM, Michel Dänzer wrote:
>>> Hi Thomas,
>>>
>>> On 05/09/17 10:15 AM, Thomas Hellstrom wrote:
>>>> If we're seeing a drawable size change, in particular after
>>>> processing a
>>>> configure notify event, make sure we invalidate so that the state
>>>> tracker
>>>> picks up the new geometry.
>>>>
>>>> Signed-off-by: Thomas Hellstrom <thellst...@vmware.com>
>>>> ---
>>>> src/loader/loader_dri3_helper.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/src/loader/loader_dri3_helper.c
>>>> b/src/loader/loader_dri3_helper.c
>>>> index 51e4e97..bcd5a66 100644
>>>> --- a/src/loader/loader_dri3_helper.c
>>>> +++ b/src/loader/loader_dri3_helper.c
>>>> @@ -348,6 +348,7 @@ dri3_handle_present_event(struct
>>>> loader_dri3_drawable *draw,
>>>> draw->width = ce->width;
>>>> draw->height = ce->height;
>>>> draw->vtable->set_drawable_size(draw, draw->width,
>>>> draw->height);
>>>> + draw->ext->flush->invalidate(draw->dri_drawable);
>>>> break;
>>>> }
>>>> case XCB_PRESENT_COMPLETE_NOTIFY: {
>>>> @@ -1592,6 +1593,7 @@ loader_dri3_update_drawable_geometry(struct
>>>> loader_dri3_drawable *draw)
>>>> draw->width = geom_reply->width;
>>>> draw->height = geom_reply->height;
>>>> draw->vtable->set_drawable_size(draw, draw->width,
>>>> draw->height);
>>>> + draw->ext->flush->invalidate(draw->dri_drawable);
>>>> free(geom_reply);
>>>> }
>>>>
>>> unfortunately, I just bisected a regression to this commit. With it, the
>>> pipe_resource textures backing DRI3 back buffers are leaked when the
>>> window is resized:
>>>
>>> ==3408== 273,760 (228,464 direct, 45,296 indirect) bytes in 131
>>> blocks are definitely lost in loss record 1,285 of 1,286
>>> ==3408== at 0x4C2DC05: calloc (vg_replace_malloc.c:711)
>>> ==3408== by 0xA04D5D3: r600_texture_create_object
>>> (r600_texture.c:1125)
>>> ==3408== by 0xA04E1C1: si_texture_create (r600_texture.c:1382)
>>> ==3408== by 0x9CE3A17: dri2_allocate_textures (dri2.c:803)
>>> ==3408== by 0x9CDDAE2: dri_st_framebuffer_validate
>>> (dri_drawable.c:85)
>>> ==3408== by 0x9B6D07F: st_framebuffer_validate (st_manager.c:195)
>>> ==3408== by 0x9B6EE84: st_manager_validate_framebuffers
>>> (st_manager.c:1063)
>>> ==3408== by 0x9B21807: st_validate_state (st_atom.c:202)
>>> ==3408== by 0x9B2AF75: st_Clear (st_cb_clear.c:411)
>>> ==3408== by 0x10A6BD: ??? (in /usr/bin/glxgears)
>>> ==3408== by 0x109E37: ??? (in /usr/bin/glxgears)
>>> ==3408== by 0x5E202E0: (below main) (libc-start.c:291)
>>> ==3408==
>>> ==3408== 362,768 (230,208 direct, 132,560 indirect) bytes in 132
>>> blocks are definitely lost in loss record 1,286 of 1,286
>>> ==3408== at 0x4C2DC05: calloc (vg_replace_malloc.c:711)
>>> ==3408== by 0xA04D5D3: r600_texture_create_object
>>> (r600_texture.c:1125)
>>> ==3408== by 0xA04E1C1: si_texture_create (r600_texture.c:1382)
>>> ==3408== by 0x9CE0C0D: dri2_create_image_common (dri2.c:1119)
>>> ==3408== by 0x9CE0C6F: dri2_create_image (dri2.c:1140)
>>> ==3408== by 0x5386BA8: dri3_alloc_render_buffer
>>> (loader_dri3_helper.c:1030)
>>> ==3408== by 0x53876F4: dri3_get_buffer.isra.15
>>> (loader_dri3_helper.c:1364)
>>> ==3408== by 0x53886DC: loader_dri3_get_buffers
>>> (loader_dri3_helper.c:1549)
>>> ==3408== by 0x9CE298C: dri_image_drawable_get_buffers (dri2.c:452)
>>> ==3408== by 0x9CE298C: dri2_allocate_textures (dri2.c:576)
>>> ==3408== by 0x9CDDAE2: dri_st_framebuffer_validate
>>> (dri_drawable.c:85)
>>> ==3408== by 0x9B6D07F: st_framebuffer_validate (st_manager.c:195)
>>> ==3408== by 0x9B6EE84: st_manager_validate_framebuffers
>>> (st_manager.c:1063)
>>>
>>>
>>> Can you reproduce this with vmwgfx as well? Any ideas why this is
>>> happening / how to fix it?
>>>
>>>
>> Looks like other buffers (depth ?) are leaked too, outside dri3, right?
>>
>> I'll take a look.
>>
>> /Thomas
>>
>>
> Could you try the attached patch? Fixes the issue on my side.
Yes, it fixes the problem for me as well, thanks! I was looking at that
code as well, but didn't realize what's going on.
I think the attached patch would be a cleaner solution though.
Some comments on your patch, assuming we end up going with that:
> + /* If we need to rerun the validation, make sure we unreference the
> + * textures first. The validate function will just clear the pointers.
> + */
> +
> + if (stfb->iface_stamp != new_stamp) {
> + for (i = 0; i < stfb->num_statts; ++i) {
> + pipe_resource_reference(&textures[i], NULL);
> + }
> + }
I'd drop the blank line after the comment and the curly braces around
the inner loop, but either way, with
Fixes: e96d175c7d2e "loader/dri3: Make sure we invalidate a drawable on
size change"
added to the commit log,
Reviewed-and-Tested-by: Michel Dänzer <michel.daen...@amd.com>
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
From 9eec660e3374ea43d82be9a60769dc001971b034 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michel=20D=C3=A4nzer?= <michel.daen...@amd.com>
Date: Mon, 16 Oct 2017 16:35:18 +0200
Subject: [PATCH] st/mesa: Initialize textures array in st_framebuffer_validate
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
And just reference pipe_resources to it in the validate callbacks.
Avoids pipe_resource leaks when st_framebuffer_validate ends up calling
the validate callback multiple times.
Fixes: e96d175c7d2e "loader/dri3: Make sure we invalidate a drawable on
size change"
Signed-off-by: Michel Dänzer <michel.daen...@amd.com>
---
src/gallium/state_trackers/dri/dri_drawable.c | 4 +---
src/gallium/state_trackers/glx/xlib/xm_st.c | 4 +---
src/gallium/state_trackers/hgl/hgl.c | 4 +---
src/gallium/state_trackers/osmesa/osmesa.c | 1 +
src/gallium/state_trackers/wgl/stw_st.c | 4 +---
src/mesa/state_tracker/st_manager.c | 12 ++++++++++--
6 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/src/gallium/state_trackers/dri/dri_drawable.c b/src/gallium/state_trackers/dri/dri_drawable.c
index 75a8197d330..d586b7564ef 100644
--- a/src/gallium/state_trackers/dri/dri_drawable.c
+++ b/src/gallium/state_trackers/dri/dri_drawable.c
@@ -99,10 +99,8 @@ dri_st_framebuffer_validate(struct st_context_iface *stctx,
return TRUE;
/* Set the window-system buffers for the state tracker. */
- for (i = 0; i < count; i++) {
- out[i] = NULL;
+ for (i = 0; i < count; i++)
pipe_resource_reference(&out[i], textures[statts[i]]);
- }
return TRUE;
}
diff --git a/src/gallium/state_trackers/glx/xlib/xm_st.c b/src/gallium/state_trackers/glx/xlib/xm_st.c
index 0c42e653c76..946b5dcff29 100644
--- a/src/gallium/state_trackers/glx/xlib/xm_st.c
+++ b/src/gallium/state_trackers/glx/xlib/xm_st.c
@@ -245,10 +245,8 @@ xmesa_st_framebuffer_validate(struct st_context_iface *stctx,
}
}
- for (i = 0; i < count; i++) {
- out[i] = NULL;
+ for (i = 0; i < count; i++)
pipe_resource_reference(&out[i], xstfb->textures[statts[i]]);
- }
return TRUE;
}
diff --git a/src/gallium/state_trackers/hgl/hgl.c b/src/gallium/state_trackers/hgl/hgl.c
index 1b702815a3a..bbc477a978c 100644
--- a/src/gallium/state_trackers/hgl/hgl.c
+++ b/src/gallium/state_trackers/hgl/hgl.c
@@ -193,10 +193,8 @@ hgl_st_framebuffer_validate(struct st_context_iface *stctxi,
//}
}
- for (i = 0; i < count; i++) {
- out[i] = NULL;
+ for (i = 0; i < count; i++)
pipe_resource_reference(&out[i], buffer->textures[statts[i]]);
- }
return TRUE;
}
diff --git a/src/gallium/state_trackers/osmesa/osmesa.c b/src/gallium/state_trackers/osmesa/osmesa.c
index 2f9558db312..44a0cc43812 100644
--- a/src/gallium/state_trackers/osmesa/osmesa.c
+++ b/src/gallium/state_trackers/osmesa/osmesa.c
@@ -432,6 +432,7 @@ osmesa_st_framebuffer_validate(struct st_context_iface *stctx,
templat.format = format;
templat.bind = bind;
+ pipe_resource_reference(&out[i], NULL);
out[i] = osbuffer->textures[statts[i]] =
screen->resource_create(screen, &templat);
}
diff --git a/src/gallium/state_trackers/wgl/stw_st.c b/src/gallium/state_trackers/wgl/stw_st.c
index 5e165c89f56..7cf18f0a8b0 100644
--- a/src/gallium/state_trackers/wgl/stw_st.c
+++ b/src/gallium/state_trackers/wgl/stw_st.c
@@ -161,10 +161,8 @@ stw_st_framebuffer_validate(struct st_context_iface *stctx,
stwfb->fb->must_resize = FALSE;
}
- for (i = 0; i < count; i++) {
- out[i] = NULL;
+ for (i = 0; i < count; i++)
pipe_resource_reference(&out[i], stwfb->textures[statts[i]]);
- }
stw_framebuffer_unlock(stwfb->fb);
diff --git a/src/mesa/state_tracker/st_manager.c b/src/mesa/state_tracker/st_manager.c
index 50bc3c33c62..76b7b46ff51 100644
--- a/src/mesa/state_tracker/st_manager.c
+++ b/src/mesa/state_tracker/st_manager.c
@@ -53,6 +53,7 @@
#include "pipe/p_context.h"
#include "pipe/p_screen.h"
+#include "os/os_memory.h"
#include "util/u_format.h"
#include "util/u_pointer.h"
#include "util/u_inlines.h"
@@ -180,7 +181,7 @@ static void
st_framebuffer_validate(struct st_framebuffer *stfb,
struct st_context *st)
{
- struct pipe_resource *textures[ST_ATTACHMENT_COUNT];
+ struct pipe_resource **textures;
uint width, height;
unsigned i;
boolean changed = FALSE;
@@ -190,11 +191,15 @@ st_framebuffer_validate(struct st_framebuffer *stfb,
if (stfb->iface_stamp == new_stamp)
return;
+ textures = os_calloc(stfb->num_statts, sizeof(textures[0]));
+ if (!textures)
+ return;
+
/* validate the fb */
do {
if (!stfb->iface->validate(&st->iface, stfb->iface, stfb->statts,
stfb->num_statts, textures))
- return;
+ goto cleanup;
stfb->iface_stamp = new_stamp;
new_stamp = p_atomic_read(&stfb->iface->stamp);
@@ -253,6 +258,9 @@ st_framebuffer_validate(struct st_framebuffer *stfb,
++stfb->stamp;
_mesa_resize_framebuffer(st->ctx, &stfb->Base, width, height);
}
+
+cleanup:
+ free(textures);
}
/**
--
2.15.0.rc0
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
