[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
<yogesh.marathe at intel.com> wrote:
>> -----Original Message-----
>>
>> Tomasz,
>>
>> > -----Original Message-----
>> > From: Tomasz Figa [mailto:tfiga at chromium.org]
>> > Sent: Saturday, August 5, 2017 8:47 AM
>> >
>> > Hi Yogesh,
>> >
>> > On Sat, Aug 5, 2017 at 1:22 AM, Marathe, Yogesh
>> > <yogesh.marathe at intel.com>
>> > wrote:
>> > >> -----Original Message-----
>> > >> From: Tomasz Figa [mailto:tfiga at chromium.org]
>> > >> Sent: Friday, August 4, 2017 9:39 PM On Sat, Aug 5, 2017 at 12:53
>> > >> AM, Marathe, Yogesh <yogesh.marathe at intel.com> wrote:
>> > >> > Tomasz, Emil,
>> > >> >
>> > >> >> -----Original Message-----
>> > >> >> From: Tomasz Figa [mailto:tfiga at chromium.org] On Fri, Aug 4,
>> > >> >> 2017 at 9:55 PM, Emil Velikov <emil.l.velikov at gmail.com>
>> > >> wrote:
>> > >> >> >>> >>  - version check (2+) the fence extension, calling
>> > >> >> >>> >> .create_fence_fd() only when
>> > >> >> >>> >> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD
>> > >> >> >>
>> > >> >> >> The check looks like below now, this is in
>> > >> >> >> dri2_surf_update_fence_fd() before
>> > >> >> create_fence_fd is called.
>> > >> >> >>
>> > >> >> >> if (dri2_surf->enable_out_fence && dri2_dpy->fence) {
>> > >> >> >>        if(__DRI_FENCE_CAP_NATIVE_FD | dri2_dpy->fence-
>> > >> >> >get_capabilities(dri2_dpy->dri_screen)) {
>> > >> >> >>           //create_fence_fd call
>> > >> >> >>        }
>> > >> >> >> }
>> > >> >> >>
>> > >> >> > Close but no cigar.
>> > >> >> >
>> > >> >> > if (dri2_surf->enable_out_fence && dri2_dpy->fence &&
>> > >> >> >     dri2_dpy->fence->base.version >= 2 &&
>> > >> >> > dri2_dpy->fence->get_capabilities) {
>> > >> >> >
>> > >> >> >    if (dri2_dpy->fence->get_capabilities(dri2_dpy->dri_screen)
>> > >> >> > &
>> > >> >> > __DRI_FENCE_CAP_NATIVE_FD) {
>> > >> >> >         //create_fence_fd call
>> > >> >> >    }
>> > >> >> > }
>> > >> >>
>> > >> >> If this needs so complicated series of checks, maybe it would
>> > >> >> make more sense to just set enable_out_fence based on
>> > >> >> availability of the capability at initialization time?
>> > >> >
>> > >> > I liked this one compared to nested ifs in dri2_surf_update_fence_fd().
>> > >> >
>> > >> >>
>> > >> >> >
>> > >> >> >> Overall, if I further go ahead and check, actually
>> > >> >> >> get_capabilities() ultimately returns based on has_exec_fence
>> > >> >> >> which depends on I915_PARAM_HAS_EXEC_FENCE. This is always
>> > >> >> >> set to true for i915 in kernel drv unless forced to false!!
>> > >> >> >> I'm not sure if that inner check of
>> > >> >> get_capabilities still makes sense. Isn't the first one sufficient?
>> > >> >> >>
>> > >> >> > Not sure what you mean with "first one", but consider the
>> > >> >> > following
>> > >> example:
>> > >> >> >  - old kernel which does not support (or has force disabled)
>> > >> >> > I915_PARAM_HAS_EXEC_FENCE.
>> > >> >> >  - new userspace which unconditionally advertises the fence v2
>> > >> >> > extension IIRC one may tweak that things to only conditionally
>> > >> >> > advertise it, but IMHO it's not worth the hassle.
>> > >> >> >
>> > >> >> > Even then, Mesa can produce 20 DRI drivers (used by the EGL
>> > >> >> > module) so focusing on one doesn't quite work.
>> > >> >> >
>> > >> >> >>> >>  - don't introduce unused variables (in make_current)
>> > >> >> >>
>> > >> >> >> Done.
>> > >> >> >>
>> > >> >> >>> >>  - the create fd for the old display surface (in
>> > >> >> >>> >> make_current) seems bogus
>> > >> >> >>
>> > >> >> >> Done.
>> > >> >> >>
>> > >> >> > Did you drop it all together or changed to use some other surface?
>> > >> >> > Would be nice to hear the reason why it was added - perhaps
>> > >> >> > I'm missing something.
>> > >> >>
>> > >> >> We have to keep it, otherwise there would be no fence available
>> > >> >> at the time of surface destruction, while, at least for Android,
>> > >> >> a fence can be passed to window's cancelBuffer callback.
>> > >> >>
>> > >> >> >
>> > >> >> > I think that we want a fence/fd for the new draw surface.
>> > >> >> > Since otherwise one won't get created up until the first SwapBuffers
>> call.
>> > >> >>
>> > >> >> I might be missing something, but wouldn't that insert a fence
>> > >> >> at the beginning of command stream, before even doing anything?
>> > >> >> At least in Android use cases, the only places we need the fence
>> > >> >> is in SwapBuffers and DestroySurface and the fence should be
>> > >> >> inserted after all the commands for rendering into given surface.
>> > >> >>
>> > >> >
>> > >> > Emil,
>> > >> >
>> > >> > Tomasz sounds convincing to me here, I just went ahead with the
>> > >> > comment to try out and flatland worked even after removing that.
>> > >> > Zhongmin can explain better but I think in earlier revisions this
>> > >> > was done for cancelBuffer to match with queueBuffer, I mean we
>> > >> > are passing valid fd for queueBuffer by doing this we would have
>> > >> > a valid fd during
>> > >> cancelBuffer.  Not sure if this is the reason / one of the reason.
>> > >> >
>> > >> > I will go ahead with rest of your comments if we are ok to keep
>> > >> > fd for old display surface in make_current.
>> > >>
>> > >> My understanding is that nobody actually cares about the fence that
>> > >> cancelBuffer returns, because the contents of the buffer are going
>> > >> to be discarded anyway and the buffer doesn't go to the consumer (e.g.
>> > >> flatland code that reads the timestamp). I even suspect that
>> > >> typically destroySurface would be called directly after swapBuffers
>> > >> and the surface wouldn't have a buffer to cancel. You can easily
>> > >> check this by adding a print before cancelBuffer call happens. So
>> > >> we might actually be fine with simpler code that gets fence only for
>> swapBuffers.
>> > >>
>> > >
>> > > Sure. I can confirm this.
>>
>> As read more, yes, cancelled buffer doesn't go to consumer although it might
>> get reused and it seems it won't be overwritten until the fence is signaled.
>> destroySurface is called in the end in tearDown and we can still live without the
>> code in discussion because second part of comment that describes cancelBuffer.
>>
>>     // cancelBuffer returns a dequeued buffer to the BufferQueue, but doesn't
>>     // queue it for use by the consumer.
>>     //
>>     // The buffer will not be overwritten until the fence signals.  The fence
>>     // will usually be the one obtained from dequeueBuffer.
>>     virtual status_t cancelBuffer(int slot, const sp<Fence>& fence);
>>
>> We definitely don’t have to create a new fence for old surface. In case of buffer
>> reuse, we anyway create a new fence during swap which should be sufficient.
>>
>> > >
>> > >> 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.

Best regards,
Tomasz