On Mon, Aug 7, 2017 at 3:44 PM, Marathe, Yogesh <yogesh.mara...@intel.com> 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 >> > <yogesh.mara...@intel.com> >> > 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 <yogesh.mara...@intel.com> wrote: >> > >> > Tomasz, Emil, >> > >> > >> > >> >> -----Original Message----- >> > >> >> From: Tomasz Figa [mailto:tf...@chromium.org] On Fri, Aug 4, >> > >> >> 2017 at 9:55 PM, Emil Velikov <emil.l.veli...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
