[Mesa-dev] [PATCH 1/4] r600g/compute: Don't leak cbufs in compute state

[Aaron Watry]

On Mon, Nov 17, 2014 at 1:45 AM, Michel Dänzer <michel at 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 <awatry at 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 <awatry at 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 at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at 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