Hi Yogesh, On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh <yogesh.mara...@intel.com> wrote: >> -----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!
The consumer must _always_ wait on the acquire fence if it's a valid FD, as this is how the ANativeWindow interface is defined. You can see Mesa already does it in droid_dequeue_buffer(). If you find a consumer that is not doing so, it's a bug in the consumer. There is no compatibility concern here, as it's strictly regulated by Android specifications. Best regards, Tomasz _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
