Re: [Mesa-dev] [PATCH 1/2] st/mesa: Take local references for sync object fences

[Nicolai Hähnle]

On 13.10.2016 05:45, Michel Dänzer wrote:

On 12/10/16 08:52 PM, Nicolai Hähnle wrote:

On 12.10.2016 11:31, Michel Dänzer wrote:


diff --git a/src/mesa/state_tracker/st_cb_syncobj.c
b/src/mesa/state_tracker/st_cb_syncobj.c
index 123925a..de01880 100644
--- a/src/mesa/state_tracker/st_cb_syncobj.c
+++ b/src/mesa/state_tracker/st_cb_syncobj.c
@@ -81,17 +81,22 @@ static void st_check_sync(struct gl_context *ctx,
struct gl_sync_object *obj)
struct pipe_context *pipe = st_context(ctx)->pipe;
struct pipe_screen *screen = pipe->screen;
struct st_sync_object *so = (struct st_sync_object*)obj;
+   struct pipe_fence_handle *fence = NULL;
+
+   screen->fence_reference(screen, , so->fence);


This should probably really use p_atomic_read (or some kind of READ_ONCE
macro if we had it) to make the intention clear, but I'm not sure
whether that would compile everywhere since so->fence is a pointer.

In practice it doesn't matter, because the function call is a sufficient
barrier. So either way,

Reviewed-by: Nicolai Hähnle 


Thanks, but unfortunately,
https://bugs.freedesktop.org/show_bug.cgi?id=98172#c5 shows that this
really isn't good enough yet. Back to the drawing board...


I expect that most of the gallium *_reference functions need a similar 
treatment in order to be able to do atomic reference changes.


Nicolai




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

Re: [Mesa-dev] [PATCH 1/2] st/mesa: Take local references for sync object fences

[Michel Dänzer]

On 12/10/16 08:52 PM, Nicolai Hähnle wrote:
> On 12.10.2016 11:31, Michel Dänzer wrote:
>>
>> diff --git a/src/mesa/state_tracker/st_cb_syncobj.c
>> b/src/mesa/state_tracker/st_cb_syncobj.c
>> index 123925a..de01880 100644
>> --- a/src/mesa/state_tracker/st_cb_syncobj.c
>> +++ b/src/mesa/state_tracker/st_cb_syncobj.c
>> @@ -81,17 +81,22 @@ static void st_check_sync(struct gl_context *ctx,
>> struct gl_sync_object *obj)
>> struct pipe_context *pipe = st_context(ctx)->pipe;
>> struct pipe_screen *screen = pipe->screen;
>> struct st_sync_object *so = (struct st_sync_object*)obj;
>> +   struct pipe_fence_handle *fence = NULL;
>> +
>> +   screen->fence_reference(screen, , so->fence);
> 
> This should probably really use p_atomic_read (or some kind of READ_ONCE
> macro if we had it) to make the intention clear, but I'm not sure
> whether that would compile everywhere since so->fence is a pointer.
> 
> In practice it doesn't matter, because the function call is a sufficient
> barrier. So either way,
> 
> Reviewed-by: Nicolai Hähnle 

Thanks, but unfortunately,
https://bugs.freedesktop.org/show_bug.cgi?id=98172#c5 shows that this
really isn't good enough yet. Back to the drawing board...


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

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

Re: [Mesa-dev] [PATCH 1/2] st/mesa: Take local references for sync object fences

[Nicolai Hähnle]

On 12.10.2016 11:31, Michel Dänzer wrote:

From: Michel Dänzer 

Fixes a race condition:

Thread AThread B


Test if so->fence != NULL
=> true
Set so->fence = NULL
Dereference so->fence
=> NULL dereference

Also, taking a reference prevents the fence from being destroyed.


Nice :)



Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98172
Cc: mesa-sta...@lists.freedesktop.org
Signed-off-by: Michel Dänzer 
---
 src/mesa/state_tracker/st_cb_syncobj.c | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/mesa/state_tracker/st_cb_syncobj.c 
b/src/mesa/state_tracker/st_cb_syncobj.c
index 123925a..de01880 100644
--- a/src/mesa/state_tracker/st_cb_syncobj.c
+++ b/src/mesa/state_tracker/st_cb_syncobj.c
@@ -81,17 +81,22 @@ static void st_check_sync(struct gl_context *ctx, struct 
gl_sync_object *obj)
struct pipe_context *pipe = st_context(ctx)->pipe;
struct pipe_screen *screen = pipe->screen;
struct st_sync_object *so = (struct st_sync_object*)obj;
+   struct pipe_fence_handle *fence = NULL;
+
+   screen->fence_reference(screen, , so->fence);


This should probably really use p_atomic_read (or some kind of READ_ONCE 
macro if we had it) to make the intention clear, but I'm not sure 
whether that would compile everywhere since so->fence is a pointer.


In practice it doesn't matter, because the function call is a sufficient 
barrier. So either way,


Reviewed-by: Nicolai Hähnle 



/* If the fence doesn't exist, assume it's signalled. */
-   if (!so->fence) {
+   if (!fence) {
   so->b.StatusFlag = GL_TRUE;
   return;
}

-   if (screen->fence_finish(screen, pipe, so->fence, 0)) {
+   if (screen->fence_finish(screen, pipe, fence, 0)) {
   screen->fence_reference(screen, >fence, NULL);
   so->b.StatusFlag = GL_TRUE;
}
+
+   screen->fence_reference(screen, , NULL);
 }

 static void st_client_wait_sync(struct gl_context *ctx,
@@ -101,9 +106,12 @@ static void st_client_wait_sync(struct gl_context *ctx,
struct pipe_context *pipe = st_context(ctx)->pipe;
struct pipe_screen *screen = pipe->screen;
struct st_sync_object *so = (struct st_sync_object*)obj;
+   struct pipe_fence_handle *fence = NULL;
+
+   screen->fence_reference(screen, , so->fence);

/* If the fence doesn't exist, assume it's signalled. */
-   if (!so->fence) {
+   if (!fence) {
   so->b.StatusFlag = GL_TRUE;
   return;
}
@@ -120,11 +128,12 @@ static void st_client_wait_sync(struct gl_context *ctx,
 * Assume GL_SYNC_FLUSH_COMMANDS_BIT is always set, because applications
 * forget to set it.
 */
-   if (so->fence &&
-   screen->fence_finish(screen, pipe, so->fence, timeout)) {
+   if (screen->fence_finish(screen, pipe, fence, timeout)) {
   screen->fence_reference(screen, >fence, NULL);
   so->b.StatusFlag = GL_TRUE;
}
+
+   screen->fence_reference(screen, , NULL);
 }

 static void st_server_wait_sync(struct gl_context *ctx,


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

[Mesa-dev] [PATCH 1/2] st/mesa: Take local references for sync object fences

[Michel Dänzer]

From: Michel Dänzer 

Fixes a race condition:

Thread AThread B


Test if so->fence != NULL
=> true
Set so->fence = NULL
Dereference so->fence
=> NULL dereference

Also, taking a reference prevents the fence from being destroyed.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98172
Cc: mesa-sta...@lists.freedesktop.org
Signed-off-by: Michel Dänzer 
---
 src/mesa/state_tracker/st_cb_syncobj.c | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/mesa/state_tracker/st_cb_syncobj.c 
b/src/mesa/state_tracker/st_cb_syncobj.c
index 123925a..de01880 100644
--- a/src/mesa/state_tracker/st_cb_syncobj.c
+++ b/src/mesa/state_tracker/st_cb_syncobj.c
@@ -81,17 +81,22 @@ static void st_check_sync(struct gl_context *ctx, struct 
gl_sync_object *obj)
struct pipe_context *pipe = st_context(ctx)->pipe;
struct pipe_screen *screen = pipe->screen;
struct st_sync_object *so = (struct st_sync_object*)obj;
+   struct pipe_fence_handle *fence = NULL;
+
+   screen->fence_reference(screen, , so->fence);
 
/* If the fence doesn't exist, assume it's signalled. */
-   if (!so->fence) {
+   if (!fence) {
   so->b.StatusFlag = GL_TRUE;
   return;
}
 
-   if (screen->fence_finish(screen, pipe, so->fence, 0)) {
+   if (screen->fence_finish(screen, pipe, fence, 0)) {
   screen->fence_reference(screen, >fence, NULL);
   so->b.StatusFlag = GL_TRUE;
}
+
+   screen->fence_reference(screen, , NULL);
 }
 
 static void st_client_wait_sync(struct gl_context *ctx,
@@ -101,9 +106,12 @@ static void st_client_wait_sync(struct gl_context *ctx,
struct pipe_context *pipe = st_context(ctx)->pipe;
struct pipe_screen *screen = pipe->screen;
struct st_sync_object *so = (struct st_sync_object*)obj;
+   struct pipe_fence_handle *fence = NULL;
+
+   screen->fence_reference(screen, , so->fence);
 
/* If the fence doesn't exist, assume it's signalled. */
-   if (!so->fence) {
+   if (!fence) {
   so->b.StatusFlag = GL_TRUE;
   return;
}
@@ -120,11 +128,12 @@ static void st_client_wait_sync(struct gl_context *ctx,
 * Assume GL_SYNC_FLUSH_COMMANDS_BIT is always set, because applications
 * forget to set it.
 */
-   if (so->fence &&
-   screen->fence_finish(screen, pipe, so->fence, timeout)) {
+   if (screen->fence_finish(screen, pipe, fence, timeout)) {
   screen->fence_reference(screen, >fence, NULL);
   so->b.StatusFlag = GL_TRUE;
}
+
+   screen->fence_reference(screen, , NULL);
 }
 
 static void st_server_wait_sync(struct gl_context *ctx,
-- 
2.9.3

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