[Mesa-dev] [PATCH 07/16] radeonsi: add reference count to si_compute

[Marek Olšák]

The patch seems OK to me.

Marek

On Wed, Aug 16, 2017 at 1:50 PM, Gert Wollny <gw.fossdev at gmail.com> wrote:
> Hello Nicolai,
>
> I've spotted a little problem in you code (see below).
>
> Best,
> Gert
>
>
> Am Mittwoch, den 16.08.2017, 13:05 +0200 schrieb Nicolai Hähnle:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> To allow keep-alive for deferred logging.
>> ---
>>  src/gallium/drivers/radeonsi/si_compute.c | 24 ++++++++++++++-------
>> ---
>>  src/gallium/drivers/radeonsi/si_compute.h | 14 ++++++++++++++
>>  2 files changed, 28 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/gallium/drivers/radeonsi/si_compute.c
>> b/src/gallium/drivers/radeonsi/si_compute.c
>> index 5efdd3919d2..8c016ba65ab 100644
>> --- a/src/gallium/drivers/radeonsi/si_compute.c
>> +++ b/src/gallium/drivers/radeonsi/si_compute.c
>> @@ -151,6 +151,7 @@ static void *si_create_compute_state(
>>       struct si_screen *sscreen = (struct si_screen *)ctx->screen;
>>       struct si_compute *program = CALLOC_STRUCT(si_compute);
>>
>> +     pipe_reference_init(&program->reference, 1);
>>       program->screen = (struct si_screen *)ctx->screen;
>>       program->ir_type = cso->ir_type;
>>       program->local_size = cso->req_local_mem;
>> @@ -855,20 +856,24 @@ static void si_launch_grid(
>>               sctx->b.flags |= SI_CONTEXT_CS_PARTIAL_FLUSH;
>>  }
>>
>> +void si_destroy_compute(struct si_compute *program)
>> +{
>> +     if (program->ir_type == PIPE_SHADER_IR_TGSI) {
>> +             util_queue_drop_job(&program->screen-
>> >shader_compiler_queue,
>> +                                 &program->ready);
>> +             util_queue_fence_destroy(&program->ready);
>> +     }
>> +
>> +     si_shader_destroy(&program->shader);
>> +     FREE(program);
>> +}
>>
>>  static void si_delete_compute_state(struct pipe_context *ctx, void*
>> state){
>>       struct si_compute *program = (struct si_compute *)state;
>>       struct si_context *sctx = (struct si_context*)ctx;
>>
>> -     if (!state) {
>> +     if (!state)
>>               return;
>> -     }
>> -
>> -     if (program->ir_type == PIPE_SHADER_IR_TGSI) {
>> -             util_queue_drop_job(&sctx->screen-
>> >shader_compiler_queue,
>> -                                 &program->ready);
>> -             util_queue_fence_destroy(&program->ready);
>> -     }
>>
>>       if (program == sctx->cs_shader_state.program)
>>               sctx->cs_shader_state.program = NULL;
>> @@ -876,8 +881,7 @@ static void si_delete_compute_state(struct
>> pipe_context *ctx, void* state){
>>       if (program == sctx->cs_shader_state.emitted_program)
>>               sctx->cs_shader_state.emitted_program = NULL;
>>
>> -     si_shader_destroy(&program->shader);
>> -     FREE(program);
>> +     si_compute_reference(&program, NULL);
>>  }
> Here you call "si_compute_reference" with parameter src=NULL and de-
> reference it in that function.
> Likewise you call FREE on "program" and also dereference it there.
> (see below).
>
>>
>>  static void si_set_compute_resources(struct pipe_context * ctx_,
>> diff --git a/src/gallium/drivers/radeonsi/si_compute.h
>> b/src/gallium/drivers/radeonsi/si_compute.h
>> index 268817b23a6..c19b701fc71 100644
>> --- a/src/gallium/drivers/radeonsi/si_compute.h
>> +++ b/src/gallium/drivers/radeonsi/si_compute.h
>> @@ -24,11 +24,14 @@
>>  #ifndef SI_COMPUTE_H
>>  #define SI_COMPUTE_H
>>
>> +#include "util/u_inlines.h"
>> +
>>  #include "si_shader.h"
>>
>>  #define MAX_GLOBAL_BUFFERS 22
>>
>>  struct si_compute {
>> +     struct pipe_reference reference;
>>       struct si_screen *screen;
>>       struct tgsi_token *tokens;
>>       struct util_queue_fence ready;
>> @@ -53,4 +56,15 @@ struct si_compute {
>>       unsigned uses_bindless_images:1;
>>  };
>>
>> +void si_destroy_compute(struct si_compute *program);
>> +
>> +static inline void
>> +si_compute_reference(struct si_compute **dst, struct si_compute
>> *src)
>> +{
>> +     if (pipe_reference(&(*dst)->reference, &src->reference))
> +       si_destroy_compute(*dst);
>
> When called from si_delete_compute_state *dst has been FREEed and src
> is NULL.
>
>> +
>> +     *dst = src;
>> +}
>> +
>>  #endif /* SI_COMPUTE_H */
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev