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

[Tomasz Figa]

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