On Wed, Aug 02, 2017 at 02:56:34PM +0100, Emil Velikov wrote: > On 1 August 2017 at 16:29, Rafael Antognolli > <rafael.antogno...@intel.com> wrote: > > On Mon, Jul 31, 2017 at 09:58:24AM -0700, Marathe, Yogesh wrote: > >> Rafael, Tomasz, > >> > >> > -----Original Message----- > >> > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf > >> > Of Rafael Antognolli > >> > Sent: Tuesday, July 25, 2017 9:39 PM > >> > To: Wu, Zhongmin <zhongmin...@intel.com> > >> > Cc: Gao, Shuo <shuo....@intel.com>; Liu, Zhiquan <zhiquan....@intel.com>; > >> > Daniel Stone <dani...@collabora.com>; emil.l.veli...@gmail.com; Eric > >> > Engestrom <e...@engestrom.ch>; Marathe, Yogesh > >> > <yogesh.mara...@intel.com>; tf...@chromium.org; Kondapally, Kalyan > >> > <kalyan.kondapa...@intel.com>; mesa-dev@lists.freedesktop.org; Varad > >> > Gautam <varad.gau...@collabora.com> > >> > Subject: Re: [Mesa-dev] [PATCH 2/2] i965: Queue the buffer with a sync > >> > fence > >> > for Android OS v4.2 > >> > > >> > On Tue, Jul 25, 2017 at 10:07:23AM +0800, Zhongmin Wu wrote: > >> > > Before we queued the buffer with a invalid fence (-1), it will make > >> > > some benchmarks failed to test such as flatland. > >> > > > >> > > 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 breaked into two patches. > >> > > > >> > > v4.2: a) Add a deinit interface for surface to clear the out fence > >> > > >> > Hi Zhongmin, > >> > > >> > The patch is indeed looking better. I agree with Tomasz, it would be > >> > good to > >> > have a way for the platform to inform whether it is interested in fences > >> > or not. > >> > What about some flag that you pass to dri2_surf_init? (I'm not sure > >> > that's the > >> > best place, though). > >> > > >> > >> I would like to take it forward from here for remaining review comments, > >> Zhongmin agrees. > >> > >> I added 'enable_out_fence' bool to dri2_surf_init() as a parameter and all > >> platforms except > >> android pass this as false. Based on 'enable_out_fence' stored with > >> surface, > >> 'dri2_surf_get/update_fence_fd()' has a check before it calls > >> create_fence_fd(). Is this the > >> right expectation here? > > > > Sounds good to me, although I would need to see the patch. Also please make > > sure that the dri2 fence extension is supported, assuming you are trying to > > enable this. > > > I've let glmark2 spin (egl+x11) during lunch and it showed no perf. > difference. > > Admittedly may not be the best of benchmarks for this use case, even > though it pushes 200-2000fps, while the new code is in the > eglSwapBuffers path. > I got the feeling that one is micro optimising then there's no need for it. > > Rafael, have you tried any test/benchmark on your end?
Hi Emil, Are you referring to checking whether the platform requested the fence fd or not? If so, I was thinking about making it optional mainly to avoid creating extra fds, but I didn't think it would have an impact on performance. Anyway, I didn't run any benchmark on this. Rafael _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
