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? > >> - 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 } } 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? > >> - don't introduce unused variables (in make_current) Done. > >> - the create fd for the old display surface (in make_current) seems > >> bogus Done. > >> > > > > I think the patch has gone through headline changes, so it's bit > > difficult to track back. I tried to address Rafael's comments (latest > > before v5) assuming others were taken care of, after > > 4 versions. Doesn’t seem to be the case. No problem, thanks for > > re-iterating. I'll work on fixing these and discuss as required. > > > >> Also, please change the commit summary message to reflect reality. > >> I'm thinking of the following, although you can come with something better. > >> > >> "egl: allow creation of per surface out fence > >> > >> Add plumbing to allow creation of per display surface out fence. > >> > >> Currently enabled only on Android, since the system expects a valid > >> fd in ANativeWindow::{queue,cancel}Buffer. > >> Without it tools such as flatland will fail, since they cannot get > >> the timestamp as we currently pass an fd of -1." > >> Done. I have taken care of things you've mentioned, so will push v6 based on answers above. > > > > Ok. This sounds good to me, will also check commit message while pushing > next version. > > Also please don't drop people from CC. Thanks in advance. > > Best regards, > Tomasz _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
