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

[Marathe, Yogesh]

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.

> Best regards,
> Tomasz