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. > -Emil Thanks, Luigi _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
