Re: [Mesa-dev] [PATCH] i965: store reference to the context within struct brw_fence

[Chih-Wei Huang]

a

2015-09-30 5:36 GMT+08:00 Chad Versace :
> On Mon 28 Sep 2015, Marek Olšák wrote:
>> On Mon, Sep 28, 2015 at 1:29 PM, Emil Velikov  
>> wrote:
>> > As the spec allows for {server,client}_wait_sync to be called without
>> > currently bound context, while our implementation requires context
>> > pointer.
>> >
>> > UNTESTED.
>> >
>> > Cc: Chad Versace 
>> > Cc: Marek Olšák 
>> > Cc: Chih-Wei Huang 
>> > Cc: Mauro Rossi 
>> > Cc: 10.6 11.0 
>> > Signed-off-by: Emil Velikov 
>> > ---
>> >
>> > Upon second thought I'm leaning that we should move this to the API
>> > specific library (i.e. libEGL) rather than keeping it here. It will give
>> > us remove API specific from the DRI extension, plus it'll give us better
>> > mix'n'match (loader+dri module) compatibility.
>> >
>> > How do you guys feel on the topic ?
>>
>> I don't think there is anything to move into libEGL. st/dri doesn't
>> add a context pointer to the fence, instead it adds a screen pointer,
>> which is a different thing.
>
> I agree with Mark here.  Even if there was something you could move into
> libEGL, I don't it could be done safely. It needs to remain in the
> driver.
>
>> This i965 fix is probably not thread-safe, because eglClientWaitSync
>> can be called from another thread and contexts typically contain no
>> locks.
>
> Emil, the fix is not thread-safe because drm_intel_gem_bo_wait() mutates
> the bo. Thread-safety matters because some Chromium Ozone code that's
> cooking does call eglCreateSync and eglClientWaitSync from different
> threads.
>
> If you add a mutex to struct brw_fence, and lock it for the duration of
> brw_fence_client_wait() and brw_fence_is_completed(), then I think
> that's sufficient for ensuring thread-safety.

Any update?

I can confirm the patch (+ 2 Marek's patches)
fix the crashing in android-x86.
But I don't know whether if the
thread-safe issue affect us.

Since I'm going to release lollipop-x86 soon
and the camera crashing is a must fix bug.
I'll just apply the patch to the lollipop-x86
if no further update in 1-2 day(s).


-- 
Chih-Wei
Android-x86 project
http://www.android-x86.org
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: store reference to the context within struct brw_fence

[Chad Versace]

On Mon 28 Sep 2015, Marek Olšák wrote:
> On Mon, Sep 28, 2015 at 1:29 PM, Emil Velikov  
> wrote:
> > As the spec allows for {server,client}_wait_sync to be called without
> > currently bound context, while our implementation requires context
> > pointer.
> >
> > UNTESTED.
> >
> > Cc: Chad Versace 
> > Cc: Marek Olšák 
> > Cc: Chih-Wei Huang 
> > Cc: Mauro Rossi 
> > Cc: 10.6 11.0 
> > Signed-off-by: Emil Velikov 
> > ---
> >
> > Upon second thought I'm leaning that we should move this to the API
> > specific library (i.e. libEGL) rather than keeping it here. It will give
> > us remove API specific from the DRI extension, plus it'll give us better
> > mix'n'match (loader+dri module) compatibility.
> >
> > How do you guys feel on the topic ?
> 
> I don't think there is anything to move into libEGL. st/dri doesn't
> add a context pointer to the fence, instead it adds a screen pointer,
> which is a different thing.

I agree with Mark here.  Even if there was something you could move into
libEGL, I don't it could be done safely. It needs to remain in the
driver.

> This i965 fix is probably not thread-safe, because eglClientWaitSync
> can be called from another thread and contexts typically contain no
> locks.

Emil, the fix is not thread-safe because drm_intel_gem_bo_wait() mutates
the bo. Thread-safety matters because some Chromium Ozone code that's
cooking does call eglCreateSync and eglClientWaitSync from different
threads.

If you add a mutex to struct brw_fence, and lock it for the duration of
brw_fence_client_wait() and brw_fence_is_completed(), then I think
that's sufficient for ensuring thread-safety.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965: store reference to the context within struct brw_fence

[Emil Velikov]

As the spec allows for {server,client}_wait_sync to be called without
currently bound context, while our implementation requires context
pointer.

UNTESTED.

Cc: Chad Versace 
Cc: Marek Olšák 
Cc: Chih-Wei Huang 
Cc: Mauro Rossi 
Cc: 10.6 11.0 
Signed-off-by: Emil Velikov 
---

Upon second thought I'm leaning that we should move this to the API 
specific library (i.e. libEGL) rather than keeping it here. It will give 
us remove API specific from the DRI extension, plus it'll give us better
mix'n'match (loader+dri module) compatibility.

How do you guys feel on the topic ?

-Emil


 src/mesa/drivers/dri/i965/intel_syncobj.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_syncobj.c 
b/src/mesa/drivers/dri/i965/intel_syncobj.c
index c44c4be..3e359a5 100644
--- a/src/mesa/drivers/dri/i965/intel_syncobj.c
+++ b/src/mesa/drivers/dri/i965/intel_syncobj.c
@@ -45,6 +45,7 @@
 #include "intel_reg.h"
 
 struct brw_fence {
+   struct brw_context *brw;
/** The fence waits for completion of this batch. */
drm_intel_bo *batch_bo;
 
@@ -214,6 +215,7 @@ intel_dri_create_fence(__DRIcontext *ctx)
if (!fence)
   return NULL;
 
+   fence->brw = brw;
brw_fence_insert(brw, fence);
 
return fence;
@@ -232,19 +234,17 @@ static GLboolean
 intel_dri_client_wait_sync(__DRIcontext *ctx, void *driver_fence, unsigned 
flags,
uint64_t timeout)
 {
-   struct brw_context *brw = ctx->driverPrivate;
struct brw_fence *fence = driver_fence;
 
-   return brw_fence_client_wait(brw, fence, timeout);
+   return brw_fence_client_wait(fence->brw, fence, timeout);
 }
 
 static void
 intel_dri_server_wait_sync(__DRIcontext *ctx, void *driver_fence, unsigned 
flags)
 {
-   struct brw_context *brw = ctx->driverPrivate;
struct brw_fence *fence = driver_fence;
 
-   brw_fence_server_wait(brw, fence);
+   brw_fence_server_wait(fence->brw, fence);
 }
 
 const __DRI2fenceExtension intelFenceExtension = {
-- 
2.5.0

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

Re: [Mesa-dev] [PATCH] i965: store reference to the context within struct brw_fence

[Marek Olšák]

On Mon, Sep 28, 2015 at 1:29 PM, Emil Velikov  wrote:
> As the spec allows for {server,client}_wait_sync to be called without
> currently bound context, while our implementation requires context
> pointer.
>
> UNTESTED.
>
> Cc: Chad Versace 
> Cc: Marek Olšák 
> Cc: Chih-Wei Huang 
> Cc: Mauro Rossi 
> Cc: 10.6 11.0 
> Signed-off-by: Emil Velikov 
> ---
>
> Upon second thought I'm leaning that we should move this to the API
> specific library (i.e. libEGL) rather than keeping it here. It will give
> us remove API specific from the DRI extension, plus it'll give us better
> mix'n'match (loader+dri module) compatibility.
>
> How do you guys feel on the topic ?

I don't think there is anything to move into libEGL. st/dri doesn't
add a context pointer to the fence, instead it adds a screen pointer,
which is a different thing.

This i965 fix is probably not thread-safe, because eglClientWaitSync
can be called from another thread and contexts typically contain no
locks.

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

Re: [Mesa-dev] [PATCH] i965: store reference to the context within struct brw_fence

[Emil Velikov]

On 28 September 2015 at 14:15, Marek Olšák  wrote:
> On Mon, Sep 28, 2015 at 1:29 PM, Emil Velikov  
> wrote:
>> As the spec allows for {server,client}_wait_sync to be called without
>> currently bound context, while our implementation requires context
>> pointer.
>>
>> UNTESTED.
>>
>> Cc: Chad Versace 
>> Cc: Marek Olšák 
>> Cc: Chih-Wei Huang 
>> Cc: Mauro Rossi 
>> Cc: 10.6 11.0 
>> Signed-off-by: Emil Velikov 
>> ---
>>
>> Upon second thought I'm leaning that we should move this to the API
>> specific library (i.e. libEGL) rather than keeping it here. It will give
>> us remove API specific from the DRI extension, plus it'll give us better
>> mix'n'match (loader+dri module) compatibility.
>>
>> How do you guys feel on the topic ?
>
> I don't think there is anything to move into libEGL. st/dri doesn't
> add a context pointer to the fence, instead it adds a screen pointer,
> which is a different thing.
>
> This i965 fix is probably not thread-safe, because eglClientWaitSync
> can be called from another thread and contexts typically contain no
> locks.
>
Indeed, it's not MT safe. I forgot about the wonders of multithreaded GL.

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