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

[Tomasz Figa]

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]
>> Sent: Friday, August 4, 2017 6:54 PM
>> To: Emil Velikov <emil.l.velikov at gmail.com>
>> Cc: Marathe, Yogesh <yogesh.marathe at intel.com>; Antognolli, Rafael
>> <rafael.antognolli at intel.com>; ML mesa-dev <mesa-
>> dev at lists.freedesktop.org>; Wu, Zhongmin <zhongmin.wu at intel.com>; Gao,
>> Shuo <shuo.gao at intel.com>; Liu, Zhiquan <zhiquan.liu at intel.com>; Daniel
>> Stone <daniels at collabora.com>; Timothy Arceri <tarceri at itsqueeze.com>; Eric
>> Engestrom <eric at engestrom.ch>; Kenneth Graunke <kenneth at whitecape.org>;
>> Kondapally, Kalyan <kalyan.kondapally at intel.com>; Varad Gautam
>> <varad.gautam at collabora.com>; Rainer Hochecker <fernetmenta at online.de>;
>> Nicolai Hähnle <nicolai.haehnle at amd.com>
>> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence
>> for Android OS
>>
>> 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.

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?

Best regards,
Tomasz