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

[Hans de Goede]

Hi,

On 07-09-16 20:19, 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.

Ah, I see. I'll give this a try with my crash reproducer and post
a new version.

Regards,

Hans