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

[Marathe, Yogesh]

Tomasz,

> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On Behalf
> Of Tomasz Figa
> Sent: Tuesday, August 8, 2017 7:43 AM
> To: Marathe, Yogesh <yogesh.marathe at intel.com>
> On Mon, Aug 7, 2017 at 2:19 PM, Marathe, Yogesh
> <yogesh.marathe at intel.com> wrote:
> > 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.
> 
> I think it would make sense to behave consistently in both
> queueBuffer() and cancelBuffer(), especially since Zhongmin's patch (based on
> my suggestions) had that already implemented. There might be also some
> performance benefits if we could call cancelBuffer() with a fence, without doing
> a flush on Mesa side.
> 

Sorry I'm confused, we are moving back and forth, finally do you want to put the
check back for old display surface? v6 that I pushed yesterday doesn’t have it!

> Best regards,
> Tomasz
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev