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

[Tomasz Figa]

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?

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

Best regards,
Tomasz