[Mesa-dev] [EGL android: accquire fence implementation] i965: Queue the buffer with a sync fence for Android OS (v2)

[Tomasz Figa]

Hi Rafael,

On Sat, Jul 15, 2017 at 1:45 AM, Rafael Antognolli
<rafael.antognolli at intel.com> wrote:
> On Fri, Jul 14, 2017 at 09:13:49AM +0100, Chris Wilson wrote:
>> Quoting Zhongmin Wu (2017-07-14 07:55:45)
[snip]
>> >  extern uint32_t
>> > diff --git a/src/mesa/drivers/dri/i915/intel_screen.c b/src/mesa/drivers/dri/i915/intel_screen.c
>> > index cba5434..38d0e63 100644
>> > --- a/src/mesa/drivers/dri/i915/intel_screen.c
>> > +++ b/src/mesa/drivers/dri/i915/intel_screen.c
>> > @@ -182,6 +182,7 @@ static const struct __DRI2flushExtensionRec intelFlushExtension = {
>> >
>> >      .flush              = intelDRI2Flush,
>> >      .invalidate         = dri2InvalidateDrawable,
>> > +    .get_retrieve_fd    = NULL,
>> >  };
>> >
>> >  static struct intel_image_format intel_image_formats[] = {
>> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> > index 62d2fe8..9813c8c 100644
>> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> > @@ -648,9 +648,25 @@ do_flush_locked(struct brw_context *brw, int in_fence_fd, int *out_fence_fd)
>> >           /* Add the batch itself to the end of the validation list */
>> >           add_exec_bo(batch, batch->bo);
>> >
>> > +         if (brw->driContext->driDrawablePriv &&
>> > +            brw->driContext->driDrawablePriv->retrieve_fd != -1) {
>> > +            close(brw->driContext->driDrawablePriv->retrieve_fd);
>> > +            brw->driContext->driDrawablePriv->retrieve_fd = -1;
>> > +         }
>>
>> No. Use the correct interfaces for creating a fence, stop imposing the
>> cost upon everyone else.
>
> Yeah, that's correct, it definitely shouldn't be inside intel_batchbuffer.
>
> Chris, the main problem that Zhongmin raised was that on
> droid_window_cancel_buffer the context has been previously destroyed already,
> so they can't create a new fence for that old context. I haven't checked this
> myself as I don't have an android build.

Yes, that's a problem. But it should be solved at the source and not
by digging around it. IMHO it's a limitation of EGL DRI2 code and
that's where it should be fixed, by using all the tools already
available (i.e. DRI2 fence extension).

>
> Still, it looks to me that they would need to store the previous fence to
> solve this.

Makes sense.

>
> So, the right place to do so would be inside platform_android.c,
> right? And since I don't see any private struct that could store such fence
> there, one option would be to extend the struct dri2_egl_surface for android,
> adding private data there. Does that make sense?

I'd say these fences are not 100% Android-specific anymore. They are
an upstream Linux feature and can be used on other platforms as well.
I think wiring this properly in EGL DRI2 core would make sense,
especially since this is the place where the context is being
destroyed (platform_android doesn't handle context destruction).

Best regards,
Tomasz