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

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