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 <yogesh.mara...@intel.com> > On Mon, Aug 7, 2017 at 2:19 PM, Marathe, Yogesh > <yogesh.mara...@intel.com> 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 > >> <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. > > I think it would make sense to behave consistently in both > queueBuffer() and cancelBuffer(), especially since Zhongmin's patch (based on > my suggestions) had that already implemented. There might be also some > performance benefits if we could call cancelBuffer() with a fence, without > doing > a flush on Mesa side. >
Sorry I'm confused, we are moving back and forth, finally do you want to put the check back for old display surface? v6 that I pushed yesterday doesn’t have it! > 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
