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 >> - use more consistent function names >> - comment above queueBuffer needs fixing >> - version check (2+) the fence extension, calling .create_fence_fd() only >> when >> .get_capabilities() advertises __DRI_FENCE_CAP_NATIVE_FD >> - don't introduce unused variables (in make_current) >> - the create fd for the old display surface (in make_current) seems bogus >> > > 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." >> > > 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
