[Mesa-dev] [PATCH v5 2/2] i965: Queue the buffer with a sync fence for Android OS

Tomasz Figa tfiga at chromium.org
Thu Aug 3 13:48:41 UTC 2017


On Thu, Aug 3, 2017 at 10:11 PM, Marathe, Yogesh
<yogesh.marathe at intel.com> wrote:
> Emil,
>
>> -----Original Message-----
>> From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>> Sent: Thursday, August 3, 2017 4:06 PM
>> To: Marathe, Yogesh <yogesh.marathe at intel.com>
>> Cc: ML mesa-dev <mesa-dev at lists.freedesktop.org>; Wu, Zhongmin
>> <zhongmin.wu at 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.marathe at intel.com> wrote:
>> > From: Zhongmin Wu <zhongmin.wu at 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


More information about the mesa-dev mailing list