Tomasz, > -----Original Message----- > From: Tomasz Figa [mailto:[email protected]] > Sent: Friday, August 4, 2017 6:52 AM > To: Marathe, Yogesh <[email protected]> > Cc: Emil Velikov <[email protected]>; Gao, Shuo > <[email protected]>; Liu, Zhiquan <[email protected]>; Daniel Stone > <[email protected]>; Nicolai Hähnle <[email protected]>; > Antognolli, Rafael <[email protected]>; Eric Engestrom > <[email protected]>; Wu, Zhongmin <[email protected]>; Kenneth > Graunke <[email protected]>; Kondapally, Kalyan > <[email protected]>; Rainer Hochecker <[email protected]>; > ML mesa-dev <[email protected]>; Timothy Arceri > <[email protected]>; Varad Gautam <[email protected]> > 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 <[email protected]> > wrote: > > Adding folks who were CCed for earlier versions. > > > > Hi Emil, few doubts and comments below. > > > >> -----Original Message----- > >> From: Tomasz Figa [mailto:[email protected]] > >> Sent: Thursday, August 3, 2017 7:19 PM > >> To: Marathe, Yogesh <[email protected]> > >> Cc: Emil Velikov <[email protected]>; Antognolli, Rafael > >> <[email protected]>; ML mesa-dev <mesa- > >> [email protected]>; Wu, Zhongmin <[email protected]> > >> 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 > >> <[email protected]> wrote: > >> > Emil, > >> > > >> >> -----Original Message----- > >> >> From: Emil Velikov [mailto:[email protected]] > >> >> Sent: Thursday, August 3, 2017 4:06 PM > >> >> To: Marathe, Yogesh <[email protected]> > >> >> Cc: ML mesa-dev <[email protected]>; Wu, Zhongmin > >> >> <[email protected]> > >> >> 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, <[email protected]> wrote: > >> >> > From: Zhongmin Wu <[email protected]> > >> >> > > >> >> > 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
