[Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS

[Marathe, Yogesh]

> -----Original Message-----
> From: Tomasz Figa [mailto:tfiga at chromium.org]
> Sent: Friday, August 4, 2017 9:39 PM
> On Sat, Aug 5, 2017 at 12:53 AM, Marathe, Yogesh
> <yogesh.marathe at intel.com> wrote:
> > Tomasz, Emil,
> >
> >> -----Original Message-----
> >> From: Tomasz Figa [mailto:tfiga at chromium.org]
> >> On Fri, Aug 4, 2017 at 9:55 PM, Emil Velikov <emil.l.velikov at 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