[PATCH xserver] glamor: Fix crash when master gpu is using glamor and another gpu is hotplugged

[Michel Dänzer]

On 08/09/16 03:19 AM, Eric Anholt wrote:
> Hans de Goede <hdegoede at redhat.com> writes:
> 
>> Hi,
>>
>> On 06-09-16 22:22, Eric Anholt wrote:
>>> Hans de Goede <hdegoede at redhat.com> writes:
>>>
>>>> When a GPU gets hotplugged while X is already running, glamor_egl_init()
>>>> gets called and changes the current egl context at a point where glamor
>>>> does not expect this.
>>>>
>>>> This causes glamor to e.g. crash in the next glamor_create_pixmap() call
>>>> (caled through the master's screen->CreatePixmap), note this is not the
>>>> only troublesome entry point I've seen other backtraces when using a
>>>> composting window manager.
>>>>
>>>> Adding glamor_make_current calls to all entry points is quite expensive,
>>>> so this commit consists of a miminal fix for this problem by restoring the
>>>> original egl context when leaving glamor_egl_init() with an error, based
>>>> on only usb gpu's getting hotplugged and they do not support glamor.
>>>
>>> I think the problem is just mismatching our lastGLContext with the
>>> actual makecurrent state.  Couldn't we just use glamor_make_current()
>>> instead of our own eglMakeCurrent() call in this function?
>>
>> We cannot use glamor_make_current() in the problem scenario I'm
>> trying to fix, because we do not need to set the egl context
>> from the current screen, we need to not mess up the egl context
>> which was current *before* glamor_egl_init() gets called for
>> a different screen then the one which owns the current egl context.
>>
>> What happens is:
>>
>> 1.current egl context is the master GPUs / Screens egl context
>> 2. USB GPU gets hotplugged
>> 3. modesetting driver's PreInit() gets called on the
>>     USB-GPU's Screen
>> 4. PreInit() calls glamor_egl_init() which creates a new
>>     egl context for the USB-GPU Screen, makes that current,
>>     then fails and sets the current egl context to NONE
>> 6. master Screen->CreatePixmap gets called, this is
>>     glamor_make_pixmap, this fails (in an assert -> boom)
>>     because the current egl context is NONE
> 
> There's this callchain:
> 
> glamor_make_pixmap() ->
> glamor_create_fbo() ->
> glamor_create_tex() ->
> glamor_make_current()
> 
> And every entrypoint should be doing that before making any other GL
> calls (if there are any missing, they must be fixed).  The problem is
> that glamor_egl_init() is making a GL context current without updating
> lastGLcontext, so glamor_make_current() short-circuits.  Calling
> glamor_make_current() from glamor_egl_init() was my proposal for keeping
> lastGLcontext updated.
> 
> If we can't use that for some reason, we should just NULL out
> lastGLcontext at our custom MakeCurrent time so that it's never out of
> sync with reality.

Seconded.


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20160908/6177244c/attachment.sig>