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

2017-08-21 Thread Marek Olšák 
On Mon, Aug 21, 2017 at 8:36 AM, Gert Wollny  wrote:
> Am Sonntag, den 20.08.2017, 15:48 +0200 schrieb Marek Olšák:
>> The patch seems OK to me.
>
> Hmm,
>
> you're right in that I was wrong about "program", I still need learn to
> think in patches.
>
> However, when I look at si_delete_compute_state where
> si_compute_reference is called:
>
>
>>+   si_compute_reference(, NULL);
>>+}
> ...
>>+static inline void
>>+si_compute_reference(struct si_compute **dst, struct si_compute *src)
>>+{
>>+ if (pipe_reference(&(*dst)->reference, >reference))
>>+   si_destroy_compute(*dst);
>
> then src will be a NULL pointer and de-referencing it is most likely
> already undefined behavior.
>
> Anyway, in the end the address of a member is taken (i.e.
> src+offsetof(src, reference), and passed as a pointer, and this pointer
>  is later de-referenced in
>
>   pipe_reference_described
>
> if it is not 0.
>
> For now this runs through because "reference" is the first element in
> the si_compute struct, and hence its offset is 0 and >reference
> will also be 0, but the moment someone decides to reorder si_compute
> this may no longer the case and the code would break.
>
> At the least it should be commented that the element reference within
> the structure si_compute must always be at offset 0.

You're right. All pipe_reference declarations should always be first.
And that's why they are always first. It's an unwritten rule in
Gallium.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-21 Thread Gert Wollny 
Am Sonntag, den 20.08.2017, 15:48 +0200 schrieb Marek Olšák:
> The patch seems OK to me.

Hmm, 

you're right in that I was wrong about "program", I still need learn to
think in patches. 

However, when I look at si_delete_compute_state where
si_compute_reference is called: 

 
>+   si_compute_reference(, NULL);
>+}
...
>+static inline void
>+si_compute_reference(struct si_compute **dst, struct si_compute *src)
>+{
>+ if (pipe_reference(&(*dst)->reference, >reference))
>+   si_destroy_compute(*dst);

then src will be a NULL pointer and de-referencing it is most likely
already undefined behavior. 

Anyway, in the end the address of a member is taken (i.e.
src+offsetof(src, reference), and passed as a pointer, and this pointer
 is later de-referenced in 

  pipe_reference_described

if it is not 0. 

For now this runs through because "reference" is the first element in
the si_compute struct, and hence its offset is 0 and >reference
will also be 0, but the moment someone decides to reorder si_compute
this may no longer the case and the code would break.

At the least it should be commented that the element reference within
the structure si_compute must always be at offset 0.

Best, 
Gert

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-20 Thread Marek Olšák 
The patch seems OK to me.

Marek

On Wed, Aug 16, 2017 at 1:50 PM, Gert Wollny  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 
>>
>> 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(>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(>screen-
>> >shader_compiler_queue,
>> + >ready);
>> + util_queue_fence_destroy(>ready);
>> + }
>> +
>> + si_shader_destroy(>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(>screen-
>> >shader_compiler_queue,
>> - >ready);
>> - util_queue_fence_destroy(>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(>shader);
>> - FREE(program);
>> + si_compute_reference(, 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, >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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-08-16 Thread Nicolai Hähnle 
From: Nicolai Hähnle 

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(>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(>screen->shader_compiler_queue,
+   >ready);
+   util_queue_fence_destroy(>ready);
+   }
+
+   si_shader_destroy(>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(>screen->shader_compiler_queue,
-   >ready);
-   util_queue_fence_destroy(>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(>shader);
-   FREE(program);
+   si_compute_reference(, NULL);
 }
 
 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, >reference))
+   si_destroy_compute(*dst);
+
+   *dst = src;
+}
+
 #endif /* SI_COMPUTE_H */
-- 
2.11.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev