Tomasz, Emil, > -----Original Message----- > From: Tomasz Figa [mailto:tf...@chromium.org] > Sent: Friday, August 4, 2017 6:54 PM > To: Emil Velikov <emil.l.veli...@gmail.com> > Cc: Marathe, Yogesh <yogesh.mara...@intel.com>; Antognolli, Rafael > <rafael.antogno...@intel.com>; ML mesa-dev <mesa- > d...@lists.freedesktop.org>; Wu, Zhongmin <zhongmin...@intel.com>; Gao, > Shuo <shuo....@intel.com>; Liu, Zhiquan <zhiquan....@intel.com>; Daniel > Stone <dani...@collabora.com>; Timothy Arceri <tarc...@itsqueeze.com>; Eric > Engestrom <e...@engestrom.ch>; Kenneth Graunke <kenn...@whitecape.org>; > Kondapally, Kalyan <kalyan.kondapa...@intel.com>; Varad Gautam > <varad.gau...@collabora.com>; Rainer Hochecker <fernetme...@online.de>; > Nicolai Hähnle <nicolai.haeh...@amd.com> > Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync > fence > for Android OS > > On Fri, Aug 4, 2017 at 9:55 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > >>> >> - version check (2+) the fence extension, calling > >>> >> .create_fence_fd() only when > >>> >> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD > >> > >> The check looks like below now, this is in dri2_surf_update_fence_fd() > >> before > create_fence_fd is called. > >> > >> if (dri2_surf->enable_out_fence && dri2_dpy->fence) { > >> if(__DRI_FENCE_CAP_NATIVE_FD | dri2_dpy->fence- > >get_capabilities(dri2_dpy->dri_screen)) { > >> //create_fence_fd call > >> } > >> } > >> > > Close but no cigar. > > > > if (dri2_surf->enable_out_fence && dri2_dpy->fence && > > dri2_dpy->fence->base.version >= 2 && > > dri2_dpy->fence->get_capabilities) { > > > > if (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen) & > > __DRI_FENCE_CAP_NATIVE_FD) { > > //create_fence_fd call > > } > > } > > If this needs so complicated series of checks, maybe it would make more sense > to just set enable_out_fence based on availability of the capability at > initialization time?
I liked this one compared to nested ifs in dri2_surf_update_fence_fd(). > > > > >> Overall, if I further go ahead and check, actually get_capabilities() > >> ultimately returns based on has_exec_fence which depends on > >> I915_PARAM_HAS_EXEC_FENCE. This is always set to true for i915 in > >> kernel drv unless forced to false!! I'm not sure if that inner check of > get_capabilities still makes sense. Isn't the first one sufficient? > >> > > Not sure what you mean with "first one", but consider the following example: > > - old kernel which does not support (or has force disabled) > > I915_PARAM_HAS_EXEC_FENCE. > > - new userspace which unconditionally advertises the fence v2 > > extension IIRC one may tweak that things to only conditionally > > advertise it, but IMHO it's not worth the hassle. > > > > Even then, Mesa can produce 20 DRI drivers (used by the EGL module) so > > focusing on one doesn't quite work. > > > >>> >> - don't introduce unused variables (in make_current) > >> > >> Done. > >> > >>> >> - the create fd for the old display surface (in make_current) > >>> >> seems bogus > >> > >> Done. > >> > > Did you drop it all together or changed to use some other surface? > > Would be nice to hear the reason why it was added - perhaps I'm > > missing something. > > We have to keep it, otherwise there would be no fence available at the time of > surface destruction, while, at least for Android, a fence can be passed to > window's cancelBuffer callback. > > > > > I think that we want a fence/fd for the new draw surface. Since > > otherwise one won't get created up until the first SwapBuffers call. > > I might be missing something, but wouldn't that insert a fence at the > beginning > of command stream, before even doing anything? At least in Android use cases, > the only places we need the fence is in SwapBuffers and DestroySurface and the > fence should be inserted after all the commands for rendering into given > surface. > Emil, Tomasz sounds convincing to me here, I just went ahead with the comment to try out and flatland worked even after removing that. Zhongmin can explain better but I think in earlier revisions this was done for cancelBuffer to match with queueBuffer, I mean we are passing valid fd for queueBuffer by doing this we would have a valid fd during cancelBuffer. Not sure if this is the reason / one of the reason. I will go ahead with rest of your comments if we are ok to keep fd for old display surface in make_current. > Best regards, > Tomasz _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
