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

[Tomasz Figa]

On Wed, Aug 9, 2017 at 3:17 PM, Marathe, Yogesh
<yogesh.marathe at intel.com> wrote:
> 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,

We're not, I've never suggested that we should remove it. The only
thing I mentioned is that it probably doesn't have too much effect.
Emil has suggested removing it, but it was due to a misunderstanding
of Android requirements, which he later corrected.

> finally do you want to put the
> check back for old display surface? v6 that I pushed yesterday doesn’t have it!

Note that my reply predates your v6. Also, I'm not the only Android
person here and it would be good to let some other people comment
before doing some changes.

Best regards,
Tomasz