[Mesa-dev] [PATCH v2] egl/dri2: try to bind old context if bindContext failed

[Luigi Santivetti]

Hello Emil,

thanks for your feedback, I agree, dri2_make_current() looks complex.
I'll comment inline.

Emil Velikov <emil.l.velikov at gmail.com> writes:

> Hi all,
>
> Haven't looked it this has landed or not.
>
> On Tue, 5 Feb 2019 at 16:41, Eric Engestrom <eric.engestrom at intel.com> wrote:
>>
>> On Friday, 2019-02-01 13:36:27 +0000, Luigi Santivetti wrote:
>> > Before this change, if bindContext() failed then dri2_make_current() would
>> > rebind the old EGL context and surfaces and return EGL_BAD_MATCH. However,
>> > it wouldn't rebind the DRI context and surfaces, thus leaving it in an
>> > inconsistent and unrecoverable state.
>> >
>> > After this change, dri2_make_current() tries to bind the old DRI context
>> > and surfaces when bindContext() failed. If unable to do so, it leaves EGL
>> > and the DRI driver in a consistent state, it reports an error and returns
>> > EGL_BAD_MATCH.
>>
>> Admittedly I don't understand everything in this function, but your
>> patch looks reasonable.
>> Acked-by: Eric Engestrom <eric.engestrom at intel.com>
>>
>> I ran it through our CI and no regression was spotted, so there's
>> that :)
>>
>> If Emil doesn't raise any concern by the end of the week, I'll push
>> your patch.
>>
>
> My biggest concern, which is unrelated to this patch. As Eric's alluded:
>
> As-is the function is fairly confusing and convoluted, hence why I did
> not really get to looking at the patch earlier.
> I've tried to untangle it with
> 675719817e7bf7c5b9da22c02252aca77a41338d, although it did not cover
> all cases.
>
> No doubt Luigi spend some time trying to get this right, yet making it
> is even harder to follow.

Having spent some time on it and with the present design of
dri2_make_current(), I don't think this change can address issues
other than the DRI bindContext().

> Can we try simplifying things up?
>

Are you suggesting to split the work in refactoring first and then
re-implementing this change on top it? If so, could you suggest what you
believe is to be improved?

Some weak points I found are:
1. Convoluted control flow, due to many if/else
2. Variable naming, it is questionable: the prefix "tmp_" used in the
asserts only, "cctx" and "ctx" respectively for DRI and EGL.

> -Emil

Thanks,
Luigi