Re: [Mesa-dev] [PATCH 1/4] r600g/compute: Don't leak cbufs in compute state
On Mon, Nov 17, 2014 at 1:45 AM, Michel Dänzer mic...@daenzer.net wrote:
On 14.11.2014 19:37, Marek Olšák wrote:
surface_destroy should never be called directly, because surfaces have
a reference counter. For unreferencing resources, use
pipe_surface_reference(pointer, NULL). It will call surface_destroy
if needed.
Indeed, if this was the right place for this, it could be done both
easier and more robustly:
for (int i = 0; i fb_state-nr_cbufs; i++)
pipe_surface_reference(fb_state-cbufs[i], NULL);
Yeah, you're right about that. After your (Michel/Marek) replies, I
had come to the same conclusion about simplifying things. I'm having
*fun* deciding where the proper place to put this in the evergreen
code is. We end up calling evergreen_set_rat from multiple places,
which is where the surfaces are created, and then we keep a list with
a set count... and the individual surfaces themselves get freed as
their reference counts change. In theory, they all get
referenced/freed at the same point, but there's no guarantees that I
can see.
The first logical place that I saw to put the de-reference is in
evergreen_set_global_binding and the matching
create/delete_compute_state functions.
This seems to only affect evergreen (See the XXX early return comment
in the evergreen set global binding function, which implies someone
knew this was an issue). SI seems to have all the right de-references
in place, but I haven't verified that yet through actual run, just
inspection.
--Aaron
On Fri, Nov 14, 2014 at 12:43 AM, Aaron Watry awa...@gmail.com wrote:
Walk the array of cbufs backwards and free all of them.
v3: Rebase on top of changes since Aug 2014
Signed-off-by: Aaron Watry awa...@gmail.com
---
src/gallium/drivers/r600/evergreen_compute.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/gallium/drivers/r600/evergreen_compute.c
b/src/gallium/drivers/r600/evergreen_compute.c
index 90fdd79..4334743 100644
--- a/src/gallium/drivers/r600/evergreen_compute.c
+++ b/src/gallium/drivers/r600/evergreen_compute.c
@@ -252,6 +252,15 @@ void evergreen_delete_compute_state(struct
pipe_context *ctx, void* state)
if (!shader)
return;
+ if (shader-ctx){
+ struct pipe_framebuffer_state *fb_state =
shader-ctx-framebuffer.state;
+ for (int i = fb_state-nr_cbufs - 1; fb_state-nr_cbufs 0
; i--){
+ shader-ctx-b.b.surface_destroy(ctx,
fb_state-cbufs[i]);
+ fb_state-cbufs[i] = NULL;
+ fb_state-nr_cbufs--;
+ }
+ }
+
FREE(shader);
}
--
2.1.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Earthling Michel Dänzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] r600g/compute: Don't leak cbufs in compute state
Not sure if that's the right place, but this framebuffer state is set
by the driver and not a state tracker. Compute shader read-write
resources (buffers, images) are implemented by the CB block on r600
and are referred to as RAT (random access target) in the register
docs. The first 0-7 binding slots are shared with colorbuffers and the
8-11 slots are dedicated to compute resources only. For GL extensions
that add read-write buffers and images, the maximum number of
read-write resources is (12 - number_of_colorbuffers) and they are
only supported in the pixel shader.
This craziness doesn't exist on SI, instead, TC just can do stores there.
Marek
On Mon, Nov 17, 2014 at 8:45 AM, Michel Dänzer mic...@daenzer.net wrote:
On 14.11.2014 19:37, Marek Olšák wrote:
surface_destroy should never be called directly, because surfaces have
a reference counter. For unreferencing resources, use
pipe_surface_reference(pointer, NULL). It will call surface_destroy
if needed.
Indeed, if this was the right place for this, it could be done both
easier and more robustly:
for (int i = 0; i fb_state-nr_cbufs; i++)
pipe_surface_reference(fb_state-cbufs[i], NULL);
On Fri, Nov 14, 2014 at 12:43 AM, Aaron Watry awa...@gmail.com wrote:
Walk the array of cbufs backwards and free all of them.
v3: Rebase on top of changes since Aug 2014
Signed-off-by: Aaron Watry awa...@gmail.com
---
src/gallium/drivers/r600/evergreen_compute.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/gallium/drivers/r600/evergreen_compute.c
b/src/gallium/drivers/r600/evergreen_compute.c
index 90fdd79..4334743 100644
--- a/src/gallium/drivers/r600/evergreen_compute.c
+++ b/src/gallium/drivers/r600/evergreen_compute.c
@@ -252,6 +252,15 @@ void evergreen_delete_compute_state(struct
pipe_context *ctx, void* state)
if (!shader)
return;
+ if (shader-ctx){
+ struct pipe_framebuffer_state *fb_state =
shader-ctx-framebuffer.state;
+ for (int i = fb_state-nr_cbufs - 1; fb_state-nr_cbufs 0
; i--){
+ shader-ctx-b.b.surface_destroy(ctx,
fb_state-cbufs[i]);
+ fb_state-cbufs[i] = NULL;
+ fb_state-nr_cbufs--;
+ }
+ }
+
FREE(shader);
}
--
2.1.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Earthling Michel Dänzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] r600g/compute: Don't leak cbufs in compute state
On 17.11.2014 23:30, Aaron Watry wrote:
On Mon, Nov 17, 2014 at 1:45 AM, Michel Dänzer mic...@daenzer.net wrote:
On 14.11.2014 19:37, Marek Olšák wrote:
surface_destroy should never be called directly, because surfaces have
a reference counter. For unreferencing resources, use
pipe_surface_reference(pointer, NULL). It will call surface_destroy
if needed.
Indeed, if this was the right place for this, it could be done both
easier and more robustly:
for (int i = 0; i fb_state-nr_cbufs; i++)
pipe_surface_reference(fb_state-cbufs[i], NULL);
Yeah, you're right about that. After your (Michel/Marek) replies, I
had come to the same conclusion about simplifying things. I'm having
*fun* deciding where the proper place to put this in the evergreen
code is. We end up calling evergreen_set_rat from multiple places,
which is where the surfaces are created, and then we keep a list with
a set count... and the individual surfaces themselves get freed as
their reference counts change. In theory, they all get
referenced/freed at the same point, but there's no guarantees that I
can see.
The first logical place that I saw to put the de-reference is in
evergreen_set_global_binding and the matching
create/delete_compute_state functions.
AFAICT it indeed needs to be handled in evergreen_set_compute_resources
and evergreen_set_global_binding. Does the attached patch work?
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
From 2ee9c0c435405df7382363ae8f276cd3ce4f8c6b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michel=20D=C3=A4nzer?= michel.daen...@amd.com
Date: Tue, 18 Nov 2014 11:46:22 +0900
Subject: [PATCH] r600g/compute: Fix cbuf leaks
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Michel Dänzer michel.daen...@amd.com
---
src/gallium/drivers/r600/evergreen_compute.c | 22 +++---
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/gallium/drivers/r600/evergreen_compute.c b/src/gallium/drivers/r600/evergreen_compute.c
index 90fdd79..64c3cee 100644
--- a/src/gallium/drivers/r600/evergreen_compute.c
+++ b/src/gallium/drivers/r600/evergreen_compute.c
@@ -127,6 +127,7 @@ static void evergreen_set_rat(
rat_templ.u.tex.last_layer = 0;
/* Add the RAT the list of color buffers */
+ 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);
@@ -627,11 +628,16 @@ static void evergreen_set_compute_resources(struct pipe_context * ctx_,
{
struct r600_context *ctx = (struct r600_context *)ctx_;
struct r600_surface **resources = (struct r600_surface **)surfaces;
+ unsigned i;
COMPUTE_DBG(ctx-screen, *** evergreen_set_compute_resources: start = %u count = %u\n,
start, count);
- for (unsigned i = 0; i count; i++) {
+ /* Unreference unused surfaces */
+ for (i = count + 1; i ctx-framebuffer.state.nr_cbufs; i++)
+ pipe_surface_reference(ctx-framebuffer.state.cbufs[i], NULL);
+
+ for (i = 0; i count; i++) {
/* The First two vertex buffers are reserved for parameters and
* global buffers. */
unsigned vtx_id = 2 + i;
@@ -651,6 +657,9 @@ static void evergreen_set_compute_resources(struct pipe_context * ctx_,
evergreen_cs_set_vertex_buffer(ctx, vtx_id,
buffer-chunk-start_in_dw * 4,
resources[i]-base.texture);
+ } else {
+ pipe_surface_reference(ctx-framebuffer.state.cbufs[i+1],
+ NULL);
}
}
}
@@ -688,10 +697,11 @@ static void evergreen_set_global_binding(
COMPUTE_DBG(ctx-screen, *** evergreen_set_global_binding first = %u n = %u\n,
first, n);
- if (!resources) {
- /* XXX: Unset */
+ /* Unreference previous resource */
+ pipe_surface_reference(ctx-framebuffer.state.cbufs[0], NULL);
+
+ if (!resources)
return;
- }
/* We mark these items for promotion to the pool if they
* aren't already there */
@@ -702,10 +712,8 @@ static void evergreen_set_global_binding(
buffers[i]-chunk-status |= ITEM_FOR_PROMOTING;
}
- if (compute_memory_finalize_pending(pool, ctx_) == -1) {
- /* XXX: Unset */
+ if (compute_memory_finalize_pending(pool, ctx_) == -1)
return;
- }
for (i = first; i first + n; i++)
{
--
2.1.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] r600g/compute: Don't leak cbufs in compute state
On 14.11.2014 19:37, Marek Olšák wrote:
surface_destroy should never be called directly, because surfaces have
a reference counter. For unreferencing resources, use
pipe_surface_reference(pointer, NULL). It will call surface_destroy
if needed.
Indeed, if this was the right place for this, it could be done both
easier and more robustly:
for (int i = 0; i fb_state-nr_cbufs; i++)
pipe_surface_reference(fb_state-cbufs[i], NULL);
On Fri, Nov 14, 2014 at 12:43 AM, Aaron Watry awa...@gmail.com wrote:
Walk the array of cbufs backwards and free all of them.
v3: Rebase on top of changes since Aug 2014
Signed-off-by: Aaron Watry awa...@gmail.com
---
src/gallium/drivers/r600/evergreen_compute.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/gallium/drivers/r600/evergreen_compute.c
b/src/gallium/drivers/r600/evergreen_compute.c
index 90fdd79..4334743 100644
--- a/src/gallium/drivers/r600/evergreen_compute.c
+++ b/src/gallium/drivers/r600/evergreen_compute.c
@@ -252,6 +252,15 @@ void evergreen_delete_compute_state(struct pipe_context
*ctx, void* state)
if (!shader)
return;
+ if (shader-ctx){
+ struct pipe_framebuffer_state *fb_state =
shader-ctx-framebuffer.state;
+ for (int i = fb_state-nr_cbufs - 1; fb_state-nr_cbufs 0
; i--){
+ shader-ctx-b.b.surface_destroy(ctx,
fb_state-cbufs[i]);
+ fb_state-cbufs[i] = NULL;
+ fb_state-nr_cbufs--;
+ }
+ }
+
FREE(shader);
}
--
2.1.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Earthling Michel Dänzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] r600g/compute: Don't leak cbufs in compute state
surface_destroy should never be called directly, because surfaces have
a reference counter. For unreferencing resources, use
pipe_surface_reference(pointer, NULL). It will call surface_destroy
if needed.
Marek
On Fri, Nov 14, 2014 at 12:43 AM, Aaron Watry awa...@gmail.com wrote:
Walk the array of cbufs backwards and free all of them.
v3: Rebase on top of changes since Aug 2014
Signed-off-by: Aaron Watry awa...@gmail.com
---
src/gallium/drivers/r600/evergreen_compute.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/gallium/drivers/r600/evergreen_compute.c
b/src/gallium/drivers/r600/evergreen_compute.c
index 90fdd79..4334743 100644
--- a/src/gallium/drivers/r600/evergreen_compute.c
+++ b/src/gallium/drivers/r600/evergreen_compute.c
@@ -252,6 +252,15 @@ void evergreen_delete_compute_state(struct pipe_context
*ctx, void* state)
if (!shader)
return;
+ if (shader-ctx){
+ struct pipe_framebuffer_state *fb_state =
shader-ctx-framebuffer.state;
+ for (int i = fb_state-nr_cbufs - 1; fb_state-nr_cbufs 0 ;
i--){
+ shader-ctx-b.b.surface_destroy(ctx,
fb_state-cbufs[i]);
+ fb_state-cbufs[i] = NULL;
+ fb_state-nr_cbufs--;
+ }
+ }
+
FREE(shader);
}
--
2.1.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/4] r600g/compute: Don't leak cbufs in compute state
Walk the array of cbufs backwards and free all of them.
v3: Rebase on top of changes since Aug 2014
Signed-off-by: Aaron Watry awa...@gmail.com
---
src/gallium/drivers/r600/evergreen_compute.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/gallium/drivers/r600/evergreen_compute.c
b/src/gallium/drivers/r600/evergreen_compute.c
index 90fdd79..4334743 100644
--- a/src/gallium/drivers/r600/evergreen_compute.c
+++ b/src/gallium/drivers/r600/evergreen_compute.c
@@ -252,6 +252,15 @@ void evergreen_delete_compute_state(struct pipe_context
*ctx, void* state)
if (!shader)
return;
+ if (shader-ctx){
+ struct pipe_framebuffer_state *fb_state =
shader-ctx-framebuffer.state;
+ for (int i = fb_state-nr_cbufs - 1; fb_state-nr_cbufs 0 ;
i--){
+ shader-ctx-b.b.surface_destroy(ctx,
fb_state-cbufs[i]);
+ fb_state-cbufs[i] = NULL;
+ fb_state-nr_cbufs--;
+ }
+ }
+
FREE(shader);
}
--
2.1.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] r600g/compute: Don't leak cbufs in compute state
On 14.11.2014 08:43, Aaron Watry wrote:
Walk the array of cbufs backwards and free all of them.
v3: Rebase on top of changes since Aug 2014
Signed-off-by: Aaron Watry awa...@gmail.com
---
src/gallium/drivers/r600/evergreen_compute.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/gallium/drivers/r600/evergreen_compute.c
b/src/gallium/drivers/r600/evergreen_compute.c
index 90fdd79..4334743 100644
--- a/src/gallium/drivers/r600/evergreen_compute.c
+++ b/src/gallium/drivers/r600/evergreen_compute.c
@@ -252,6 +252,15 @@ void evergreen_delete_compute_state(struct pipe_context
*ctx, void* state)
if (!shader)
return;
+ if (shader-ctx){
+ struct pipe_framebuffer_state *fb_state =
shader-ctx-framebuffer.state;
+ for (int i = fb_state-nr_cbufs - 1; fb_state-nr_cbufs 0 ;
i--){
+ shader-ctx-b.b.surface_destroy(ctx,
fb_state-cbufs[i]);
+ fb_state-cbufs[i] = NULL;
+ fb_state-nr_cbufs--;
+ }
+ }
I think this is the wrong place to do this. It's the state tracker's
responsibility to set an empty framebuffer state, so that the driver can
unreference the surfaces of the previous framebuffer state.
--
Earthling Michel Dänzer| http://www.amd.com
Libre software enthusiast |Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
