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

[Marathe, Yogesh]

Tomasz,

> -----Original Message-----
> From: Tomasz Figa [mailto:tfiga at chromium.org]
> Sent: Saturday, August 5, 2017 8:47 AM
> 
> Hi Yogesh,
> 
> On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh <yogesh.marathe at intel.com>
> wrote:
> >> -----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.

As read more, yes, cancelled buffer doesn't go to consumer although it might get
reused and it seems it won't be overwritten until the fence is signaled.
destroySurface is called in the end in tearDown and we can still live without the
code in discussion because second part of comment that describes cancelBuffer.

    // cancelBuffer returns a dequeued buffer to the BufferQueue, but doesn't
    // queue it for use by the consumer.
    //
    // The buffer will not be overwritten until the fence signals.  The fence
    // will usually be the one obtained from dequeueBuffer.
    virtual status_t cancelBuffer(int slot, const sp<Fence>& fence);
  
We definitely don’t have to create a new fence for old surface. In case of buffer reuse,
we anyway create a new fence during swap which should be sufficient.

> >
> >> 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.

I checked this, yes, BufferConsumer waits on fence provided its valid. 

> 
> Best regards,
> Tomasz