On Wed, 13 Feb 2019 at 09:32, Luigi Santivetti <luigi.santive...@imgtec.com> wrote: > > Hello Emil, > > thanks for your feedback, I agree, dri2_make_current() looks complex. > I'll comment inline. > > Emil Velikov <emil.l.veli...@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.engest...@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.engest...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev