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

[Emil Velikov]

On Wed, 13 Feb 2019 at 09:32, Luigi Santivetti
<luigi.santivetti at imgtec.com> wrote:
>
> 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.
>
Off the top of my head:
Patch 1: factor the error handling outside of _eglBindContext() and
use only as needed
Patch 2: fold the separate old_ctx hunks - glFlush, old_dpy, unbindContext
Patch 3: do not conflate the unbind with the bindContext() failure -
perhaps try something like my earlier commit

Can you give that a stab please?

FWIW bindContext() cannot fail for the in-tree drivers, since the only
probable error case has been handled further up in the EGL stack.
Or at least, the drivers lack actual error handling/reporting - but
that's a topic for another day :-P

-Emil