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

[Marathe, Yogesh]

Tomasz,

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Thursday, August 10, 2017 5:29 PM
> To: Marathe, Yogesh 
> Hi Yogesh,
> 
> On Thu, Aug 10, 2017 at 5:25 PM, Marathe, Yogesh
>  wrote:
> > Hi Tomasz,
> >
> >> -Original Message-
> >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On
> >> Behalf Of Tomasz Figa
> >> Sent: Tuesday, August 8, 2017 7:45 AM
> >> To: Marathe, Yogesh 
> >> >> > >> 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.
> >> >>
> >> >> I checked this, yes, BufferConsumer waits on fence provided its valid.
> >> >
> >> > Hi Tomasz,
> >> >
> >> > Is it ok to move that 'flush' removal change to separate commit? I
> >> > would opt for that. This gflush removal change is going to trigger
> >> > additional tests, while this one fixes the issue for now and has
> >> > list of review comments done. If this is fine, I'll push v6 for this.
> >>
> >> I'm okay with either.
> >>
> >
> > I found GLConsumer aosp has glFlush() is already, it means we had two
> > flush calls in the path, one in swapBuffers and other in libgui on consumer.
> >
> > I went ahead and removed  dri2_flush_drawable_for_swapbuffer.
> > Functionally, things seem to be ok. I assume this will be valid only
> > for android with valid fence changes and not for other platforms. Is this 
> > right
> expectation? Diff below.
> >
> > diff --git a/src/egl/drivers/dri2/platform_android.c
> > b/src/egl/drivers/dri2/platform_android.c
> > index 8bca753..80da021 100644
> > --- a/src/egl/drivers/dri2/platform_android.c
> > +++ b/src/egl/drivers/dri2/platform_android.c
> > @@ -706,7 +706,6 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay
> *disp, _EGLSurface *draw)
> > if (dri2_surf->back)
> >dri2_surf->back->age = 1;
> >
> > -   dri2_flush_drawable_for_swapbuffers(disp, draw);
> >
> > /* dri2_surf->buffer can be null even when no error has occured. For
> >  * example, if the user has called no GL rendering commands since
> > the
> >
> > If this is only change, I don’t think we need separate patch here. Please 
> > correct
> me if I'm wrong.
> 
> I think I have been mistaken in what
> dri2_flush_drawable_for_swabuffers() does. We need to keep it there as it
> makes the DRI2 driver issue operations finalizing the buffer, e.g.
> remaining drawing or a multisample resolve if necessary. However it doesn't do
> any synchronous wait on the queued operations, so there is no performance
> loss.
> 

I am OK either ways. Another thing, glFlush in libgui seems legitimate, it is
required in order to have eglDupNativeFenceFDANDROID() return a valid fd!

Now only pending thing is the 'old display surface' fence then, and we have 
decided
to wait for someone else from android to comment on it.

> Best regards,
> Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Tomasz Figa]

Hi Yogesh,

On Thu, Aug 10, 2017 at 5:25 PM, Marathe, Yogesh
 wrote:
> Hi Tomasz,
>
>> -Original Message-
>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
>> Of Tomasz Figa
>> Sent: Tuesday, August 8, 2017 7:45 AM
>> To: Marathe, Yogesh 
>> >> > >> 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.
>> >>
>> >> I checked this, yes, BufferConsumer waits on fence provided its valid.
>> >
>> > Hi Tomasz,
>> >
>> > Is it ok to move that 'flush' removal change to separate commit? I
>> > would opt for that. This gflush removal change is going to trigger
>> > additional tests, while this one fixes the issue for now and has list
>> > of review comments done. If this is fine, I'll push v6 for this.
>>
>> I'm okay with either.
>>
>
> I found GLConsumer aosp has glFlush() is already, it means we had two flush 
> calls in
> the path, one in swapBuffers and other in libgui on consumer.
>
> I went ahead and removed  dri2_flush_drawable_for_swapbuffer. Functionally,
> things seem to be ok. I assume this will be valid only for android with valid 
> fence changes
> and not for other platforms. Is this right expectation? Diff below.
>
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index 8bca753..80da021 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -706,7 +706,6 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, 
> _EGLSurface *draw)
> if (dri2_surf->back)
>dri2_surf->back->age = 1;
>
> -   dri2_flush_drawable_for_swapbuffers(disp, draw);
>
> /* dri2_surf->buffer can be null even when no error has occured. For
>  * example, if the user has called no GL rendering commands since the
>
> If this is only change, I don’t think we need separate patch here. Please 
> correct me if I'm wrong.

I think I have been mistaken in what
dri2_flush_drawable_for_swabuffers() does. We need to keep it there as
it makes the DRI2 driver issue operations finalizing the buffer, e.g.
remaining drawing or a multisample resolve if necessary. However it
doesn't do any synchronous wait on the queued operations, so there is
no performance loss.

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marathe, Yogesh]

Hi Tomasz, 

> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Tomasz Figa
> Sent: Tuesday, August 8, 2017 7:45 AM
> To: Marathe, Yogesh 
> >> > >> 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.
> >>
> >> I checked this, yes, BufferConsumer waits on fence provided its valid.
> >
> > Hi Tomasz,
> >
> > Is it ok to move that 'flush' removal change to separate commit? I
> > would opt for that. This gflush removal change is going to trigger
> > additional tests, while this one fixes the issue for now and has list
> > of review comments done. If this is fine, I'll push v6 for this.
> 
> I'm okay with either.
> 

I found GLConsumer aosp has glFlush() is already, it means we had two flush 
calls in
the path, one in swapBuffers and other in libgui on consumer.

I went ahead and removed  dri2_flush_drawable_for_swapbuffer. Functionally,
things seem to be ok. I assume this will be valid only for android with valid 
fence changes
and not for other platforms. Is this right expectation? Diff below.

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 8bca753..80da021 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -706,7 +706,6 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *draw)
if (dri2_surf->back)
   dri2_surf->back->age = 1;

-   dri2_flush_drawable_for_swapbuffers(disp, draw);

/* dri2_surf->buffer can be null even when no error has occured. For
 * example, if the user has called no GL rendering commands since the

If this is only change, I don’t think we need separate patch here. Please 
correct me if I'm wrong.

> Best regards,
> Tomasz
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Tomasz Figa]

On Wed, Aug 9, 2017 at 3:17 PM, Marathe, Yogesh
 wrote:
> Tomasz,
>
>> -Original Message-
>> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
>> Of Tomasz Figa
>> Sent: Tuesday, August 8, 2017 7:43 AM
>> To: Marathe, Yogesh 
>> On Mon, Aug 7, 2017 at 2:19 PM, Marathe, Yogesh
>>  wrote:
>> > Tomasz,
>> >
>> >> -Original Message-
>> >> From: Tomasz Figa [mailto:tf...@chromium.org]
>> >> Sent: Saturday, August 5, 2017 8:47 AM
>> >>
>> >> Hi Yogesh,
>> >>
>> >> On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh
>> >> 
>> >> wrote:
>> >> >> -Original Message-
>> >> >> From: Tomasz Figa [mailto:tf...@chromium.org]
>> >> >> Sent: Friday, August 4, 2017 9:39 PM On Sat, Aug 5, 2017 at 12:53
>> >> >> AM, Marathe, Yogesh  wrote:
>> >> >> > Tomasz, Emil,
>> >> >> >
>> >> >> >> -Original Message-
>> >> >> >> From: Tomasz Figa [mailto:tf...@chromium.org] On Fri, Aug 4,
>> >> >> >> 2017 at 9:55 PM, Emil Velikov 
>> >> >> 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.
>> >> >> >>
>> >> 

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

[Marathe, Yogesh]

Tomasz,

> -Original Message-
> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf
> Of Tomasz Figa
> Sent: Tuesday, August 8, 2017 7:43 AM
> To: Marathe, Yogesh 
> On Mon, Aug 7, 2017 at 2:19 PM, Marathe, Yogesh
>  wrote:
> > Tomasz,
> >
> >> -Original Message-
> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> Sent: Saturday, August 5, 2017 8:47 AM
> >>
> >> Hi Yogesh,
> >>
> >> On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh
> >> 
> >> wrote:
> >> >> -Original Message-
> >> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> >> Sent: Friday, August 4, 2017 9:39 PM On Sat, Aug 5, 2017 at 12:53
> >> >> AM, Marathe, Yogesh  wrote:
> >> >> > Tomasz, Emil,
> >> >> >
> >> >> >> -Original Message-
> >> >> >> From: Tomasz Figa [mailto:tf...@chromium.org] On Fri, Aug 4,
> >> >> >> 2017 at 9:55 PM, Emil Velikov 
> >> >> 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 

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

[Tomasz Figa]

On Mon, Aug 7, 2017 at 2:19 PM, Marathe, Yogesh
 wrote:
> Tomasz,
>
>> -Original Message-
>> From: Tomasz Figa [mailto:tf...@chromium.org]
>> Sent: Saturday, August 5, 2017 8:47 AM
>>
>> Hi Yogesh,
>>
>> On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh 
>> wrote:
>> >> -Original Message-
>> >> From: Tomasz Figa [mailto:tf...@chromium.org]
>> >> Sent: Friday, August 4, 2017 9:39 PM
>> >> On Sat, Aug 5, 2017 at 12:53 AM, Marathe, Yogesh
>> >>  wrote:
>> >> > Tomasz, Emil,
>> >> >
>> >> >> -Original Message-
>> >> >> From: Tomasz Figa [mailto:tf...@chromium.org] On Fri, Aug 4, 2017
>> >> >> at 9:55 PM, Emil Velikov 
>> >> 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 

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

[Tomasz Figa]

On Mon, Aug 7, 2017 at 3:44 PM, Marathe, Yogesh
 wrote:
>> -Original Message-
>>
>> Tomasz,
>>
>> > -Original Message-
>> > From: Tomasz Figa [mailto:tf...@chromium.org]
>> > Sent: Saturday, August 5, 2017 8:47 AM
>> >
>> > Hi Yogesh,
>> >
>> > On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh
>> > 
>> > wrote:
>> > >> -Original Message-
>> > >> From: Tomasz Figa [mailto:tf...@chromium.org]
>> > >> Sent: Friday, August 4, 2017 9:39 PM On Sat, Aug 5, 2017 at 12:53
>> > >> AM, Marathe, Yogesh  wrote:
>> > >> > Tomasz, Emil,
>> > >> >
>> > >> >> -Original Message-
>> > >> >> From: Tomasz Figa [mailto:tf...@chromium.org] On Fri, Aug 4,
>> > >> >> 2017 at 9:55 PM, Emil Velikov 
>> > >> 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 

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

[Marathe, Yogesh]

> -Original Message-
> 
> Tomasz,
> 
> > -Original Message-
> > From: Tomasz Figa [mailto:tf...@chromium.org]
> > Sent: Saturday, August 5, 2017 8:47 AM
> >
> > Hi Yogesh,
> >
> > On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh
> > 
> > wrote:
> > >> -Original Message-
> > >> From: Tomasz Figa [mailto:tf...@chromium.org]
> > >> Sent: Friday, August 4, 2017 9:39 PM On Sat, Aug 5, 2017 at 12:53
> > >> AM, Marathe, Yogesh  wrote:
> > >> > Tomasz, Emil,
> > >> >
> > >> >> -Original Message-
> > >> >> From: Tomasz Figa [mailto:tf...@chromium.org] On Fri, Aug 4,
> > >> >> 2017 at 9:55 PM, Emil Velikov 
> > >> 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 

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

[Marathe, Yogesh]

Tomasz,

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Saturday, August 5, 2017 8:47 AM
> 
> Hi Yogesh,
> 
> On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh 
> wrote:
> >> -Original Message-
> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> Sent: Friday, August 4, 2017 9:39 PM
> >> On Sat, Aug 5, 2017 at 12:53 AM, Marathe, Yogesh
> >>  wrote:
> >> > Tomasz, Emil,
> >> >
> >> >> -Original Message-
> >> >> From: Tomasz Figa [mailto:tf...@chromium.org] On Fri, Aug 4, 2017
> >> >> at 9:55 PM, Emil Velikov 
> >> 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 

Re: [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
 wrote:
>> -Original Message-
>> From: Tomasz Figa [mailto:tf...@chromium.org]
>> Sent: Friday, August 4, 2017 9:39 PM
>> On Sat, Aug 5, 2017 at 12:53 AM, Marathe, Yogesh
>>  wrote:
>> > Tomasz, Emil,
>> >
>> >> -Original Message-
>> >> From: Tomasz Figa [mailto:tf...@chromium.org]
>> >> On Fri, Aug 4, 2017 at 9:55 PM, Emil Velikov 
>> 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 

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

[Emil Velikov]

On 4 August 2017 at 14:23, Tomasz Figa  wrote:

>
> 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?
>
Either way is fine with me.

>> 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.
>
Thanks for the correction. You're absolutely correct in both cases.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marathe, Yogesh]

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Friday, August 4, 2017 9:39 PM
> On Sat, Aug 5, 2017 at 12:53 AM, Marathe, Yogesh
>  wrote:
> > Tomasz, Emil,
> >
> >> -Original Message-
> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> On Fri, Aug 4, 2017 at 9:55 PM, Emil Velikov 
> 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 

Re: [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
 wrote:
> Tomasz, Emil,
>
>> -Original Message-
>> From: Tomasz Figa [mailto:tf...@chromium.org]
>> Sent: Friday, August 4, 2017 6:54 PM
>> To: Emil Velikov 
>> Cc: Marathe, Yogesh ; Antognolli, Rafael
>> ; ML mesa-dev > d...@lists.freedesktop.org>; Wu, Zhongmin ; Gao,
>> Shuo ; Liu, Zhiquan ; Daniel
>> Stone ; Timothy Arceri ; Eric
>> Engestrom ; Kenneth Graunke ;
>> Kondapally, Kalyan ; Varad Gautam
>> ; Rainer Hochecker ;
>> Nicolai Hähnle 
>> 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  
>> 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

Re: [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:tf...@chromium.org]
> Sent: Friday, August 4, 2017 6:54 PM
> To: Emil Velikov 
> Cc: Marathe, Yogesh ; Antognolli, Rafael
> ; ML mesa-dev  d...@lists.freedesktop.org>; Wu, Zhongmin ; Gao,
> Shuo ; Liu, Zhiquan ; Daniel
> Stone ; Timothy Arceri ; Eric
> Engestrom ; Kenneth Graunke ;
> Kondapally, Kalyan ; Varad Gautam
> ; Rainer Hochecker ;
> Nicolai Hähnle 
> 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  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 12:21 PM, Marathe, Yogesh
 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)) {
>>
>> This doesn't make any sense, because non-zero OR whatever is always true. Did
>> you by any chance meant to use AND instead? Also please just extend the
>> condition of the first if, instead of nesting another under it for no reason.
>>
>
> Right. It must be '&', thanks for pointing out.
>
> On the nesting, I want to check dri2_dpy->fence is valid first before 
> dri2_dpy->fence->(anything())
> can be called, so I believe that nesting can still be there. Rafael had that 
> review comment.
> Do you still want to combine conditions in a single 'if'?

I'm not sure what's the problem with single if. The order of
evaluating conditions is strictly defined by C standard.

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 4 August 2017 at 04:21, Marathe, Yogesh  wrote:

>> > 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)) {
>>
>> This doesn't make any sense, because non-zero OR whatever is always true. Did
>> you by any chance meant to use AND instead? Also please just extend the
>> condition of the first if, instead of nesting another under it for no reason.
>>
>
> Right. It must be '&', thanks for pointing out.
>
> On the nesting, I want to check dri2_dpy->fence is valid first before 
> dri2_dpy->fence->(anything())
> can be called, so I believe that nesting can still be there. Rafael had that 
> review comment.
> Do you still want to combine conditions in a single 'if'?
>
In my example (just a second ago) I've kept the conditionals separate
since they're quite large.
Squashing them in a single if () block is fine with me.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 3 August 2017 at 17:55, Marathe, Yogesh  wrote:
> Adding folks who were CCed for earlier versions.
>
> Hi Emil, few doubts and comments below.
>
>> -Original Message-
>> From: Tomasz Figa [mailto:tf...@chromium.org]
>> Sent: Thursday, August 3, 2017 7:19 PM
>> To: Marathe, Yogesh 
>> Cc: Emil Velikov ; Antognolli, Rafael
>> ; ML mesa-dev > d...@lists.freedesktop.org>; Wu, Zhongmin 
>> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync 
>> fence
>> for Android OS
>>
>> On Thu, Aug 3, 2017 at 10:11 PM, Marathe, Yogesh
>>  wrote:
>> > Emil,
>> >
>> >> -Original Message-
>> >> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
>> >> Sent: Thursday, August 3, 2017 4:06 PM
>> >> To: Marathe, Yogesh 
>> >> Cc: ML mesa-dev ; Wu, Zhongmin
>> >> 
>> >> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a
>> >> sync fence for Android OS
>> >>
>> >> On 2 August 2017 at 20:01,   wrote:
>> >> > From: Zhongmin Wu 
>> >> >
>> >> > Before we queued the buffer with a invalid fence (-1), it causes
>> >> > benchmarks such as flatland to fail. This patch enables explicit
>> >> > sync feature on android.
>> >> >
>> >> > Now we get the out fence during the flushing buffer and then pass
>> >> > it to SurfaceFlinger in eglSwapbuffer function.
>> >> >
>> >> > v2: a) Also implement the fence in cancelBuffer.
>> >> > b) The last sync fence is stored in drawable object
>> >> >rather than brw context.
>> >> > c) format clear.
>> >> >
>> >> > v3: a) Save the last fence fd in DRI Context object.
>> >> > b) Return the last fence if the batch buffer is empty and
>> >> >nothing to be flushed when _intel_batchbuffer_flush_fence
>> >> > c) Add the new interface in vbtl to set the retrieve fence
>> >> >
>> >> > v3.1 a) close fd in the new vbtl interface on none Android platform
>> >> >
>> >> > v4: a) The last fence is saved in brw context.
>> >> > b) The retrieve fd is for all the platform but not just Android
>> >> > c) Add a uniform dri2 interface to initialize the surface.
>> >> >
>> >> > v4.1: a) make some changes of variable name.
>> >> >   b) the patch is broken into two patches.
>> >> >
>> >> > v4.2: a) Add a deinit interface for surface to clear the out fence
>> >> >
>> >> > v5: a) Add enable_out_fence to init, platform sets it true or
>> >> >false
>> >> > b) Change get fd to update fd and check for fence
>> >> > c) Commit description updated
>> >> >
>> >> Seems like most suggestions in last version did not get addressed.
>> >> Please comment if you disagree (it can be that we've
>> >> misread/misremember
>> >> something) before posting another version.
>> >>
>> >> To reiterate:
>> >>  - add blank line between variable declarations and code
>
> Done.
>
>> >>  - use more consistent function names
>
> Do you want any specific functioned to be renamed?
>
s/dri2_surf_init/dri2_init_surface/
s/dri2_surf_deinit/dri2_fini_surface/
s/dri2_surface_set_out_fence/dri2_surface_set_out_fence_fd/


>> >>  - comment above queueBuffer needs fixing
>
> It reads as below now.
>/* Queue the buffer with stored out fence fd. The ANativeWindow or buffer
> * consumer may choose to wait for the fence to signal before accessing
> * it. If fence fd value is -1, buffer can be accessed by consumer
> * immediately. Consumer or application shouldn't rely on timestamp
> * associated with fence if the fence fd is -1.
> *
> * Ownership of fd is transferred to consumer after queueBuffer and the
> * consumer is responsible for closing it. Caller must not use the fd
> * after passing it to queueBuffer.
> */
>
> Looks ok?
>
A lot better than I was thinking.

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

> 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 

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

[Marathe, Yogesh]

Tomasz,

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Friday, August 4, 2017 6:52 AM
> To: Marathe, Yogesh 
> Cc: Emil Velikov ; Gao, Shuo
> ; Liu, Zhiquan ; Daniel Stone
> ; Nicolai Hähnle ;
> Antognolli, Rafael ; Eric Engestrom
> ; Wu, Zhongmin ; Kenneth
> Graunke ; Kondapally, Kalyan
> ; Rainer Hochecker ;
> ML mesa-dev ; Timothy Arceri
> ; Varad Gautam 
> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync 
> fence
> for Android OS
> 
> Hi Yogesh,
> 
> On Fri, Aug 4, 2017 at 1:55 AM, Marathe, Yogesh 
> wrote:
> > Adding folks who were CCed for earlier versions.
> >
> > Hi Emil, few doubts and comments below.
> >
> >> -Original Message-
> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> Sent: Thursday, August 3, 2017 7:19 PM
> >> To: Marathe, Yogesh 
> >> Cc: Emil Velikov ; Antognolli, Rafael
> >> ; ML mesa-dev  >> d...@lists.freedesktop.org>; Wu, Zhongmin 
> >> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a
> >> sync fence for Android OS
> >>
> >> On Thu, Aug 3, 2017 at 10:11 PM, Marathe, Yogesh
> >>  wrote:
> >> > Emil,
> >> >
> >> >> -Original Message-
> >> >> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> >> >> Sent: Thursday, August 3, 2017 4:06 PM
> >> >> To: Marathe, Yogesh 
> >> >> Cc: ML mesa-dev ; Wu, Zhongmin
> >> >> 
> >> >> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with
> >> >> a sync fence for Android OS
> >> >>
> >> >> On 2 August 2017 at 20:01,   wrote:
> >> >> > From: Zhongmin Wu 
> >> >> >
> >> >> > Before we queued the buffer with a invalid fence (-1), it causes
> >> >> > benchmarks such as flatland to fail. This patch enables explicit
> >> >> > sync feature on android.
> >> >> >
> >> >> > Now we get the out fence during the flushing buffer and then
> >> >> > pass it to SurfaceFlinger in eglSwapbuffer function.
> >> >> >
> >> >> > v2: a) Also implement the fence in cancelBuffer.
> >> >> > b) The last sync fence is stored in drawable object
> >> >> >rather than brw context.
> >> >> > c) format clear.
> >> >> >
> >> >> > v3: a) Save the last fence fd in DRI Context object.
> >> >> > b) Return the last fence if the batch buffer is empty and
> >> >> >nothing to be flushed when _intel_batchbuffer_flush_fence
> >> >> > c) Add the new interface in vbtl to set the retrieve fence
> >> >> >
> >> >> > v3.1 a) close fd in the new vbtl interface on none Android
> >> >> > platform
> >> >> >
> >> >> > v4: a) The last fence is saved in brw context.
> >> >> > b) The retrieve fd is for all the platform but not just Android
> >> >> > c) Add a uniform dri2 interface to initialize the surface.
> >> >> >
> >> >> > v4.1: a) make some changes of variable name.
> >> >> >   b) the patch is broken into two patches.
> >> >> >
> >> >> > v4.2: a) Add a deinit interface for surface to clear the out
> >> >> > fence
> >> >> >
> >> >> > v5: a) Add enable_out_fence to init, platform sets it true or
> >> >> >false
> >> >> > b) Change get fd to update fd and check for fence
> >> >> > c) Commit description updated
> >> >> >
> >> >> Seems like most suggestions in last version did not get addressed.
> >> >> Please comment if you disagree (it can be that we've
> >> >> misread/misremember
> >> >> something) before posting another version.
> >> >>
> >> >> To reiterate:
> >> >>  - add blank line between variable declarations and code
> >
> > Done.
> >
> >> >>  - use more consistent function names
> >
> > Do you want any specific functioned to be renamed?
> >
> >> >>  - comment above queueBuffer needs fixing
> >
> > It reads as below now.
> >/* Queue the buffer with stored out fence fd. The ANativeWindow or buffer
> > * consumer may choose to wait for the fence to signal before accessing
> > * it. If fence fd value is -1, buffer can be accessed by consumer
> > * immediately. Consumer or application shouldn't rely on timestamp
> > * associated with fence if the fence fd is -1.
> > *
> > * Ownership of fd is transferred to consumer after queueBuffer and the
> > * consumer is responsible for closing it. Caller must not use the fd
> > * after passing it to queueBuffer.
> > */
> >
> > Looks ok?
> 
> Sounds okay to me, but let's wait for Emil's ack.
> 
> >
> 

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

[Tomasz Figa]

Hi Yogesh,

On Fri, Aug 4, 2017 at 1:55 AM, Marathe, Yogesh
 wrote:
> Adding folks who were CCed for earlier versions.
>
> Hi Emil, few doubts and comments below.
>
>> -Original Message-
>> From: Tomasz Figa [mailto:tf...@chromium.org]
>> Sent: Thursday, August 3, 2017 7:19 PM
>> To: Marathe, Yogesh 
>> Cc: Emil Velikov ; Antognolli, Rafael
>> ; ML mesa-dev > d...@lists.freedesktop.org>; Wu, Zhongmin 
>> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync 
>> fence
>> for Android OS
>>
>> On Thu, Aug 3, 2017 at 10:11 PM, Marathe, Yogesh
>>  wrote:
>> > Emil,
>> >
>> >> -Original Message-
>> >> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
>> >> Sent: Thursday, August 3, 2017 4:06 PM
>> >> To: Marathe, Yogesh 
>> >> Cc: ML mesa-dev ; Wu, Zhongmin
>> >> 
>> >> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a
>> >> sync fence for Android OS
>> >>
>> >> On 2 August 2017 at 20:01,   wrote:
>> >> > From: Zhongmin Wu 
>> >> >
>> >> > Before we queued the buffer with a invalid fence (-1), it causes
>> >> > benchmarks such as flatland to fail. This patch enables explicit
>> >> > sync feature on android.
>> >> >
>> >> > Now we get the out fence during the flushing buffer and then pass
>> >> > it to SurfaceFlinger in eglSwapbuffer function.
>> >> >
>> >> > v2: a) Also implement the fence in cancelBuffer.
>> >> > b) The last sync fence is stored in drawable object
>> >> >rather than brw context.
>> >> > c) format clear.
>> >> >
>> >> > v3: a) Save the last fence fd in DRI Context object.
>> >> > b) Return the last fence if the batch buffer is empty and
>> >> >nothing to be flushed when _intel_batchbuffer_flush_fence
>> >> > c) Add the new interface in vbtl to set the retrieve fence
>> >> >
>> >> > v3.1 a) close fd in the new vbtl interface on none Android platform
>> >> >
>> >> > v4: a) The last fence is saved in brw context.
>> >> > b) The retrieve fd is for all the platform but not just Android
>> >> > c) Add a uniform dri2 interface to initialize the surface.
>> >> >
>> >> > v4.1: a) make some changes of variable name.
>> >> >   b) the patch is broken into two patches.
>> >> >
>> >> > v4.2: a) Add a deinit interface for surface to clear the out fence
>> >> >
>> >> > v5: a) Add enable_out_fence to init, platform sets it true or
>> >> >false
>> >> > b) Change get fd to update fd and check for fence
>> >> > c) Commit description updated
>> >> >
>> >> Seems like most suggestions in last version did not get addressed.
>> >> Please comment if you disagree (it can be that we've
>> >> misread/misremember
>> >> something) before posting another version.
>> >>
>> >> To reiterate:
>> >>  - add blank line between variable declarations and code
>
> Done.
>
>> >>  - use more consistent function names
>
> Do you want any specific functioned to be renamed?
>
>> >>  - comment above queueBuffer needs fixing
>
> It reads as below now.
>/* Queue the buffer with stored out fence fd. The ANativeWindow or buffer
> * consumer may choose to wait for the fence to signal before accessing
> * it. If fence fd value is -1, buffer can be accessed by consumer
> * immediately. Consumer or application shouldn't rely on timestamp
> * associated with fence if the fence fd is -1.
> *
> * Ownership of fd is transferred to consumer after queueBuffer and the
> * consumer is responsible for closing it. Caller must not use the fd
> * after passing it to queueBuffer.
> */
>
> Looks ok?

Sounds okay to me, but let's wait for Emil's ack.

>
>> >>  - 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)) {

This doesn't make any sense, because non-zero OR whatever is always
true. Did you by any chance meant to use AND instead? Also please just
extend the condition of the first if, instead of nesting another under
it for no reason.

>   //create_fence_fd call
>
> }
>
> 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?

i965 is not the only driver in Mesa...

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

[Marathe, Yogesh]

Adding folks who were CCed for earlier versions.

Hi Emil, few doubts and comments below.

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Thursday, August 3, 2017 7:19 PM
> To: Marathe, Yogesh 
> Cc: Emil Velikov ; Antognolli, Rafael
> ; ML mesa-dev  d...@lists.freedesktop.org>; Wu, Zhongmin 
> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync 
> fence
> for Android OS
> 
> On Thu, Aug 3, 2017 at 10:11 PM, Marathe, Yogesh
>  wrote:
> > Emil,
> >
> >> -Original Message-
> >> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> >> Sent: Thursday, August 3, 2017 4:06 PM
> >> To: Marathe, Yogesh 
> >> Cc: ML mesa-dev ; Wu, Zhongmin
> >> 
> >> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a
> >> sync fence for Android OS
> >>
> >> On 2 August 2017 at 20:01,   wrote:
> >> > From: Zhongmin Wu 
> >> >
> >> > Before we queued the buffer with a invalid fence (-1), it causes
> >> > benchmarks such as flatland to fail. This patch enables explicit
> >> > sync feature on android.
> >> >
> >> > Now we get the out fence during the flushing buffer and then pass
> >> > it to SurfaceFlinger in eglSwapbuffer function.
> >> >
> >> > v2: a) Also implement the fence in cancelBuffer.
> >> > b) The last sync fence is stored in drawable object
> >> >rather than brw context.
> >> > c) format clear.
> >> >
> >> > v3: a) Save the last fence fd in DRI Context object.
> >> > b) Return the last fence if the batch buffer is empty and
> >> >nothing to be flushed when _intel_batchbuffer_flush_fence
> >> > c) Add the new interface in vbtl to set the retrieve fence
> >> >
> >> > v3.1 a) close fd in the new vbtl interface on none Android platform
> >> >
> >> > v4: a) The last fence is saved in brw context.
> >> > b) The retrieve fd is for all the platform but not just Android
> >> > c) Add a uniform dri2 interface to initialize the surface.
> >> >
> >> > v4.1: a) make some changes of variable name.
> >> >   b) the patch is broken into two patches.
> >> >
> >> > v4.2: a) Add a deinit interface for surface to clear the out fence
> >> >
> >> > v5: a) Add enable_out_fence to init, platform sets it true or
> >> >false
> >> > b) Change get fd to update fd and check for fence
> >> > c) Commit description updated
> >> >
> >> Seems like most suggestions in last version did not get addressed.
> >> Please comment if you disagree (it can be that we've
> >> misread/misremember
> >> something) before posting another version.
> >>
> >> To reiterate:
> >>  - add blank line between variable declarations and code

Done.

> >>  - use more consistent function names

Do you want any specific functioned to be renamed?

> >>  - comment above queueBuffer needs fixing

It reads as below now.
   /* Queue the buffer with stored out fence fd. The ANativeWindow or buffer
* consumer may choose to wait for the fence to signal before accessing
* it. If fence fd value is -1, buffer can be accessed by consumer
* immediately. Consumer or application shouldn't rely on timestamp
* associated with fence if the fence fd is -1.
*
* Ownership of fd is transferred to consumer after queueBuffer and the
* consumer is responsible for closing it. Caller must not use the fd
* after passing it to queueBuffer.
*/

Looks ok?

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

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?

> >>  - don't introduce unused variables (in make_current)

Done.

> >>  - the create fd for the old display surface (in make_current) seems
> >> bogus

Done.

> >>
> >
> > I think the patch has gone through headline changes, so it's bit
> > difficult to track back. I tried to address Rafael's comments (latest
> > before v5) assuming others were taken care of, after
> > 4 versions. Doesn’t seem to be the case. No problem, thanks for
> > re-iterating.  I'll work on fixing these and discuss as required.
> >
> >> Also, please change the commit 

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

[Tomasz Figa]

On Thu, Aug 3, 2017 at 10:11 PM, Marathe, Yogesh
 wrote:
> Emil,
>
>> -Original Message-
>> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
>> Sent: Thursday, August 3, 2017 4:06 PM
>> To: Marathe, Yogesh 
>> Cc: ML mesa-dev ; Wu, Zhongmin
>> 
>> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync 
>> fence
>> for Android OS
>>
>> On 2 August 2017 at 20:01,   wrote:
>> > From: Zhongmin Wu 
>> >
>> > Before we queued the buffer with a invalid fence (-1), it causes
>> > benchmarks such as flatland to fail. This patch enables explicit sync
>> > feature on android.
>> >
>> > Now we get the out fence during the flushing buffer and then pass it
>> > to SurfaceFlinger in eglSwapbuffer function.
>> >
>> > v2: a) Also implement the fence in cancelBuffer.
>> > b) The last sync fence is stored in drawable object
>> >rather than brw context.
>> > c) format clear.
>> >
>> > v3: a) Save the last fence fd in DRI Context object.
>> > b) Return the last fence if the batch buffer is empty and
>> >nothing to be flushed when _intel_batchbuffer_flush_fence
>> > c) Add the new interface in vbtl to set the retrieve fence
>> >
>> > v3.1 a) close fd in the new vbtl interface on none Android platform
>> >
>> > v4: a) The last fence is saved in brw context.
>> > b) The retrieve fd is for all the platform but not just Android
>> > c) Add a uniform dri2 interface to initialize the surface.
>> >
>> > v4.1: a) make some changes of variable name.
>> >   b) the patch is broken into two patches.
>> >
>> > v4.2: a) Add a deinit interface for surface to clear the out fence
>> >
>> > v5: a) Add enable_out_fence to init, platform sets it true or
>> >false
>> > b) Change get fd to update fd and check for fence
>> > c) Commit description updated
>> >
>> Seems like most suggestions in last version did not get addressed.
>> Please comment if you disagree (it can be that we've misread/misremember
>> something) before posting another version.
>>
>> To reiterate:
>>  - add blank line between variable declarations and code
>>  - use more consistent function names
>>  - comment above queueBuffer needs fixing
>>  - version check (2+) the fence extension, calling .create_fence_fd() only 
>> when
>> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
>>  - don't introduce unused variables (in make_current)
>>  - the create fd for the old display surface (in make_current) seems bogus
>>
>
> I think the patch has gone through headline changes, so it's bit difficult to 
> track back. I tried
> to address Rafael's comments (latest before v5) assuming others were taken 
> care of, after
> 4 versions. Doesn’t seem to be the case. No problem, thanks for re-iterating. 
>  I'll work on
> fixing these and discuss as required.
>
>> Also, please change the commit summary message to reflect reality.
>> I'm thinking of the following, although you can come with something better.
>>
>> "egl: allow creation of per surface out fence
>>
>> Add plumbing to allow creation of per display surface out fence.
>>
>> Currently enabled only on Android, since the system expects a valid fd in
>> ANativeWindow::{queue,cancel}Buffer.
>> Without it tools such as flatland will fail, since they cannot get the 
>> timestamp as
>> we currently pass an fd of -1."
>>
>
> Ok. This sounds good to me, will also check commit message while pushing next 
> version.

Also please don't drop people from CC. Thanks in advance.

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marathe, Yogesh]

Emil,

> -Original Message-
> From: Emil Velikov [mailto:emil.l.veli...@gmail.com]
> Sent: Thursday, August 3, 2017 4:06 PM
> To: Marathe, Yogesh 
> Cc: ML mesa-dev ; Wu, Zhongmin
> 
> Subject: Re: [Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync 
> fence
> for Android OS
> 
> On 2 August 2017 at 20:01,   wrote:
> > From: Zhongmin Wu 
> >
> > Before we queued the buffer with a invalid fence (-1), it causes
> > benchmarks such as flatland to fail. This patch enables explicit sync
> > feature on android.
> >
> > Now we get the out fence during the flushing buffer and then pass it
> > to SurfaceFlinger in eglSwapbuffer function.
> >
> > v2: a) Also implement the fence in cancelBuffer.
> > b) The last sync fence is stored in drawable object
> >rather than brw context.
> > c) format clear.
> >
> > v3: a) Save the last fence fd in DRI Context object.
> > b) Return the last fence if the batch buffer is empty and
> >nothing to be flushed when _intel_batchbuffer_flush_fence
> > c) Add the new interface in vbtl to set the retrieve fence
> >
> > v3.1 a) close fd in the new vbtl interface on none Android platform
> >
> > v4: a) The last fence is saved in brw context.
> > b) The retrieve fd is for all the platform but not just Android
> > c) Add a uniform dri2 interface to initialize the surface.
> >
> > v4.1: a) make some changes of variable name.
> >   b) the patch is broken into two patches.
> >
> > v4.2: a) Add a deinit interface for surface to clear the out fence
> >
> > v5: a) Add enable_out_fence to init, platform sets it true or
> >false
> > b) Change get fd to update fd and check for fence
> > c) Commit description updated
> >
> Seems like most suggestions in last version did not get addressed.
> Please comment if you disagree (it can be that we've misread/misremember
> something) before posting another version.
> 
> To reiterate:
>  - add blank line between variable declarations and code
>  - use more consistent function names
>  - comment above queueBuffer needs fixing
>  - version check (2+) the fence extension, calling .create_fence_fd() only 
> when
> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
>  - don't introduce unused variables (in make_current)
>  - the create fd for the old display surface (in make_current) seems bogus
> 

I think the patch has gone through headline changes, so it's bit difficult to 
track back. I tried
to address Rafael's comments (latest before v5) assuming others were taken care 
of, after
4 versions. Doesn’t seem to be the case. No problem, thanks for re-iterating.  
I'll work on
fixing these and discuss as required.

> Also, please change the commit summary message to reflect reality.
> I'm thinking of the following, although you can come with something better.
> 
> "egl: allow creation of per surface out fence
> 
> Add plumbing to allow creation of per display surface out fence.
> 
> Currently enabled only on Android, since the system expects a valid fd in
> ANativeWindow::{queue,cancel}Buffer.
> Without it tools such as flatland will fail, since they cannot get the 
> timestamp as
> we currently pass an fd of -1."
>

Ok. This sounds good to me, will also check commit message while pushing next 
version.
 
> Thanks
> Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 2 August 2017 at 20:01,   wrote:
> From: Zhongmin Wu 
>
> Before we queued the buffer with a invalid fence (-1), it causes
> benchmarks such as flatland to fail. This patch enables explicit
> sync feature on android.
>
> Now we get the out fence during the flushing buffer and then pass
> it to SurfaceFlinger in eglSwapbuffer function.
>
> v2: a) Also implement the fence in cancelBuffer.
> b) The last sync fence is stored in drawable object
>rather than brw context.
> c) format clear.
>
> v3: a) Save the last fence fd in DRI Context object.
> b) Return the last fence if the batch buffer is empty and
>nothing to be flushed when _intel_batchbuffer_flush_fence
> c) Add the new interface in vbtl to set the retrieve fence
>
> v3.1 a) close fd in the new vbtl interface on none Android platform
>
> v4: a) The last fence is saved in brw context.
> b) The retrieve fd is for all the platform but not just Android
> c) Add a uniform dri2 interface to initialize the surface.
>
> v4.1: a) make some changes of variable name.
>   b) the patch is broken into two patches.
>
> v4.2: a) Add a deinit interface for surface to clear the out fence
>
> v5: a) Add enable_out_fence to init, platform sets it true or
>false
> b) Change get fd to update fd and check for fence
> c) Commit description updated
>
Seems like most suggestions in last version did not get addressed.
Please comment if you disagree (it can be that we've
misread/misremember something) before posting another version.

To reiterate:
 - add blank line between variable declarations and code
 - use more consistent function names
 - comment above queueBuffer needs fixing
 - version check (2+) the fence extension, calling .create_fence_fd()
only when .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
 - don't introduce unused variables (in make_current)
 - the create fd for the old display surface (in make_current) seems bogus

Also, please change the commit summary message to reflect reality.
I'm thinking of the following, although you can come with something better.

"egl: allow creation of per surface out fence

Add plumbing to allow creation of per display surface out fence.

Currently enabled only on Android, since the system expects a valid fd
in ANativeWindow::{queue,cancel}Buffer.
Without it tools such as flatland will fail, since they cannot get the
timestamp as we currently pass an fd of -1."

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[yogesh . marathe]

From: Zhongmin Wu 

Before we queued the buffer with a invalid fence (-1), it causes
benchmarks such as flatland to fail. This patch enables explicit
sync feature on android.

Now we get the out fence during the flushing buffer and then pass
it to SurfaceFlinger in eglSwapbuffer function.

v2: a) Also implement the fence in cancelBuffer.
b) The last sync fence is stored in drawable object
   rather than brw context.
c) format clear.

v3: a) Save the last fence fd in DRI Context object.
b) Return the last fence if the batch buffer is empty and
   nothing to be flushed when _intel_batchbuffer_flush_fence
c) Add the new interface in vbtl to set the retrieve fence

v3.1 a) close fd in the new vbtl interface on none Android platform

v4: a) The last fence is saved in brw context.
b) The retrieve fd is for all the platform but not just Android
c) Add a uniform dri2 interface to initialize the surface.

v4.1: a) make some changes of variable name.
  b) the patch is broken into two patches.

v4.2: a) Add a deinit interface for surface to clear the out fence

v5: a) Add enable_out_fence to init, platform sets it true or
   false
b) Change get fd to update fd and check for fence
c) Commit description updated

Signed-off-by: Zhongmin Wu 
Signed-off-by: Yogesh Marathe 
---
 src/egl/drivers/dri2/egl_dri2.c | 57 +
 src/egl/drivers/dri2/egl_dri2.h |  9 +
 src/egl/drivers/dri2/platform_android.c | 12 --
 src/egl/drivers/dri2/platform_drm.c |  3 +-
 src/egl/drivers/dri2/platform_surfaceless.c |  3 +-
 src/egl/drivers/dri2/platform_wayland.c |  3 +-
 src/egl/drivers/dri2/platform_x11.c |  3 +-
 src/egl/drivers/dri2/platform_x11_dri3.c|  3 +-
 8 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index a197e04..f4c521d 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1315,6 +1315,34 @@ dri2_destroy_context(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLContext *ctx)
return EGL_TRUE;
 }
 
+EGLBoolean
+dri2_surf_init(_EGLSurface *surf, _EGLDisplay *dpy, EGLint type,
+_EGLConfig *conf, const EGLint *attrib_list, EGLBoolean 
enable_out_fence)
+{
+   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
+   dri2_surf->out_fence_fd = -1;
+   dri2_surf->enable_out_fence = enable_out_fence;
+   return _eglInitSurface(surf, dpy, type, conf, attrib_list);
+}
+
+static void
+dri2_surface_set_out_fence( _EGLSurface *surf, int fence_fd)
+{
+   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
+   if (dri2_surf->out_fence_fd >=0)
+  close(dri2_surf->out_fence_fd);
+
+   dri2_surf->out_fence_fd = fence_fd;
+}
+
+void
+dri2_surf_deinit(_EGLSurface *surf)
+{
+   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
+   dri2_surface_set_out_fence(surf, -1);
+   dri2_surf->enable_out_fence = false;
+}
+
 static EGLBoolean
 dri2_destroy_surface(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf)
 {
@@ -1326,6 +1354,26 @@ dri2_destroy_surface(_EGLDriver *drv, _EGLDisplay *dpy, 
_EGLSurface *surf)
return dri2_dpy->vtbl->destroy_surface(drv, dpy, surf);
 }
 
+static void
+dri2_surf_update_fence_fd(_EGLContext *ctx,
+   _EGLDisplay *dpy, _EGLSurface *surf)
+{
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
+   struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
+   int fence_fd = -1;
+   void *fence;
+   __DRIcontext *dri_ctx = dri2_egl_context(ctx)->dri_context;
+   if (dri2_surf->enable_out_fence && dri2_dpy->fence) {
+  fence = dri2_dpy->fence->create_fence_fd(dri_ctx, -1);
+  if (fence) {
+ fence_fd = dri2_dpy->fence->get_fence_fd(dri2_dpy->dri_screen,
+  fence);
+ dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, fence);
+  }
+   }
+   dri2_surface_set_out_fence(surf, fence_fd);
+}
+
 /**
  * Called via eglMakeCurrent(), drv->API.MakeCurrent().
  */
@@ -1360,8 +1408,11 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *dsurf,
rdraw = (rsurf) ? dri2_dpy->vtbl->get_dri_drawable(rsurf) : NULL;
cctx = (dri2_ctx) ? dri2_ctx->dri_context : NULL;
 
+   int fence_fd = -1;
if (old_ctx) {
   __DRIcontext *old_cctx = dri2_egl_context(old_ctx)->dri_context;
+  if (old_dsurf)
+ dri2_surf_update_fence_fd(old_ctx, disp, old_dsurf);
   dri2_dpy->core->unbindContext(old_cctx);
}
 
@@ -1498,6 +1549,9 @@ static EGLBoolean
 dri2_swap_buffers(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf)
 {
struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
+   _EGLContext *ctx = _eglGetCurrentContext();
+   if (ctx && surf)
+  dri2_surf_update_fence_fd(ctx, dpy, surf);
return dri2_dpy->vtbl->swap_buffers(drv, dpy,