> -----Original Message----- > From: Tomasz Figa [mailto:tf...@chromium.org] > Sent: Friday, August 4, 2017 9:39 PM > On Sat, Aug 5, 2017 at 12:53 AM, Marathe, Yogesh > <yogesh.mara...@intel.com> wrote: > > Tomasz, Emil, > > > >> -----Original Message----- > >> From: Tomasz Figa [mailto:tf...@chromium.org] > >> 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. > > My understanding is that nobody actually cares about the fence that > cancelBuffer returns, because the contents of the buffer are going to be > discarded anyway and the buffer doesn't go to the consumer (e.g. > flatland code that reads the timestamp). I even suspect that typically > destroySurface would be called directly after swapBuffers and the surface > wouldn't have a buffer to cancel. You can easily check this by adding a print > before cancelBuffer call happens. So we might actually be fine with simpler > code > that gets fence only for swapBuffers. >
Sure. I can confirm this. > Changing the topic, the patch doesn't seem to change the implementation of > swapBuffers to stop doing a flush on the buffer, which defeats the purpose of > the fence, as the it is likely already signaled at the time it is passed to > queueBuffer. Shouldn't we fix this? > I have been wondering about it all the while, when I had prints in Fence::getSignalTime() to check finfo->status from consumer side during initial revisions, I always found it to be signaled! Can we really remove that flush in swapBuffers? In that case I believe the consumer _must_ wait on fence before really accessing it, so that would trigger a change in buffer consumer / application! > Best regards, > Tomasz _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
