Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe

[gregory hainaut]

On Tue, 09 May 2017 09:31:18 -0700
Eric Anholt  wrote:

> Gregory Hainaut  writes:
> 
> > On 5/8/17, Emil Velikov  wrote:  
>  [...]  
> >
> > Hello Emil,
> >
> > Yes you're right. And potentially it can be back-ported to stable
> > branch. Besides it would allow to know which applications aren't X
> > thread safe. And potentially app owners can fix the issue too.
> >
> > By the way, I don't have commit access so feel free to push the series :)
> >
> > On 5/8/17, Eric Anholt  wrote:  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
> >
> > Hello Eric,
> >
> > Indeed I saw this behavior on Nouveau. I got a crash on
> > GetBuffersWithFormat on DRI2 but it was fine on DRI3. I still kept the
> > check on GLX/DRI3 because I don't know if we have a strong guarantee
> > that X11 is never used.  
> 
> I would be surprised if there was a path that hit X through GL calls as
> opposed to the window system, with DRI3.  GetBuffersWithFormat was the
> problem.

Tbh, I'm a noob. If you're sure, we can always return true for GLX/DRI3. 
Otherwise
I will put a comment that it is likely fine. Your call.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe

[Eric Anholt]

Gregory Hainaut  writes:

> On 5/8/17, Emil Velikov  wrote:
>> Having a look at Xlib might be good indeed.
>>
>> Then again, the solution you've proposed looks perfectly reasonable,
>> IMHO. It handles the problem _now_ and should also work when/if we
>> address Xlib.
>> I'll take another look today/tomorrow, but I think the series is
>> perfectly fine to land as-is.
>>
>> Thanks
>> Emil
>
> Hello Emil,
>
> Yes you're right. And potentially it can be back-ported to stable
> branch. Besides it would allow to know which applications aren't X
> thread safe. And potentially app owners can fix the issue too.
>
> By the way, I don't have commit access so feel free to push the series :)
>
> On 5/8/17, Eric Anholt  wrote:
>> gregory hainaut  writes:
>>
>>> On Fri, 5 May 2017 17:45:01 +0200
>>> Axel Davy  wrote:
>>>
 Hi,

 There should be very few X11 calls while rendering (basically only at
 the beginning or end of a frame).

 Why not just always run these calls in the main thread (and wait for
 glthread work to finish) ?

 That's basically what we do for gallium nine.

 Yours,

 Axel
>>>
>>> Hello Axel,
>>>
>>> Yes it is another possibility. It would requires to track gl calls that
>>> end up in X11.
>>> I'm not sure if there is an easy way to list all those gl functions. There
>>> are at least the
>>> draw calls and maybe the clear operations. Besides I'm afraid that we will
>>> need to handle
>>> various corner cases of the OpenGL API. It is doable but likely more
>>> complicated.
>>
>> General GL calls (draws, clears) won't call X11 except for DRI2's
>> GetBuffers.  If you're in DRI3, I believe you won't need to worry about
>> that at all.
>>
>> I think this patch is a good start, though.
>>
>
> Hello Eric,
>
> Indeed I saw this behavior on Nouveau. I got a crash on
> GetBuffersWithFormat on DRI2 but it was fine on DRI3. I still kept the
> check on GLX/DRI3 because I don't know if we have a strong guarantee
> that X11 is never used.

I would be surprised if there was a path that hit X through GL calls as
opposed to the window system, with DRI3.  GetBuffersWithFormat was the
problem.


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

Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe

[Gregory Hainaut]

On 5/8/17, Emil Velikov  wrote:
> Having a look at Xlib might be good indeed.
>
> Then again, the solution you've proposed looks perfectly reasonable,
> IMHO. It handles the problem _now_ and should also work when/if we
> address Xlib.
> I'll take another look today/tomorrow, but I think the series is
> perfectly fine to land as-is.
>
> Thanks
> Emil

Hello Emil,

Yes you're right. And potentially it can be back-ported to stable
branch. Besides it would allow to know which applications aren't X
thread safe. And potentially app owners can fix the issue too.

By the way, I don't have commit access so feel free to push the series :)

On 5/8/17, Eric Anholt  wrote:
> gregory hainaut  writes:
>
>> On Fri, 5 May 2017 17:45:01 +0200
>> Axel Davy  wrote:
>>
>>> Hi,
>>>
>>> There should be very few X11 calls while rendering (basically only at
>>> the beginning or end of a frame).
>>>
>>> Why not just always run these calls in the main thread (and wait for
>>> glthread work to finish) ?
>>>
>>> That's basically what we do for gallium nine.
>>>
>>> Yours,
>>>
>>> Axel
>>
>> Hello Axel,
>>
>> Yes it is another possibility. It would requires to track gl calls that
>> end up in X11.
>> I'm not sure if there is an easy way to list all those gl functions. There
>> are at least the
>> draw calls and maybe the clear operations. Besides I'm afraid that we will
>> need to handle
>> various corner cases of the OpenGL API. It is doable but likely more
>> complicated.
>
> General GL calls (draws, clears) won't call X11 except for DRI2's
> GetBuffers.  If you're in DRI3, I believe you won't need to worry about
> that at all.
>
> I think this patch is a good start, though.
>

Hello Eric,

Indeed I saw this behavior on Nouveau. I got a crash on
GetBuffersWithFormat on DRI2 but it was fine on DRI3. I still kept the
check on GLX/DRI3 because I don't know if we have a strong guarantee
that X11 is never used.

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

Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe

[Eric Anholt]

gregory hainaut  writes:

> On Fri, 5 May 2017 17:45:01 +0200
> Axel Davy  wrote:
>
>> Hi,
>> 
>> There should be very few X11 calls while rendering (basically only at 
>> the beginning or end of a frame).
>> 
>> Why not just always run these calls in the main thread (and wait for 
>> glthread work to finish) ?
>> 
>> That's basically what we do for gallium nine.
>> 
>> Yours,
>> 
>> Axel
>
> Hello Axel,
>
> Yes it is another possibility. It would requires to track gl calls that end 
> up in X11.
> I'm not sure if there is an easy way to list all those gl functions. There 
> are at least the 
> draw calls and maybe the clear operations. Besides I'm afraid that we will 
> need to handle
> various corner cases of the OpenGL API. It is doable but likely more 
> complicated.

General GL calls (draws, clears) won't call X11 except for DRI2's
GetBuffers.  If you're in DRI3, I believe you won't need to worry about
that at all.

I think this patch is a good start, though.


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

Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe

[Emil Velikov]

On 5 May 2017 at 18:28, gregory hainaut  wrote:
> On Fri, 5 May 2017 18:17:22 +0100
> Emil Velikov  wrote:
>
>> On 5 May 2017 at 17:58, gregory hainaut  wrote:
>> > On Fri, 5 May 2017 17:45:01 +0200
>> > Axel Davy  wrote:
>> >
>>  [...]
>> >
>> > Hello Axel,
>> >
>> > Yes it is another possibility. It would requires to track gl calls that 
>> > end up in X11.
>> > I'm not sure if there is an easy way to list all those gl functions. There 
>> > are at least the
>> > draw calls and maybe the clear operations. Besides I'm afraid that we will 
>> > need to handle
>> > various corner cases of the OpenGL API. It is doable but likely more 
>> > complicated.
>> >
>> > There is also the Nvidia way, i.e. forces the driver to XInitThreads X11. 
>> > I think it
>> > can be implemented with the help of constructor attribute.
>> >
>> Did you trace the above behaviour? What would happen in the following 
>> scenario:
>>  - There is no link against libGL/libEGL
>>  - User gets the dpy primitive w/o calling XInitThreads
>>  - Then user dlopens libGL/libEGL, which in itself calls XInitThreads
>> At that point it's a bit too late isn't it?
>>
>> -Emil
>
> Hello,
>
> No I didn't trace it.
>
> You right, it is too late. You can/must use LD_PRELOAD to ensure that 
> libGL/libEGL is loaded first.
>
> To be honest, the best solution will be to have a thread safe Xlib. No more 
> hack everywhere.
>
Having a look at Xlib might be good indeed.

Then again, the solution you've proposed looks perfectly reasonable,
IMHO. It handles the problem _now_ and should also work when/if we
address Xlib.
I'll take another look today/tomorrow, but I think the series is
perfectly fine to land as-is.

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

Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe

[Axel Davy]

Hi,

There should be very few X11 calls while rendering (basically only at 
the beginning or end of a frame).


Why not just always run these calls in the main thread (and wait for 
glthread work to finish) ?


That's basically what we do for gallium nine.

Yours,

Axel

On 05/05/2017 17:37, Gregory Hainaut wrote:

Hello Mesa developers,

Following the discussion from
https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html

A check was added to ensure that X11 display can be locked. It should be enough
to ensure thread safety between X11 and glthread.

I also did the check on DRI3 as I'm not 100% sure that it is really thread safe.



v2: based on Nicolai/Matt reviews
Add a check on DRI extension version
Use C comments :)

v3: based on Emil reviews
Split the initial first patch into 3 sub patches dri extension / glx / egl
Improve error message
Improve code readability
Just include libX11 on EGL protected by ifdef

Thanks you for all the review comments.

Best regards,

Gregory Hainaut (4):
   dri: Extend __DRIbackgroundCallableExtensionRec to include a callback
 that checks for thread safety
   glx: implement __DRIbackgroundCallableExtension.isThreadSafe
   egl: implement __DRIbackgroundCallableExtension.isThreadSafe
   glthread/gallium: require safe_glthread to start glthread

  include/GL/internal/dri_interface.h  | 11 +++
  src/egl/drivers/dri2/egl_dri2.c  | 28 +++-
  src/gallium/state_trackers/dri/dri_context.c | 21 +
  src/glx/dri2_glx.c   | 15 ++-
  src/glx/dri3_glx.c   | 15 ++-
  5 files changed, 83 insertions(+), 7 deletions(-)



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

Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe

[gregory hainaut]

On Fri, 5 May 2017 18:17:22 +0100
Emil Velikov  wrote:

> On 5 May 2017 at 17:58, gregory hainaut  wrote:
> > On Fri, 5 May 2017 17:45:01 +0200
> > Axel Davy  wrote:
> >  
>  [...]  
> >
> > Hello Axel,
> >
> > Yes it is another possibility. It would requires to track gl calls that end 
> > up in X11.
> > I'm not sure if there is an easy way to list all those gl functions. There 
> > are at least the
> > draw calls and maybe the clear operations. Besides I'm afraid that we will 
> > need to handle
> > various corner cases of the OpenGL API. It is doable but likely more 
> > complicated.
> >
> > There is also the Nvidia way, i.e. forces the driver to XInitThreads X11. I 
> > think it
> > can be implemented with the help of constructor attribute.
> >  
> Did you trace the above behaviour? What would happen in the following 
> scenario:
>  - There is no link against libGL/libEGL
>  - User gets the dpy primitive w/o calling XInitThreads
>  - Then user dlopens libGL/libEGL, which in itself calls XInitThreads
> At that point it's a bit too late isn't it?
> 
> -Emil

Hello,

No I didn't trace it.

You right, it is too late. You can/must use LD_PRELOAD to ensure that 
libGL/libEGL is loaded first.

To be honest, the best solution will be to have a thread safe Xlib. No more 
hack everywhere.

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

Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe

[Emil Velikov]

On 5 May 2017 at 17:58, gregory hainaut  wrote:
> On Fri, 5 May 2017 17:45:01 +0200
> Axel Davy  wrote:
>
>> Hi,
>>
>> There should be very few X11 calls while rendering (basically only at
>> the beginning or end of a frame).
>>
>> Why not just always run these calls in the main thread (and wait for
>> glthread work to finish) ?
>>
>> That's basically what we do for gallium nine.
>>
>> Yours,
>>
>> Axel
>
> Hello Axel,
>
> Yes it is another possibility. It would requires to track gl calls that end 
> up in X11.
> I'm not sure if there is an easy way to list all those gl functions. There 
> are at least the
> draw calls and maybe the clear operations. Besides I'm afraid that we will 
> need to handle
> various corner cases of the OpenGL API. It is doable but likely more 
> complicated.
>
> There is also the Nvidia way, i.e. forces the driver to XInitThreads X11. I 
> think it
> can be implemented with the help of constructor attribute.
>
Did you trace the above behaviour? What would happen in the following scenario:
 - There is no link against libGL/libEGL
 - User gets the dpy primitive w/o calling XInitThreads
 - Then user dlopens libGL/libEGL, which in itself calls XInitThreads
At that point it's a bit too late isn't it?

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

Re: [Mesa-dev] [PATCH v3 0/4] Disable glthread if libX11 isn't thread-safe

[gregory hainaut]

On Fri, 5 May 2017 17:45:01 +0200
Axel Davy  wrote:

> Hi,
> 
> There should be very few X11 calls while rendering (basically only at 
> the beginning or end of a frame).
> 
> Why not just always run these calls in the main thread (and wait for 
> glthread work to finish) ?
> 
> That's basically what we do for gallium nine.
> 
> Yours,
> 
> Axel

Hello Axel,

Yes it is another possibility. It would requires to track gl calls that end up 
in X11.
I'm not sure if there is an easy way to list all those gl functions. There are 
at least the 
draw calls and maybe the clear operations. Besides I'm afraid that we will need 
to handle
various corner cases of the OpenGL API. It is doable but likely more 
complicated.

There is also the Nvidia way, i.e. forces the driver to XInitThreads X11. I 
think it
can be implemented with the help of constructor attribute.

Cheers,
Gregory

 
> On 05/05/2017 17:37, Gregory Hainaut wrote:
> > Hello Mesa developers,
> >
> > Following the discussion from
> > https://lists.freedesktop.org/archives/mesa-dev/2017-April/153137.html
> >
> > A check was added to ensure that X11 display can be locked. It should be 
> > enough
> > to ensure thread safety between X11 and glthread.
> >
> > I also did the check on DRI3 as I'm not 100% sure that it is really thread 
> > safe.
> >
> > 
> >
> > v2: based on Nicolai/Matt reviews
> > Add a check on DRI extension version
> > Use C comments :)
> >
> > v3: based on Emil reviews
> > Split the initial first patch into 3 sub patches dri extension / glx / egl
> > Improve error message
> > Improve code readability
> > Just include libX11 on EGL protected by ifdef
> >
> > Thanks you for all the review comments.
> >
> > Best regards,
> >
> > Gregory Hainaut (4):
> >dri: Extend __DRIbackgroundCallableExtensionRec to include a callback
> >  that checks for thread safety
> >glx: implement __DRIbackgroundCallableExtension.isThreadSafe
> >egl: implement __DRIbackgroundCallableExtension.isThreadSafe
> >glthread/gallium: require safe_glthread to start glthread
> >
> >   include/GL/internal/dri_interface.h  | 11 +++
> >   src/egl/drivers/dri2/egl_dri2.c  | 28 
> > +++-
> >   src/gallium/state_trackers/dri/dri_context.c | 21 +
> >   src/glx/dri2_glx.c   | 15 ++-
> >   src/glx/dri3_glx.c   | 15 ++-
> >   5 files changed, 83 insertions(+), 7 deletions(-)
> >  
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev