Tomasz, > -----Original Message----- > From: Tomasz Figa [mailto:tf...@chromium.org] > Sent: Friday, August 4, 2017 6:52 AM > To: Marathe, Yogesh <yogesh.mara...@intel.com> > Cc: Emil Velikov <emil.l.veli...@gmail.com>; Gao, Shuo > <shuo....@intel.com>; Liu, Zhiquan <zhiquan....@intel.com>; Daniel Stone > <dani...@collabora.com>; Nicolai Hähnle <nicolai.haeh...@amd.com>; > Antognolli, Rafael <rafael.antogno...@intel.com>; Eric Engestrom > <e...@engestrom.ch>; Wu, Zhongmin <zhongmin...@intel.com>; Kenneth > Graunke <kenn...@whitecape.org>; Kondapally, Kalyan > <kalyan.kondapa...@intel.com>; Rainer Hochecker <fernetme...@online.de>; > ML mesa-dev <mesa-dev@lists.freedesktop.org>; Timothy Arceri > <tarc...@itsqueeze.com>; Varad Gautam <varad.gau...@collabora.com> > 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 <yogesh.mara...@intel.com> > 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 <yogesh.mara...@intel.com> > >> Cc: Emil Velikov <emil.l.veli...@gmail.com>; Antognolli, Rafael > >> <rafael.antogno...@intel.com>; ML mesa-dev <mesa- > >> d...@lists.freedesktop.org>; Wu, Zhongmin <zhongmin...@intel.com> > >> 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 > >> <yogesh.mara...@intel.com> wrote: > >> > Emil, > >> > > >> >> -----Original Message----- > >> >> From: Emil Velikov [mailto:emil.l.veli...@gmail.com] > >> >> Sent: Thursday, August 3, 2017 4:06 PM > >> >> To: Marathe, Yogesh <yogesh.mara...@intel.com> > >> >> Cc: ML mesa-dev <mesa-dev@lists.freedesktop.org>; Wu, Zhongmin > >> >> <zhongmin...@intel.com> > >> >> 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, <yogesh.mara...@intel.com> wrote: > >> >> > From: Zhongmin Wu <zhongmin...@intel.com> > >> >> > > >> >> > 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. >
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'? > > //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... > > Moreover, there are users of kernels older that current upstream release, > which > might not have support for I915_PARAM_HAS_EXEC_FENCE. > Ok, understood, then we have to keep it. > Best regards, > Tomasz _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
