On Wed, Jan 27, 2016 at 01:41:03PM -0800, Greg Hackmann wrote:
On 01/27/2016 12:25 PM, Gustavo Padovan wrote:
Is there a value in keeping the abi unchanged? If not, then Documentation/ioctl/botching-up-ioctls.txt is worth a read.
None from me. I'll look where we can improve the ABI.
Android has existing clients of the current ABI. Thankfully they're all contained in system services like SurfaceFlinger, since end-user apps don't get direct access to fence fds.
As long the ABI breaks don't remove functionality we depend on, we can wrap around them in our userspace libsync. I'd rather not have to do that, but it's a price I'm willing to pay to get this moved out of staging.
- struct sync_file_info_data::fence_info is of type __u8 yet it is "a
fence_info struct for every fence in the sync_file". Thus shouldn't one use "struct fence_info" as the type ?
Agreed. But I'm currently thinking if we really should keep this ioctl.
Gustavo
I'm not seeing any consumers of driver_data in our tree. OTOH completely getting rid of the ioctl would be a problem, since SurfaceFlinger depends on the timestamp information for its own bookkeeping.
If we remove driver_data (and len is superflous too), then I think we should also make the master struct use common ioctl pattern: - Add a num_fences field or similar that the kernel fills out. - Make pt_info an __u64 pointer instead of a variable-length array (and length) - ioctl payload sizes are somewhat limited.
This way the interface is future-proofed for truly patalogical number of fences (which surface flinger won't do, but could happen in server/opencl/media workloads I'd imagine).
And I think driver_data really shouldn't be there, it makes things complicated with the array of variable-sized objects, and generic userspace can't really use it - for debug output we already have obj/driver_name per fence point, which I think is good enough.
Would that be ok for you from the Android side if Gustavo also provides a patch to update libsync? I don't think the ABI is fundamentally broken, but this light cleanup would be nice.
Wrt keeping SYNC_WAIT: I think that's totally fine. Redundant since polling is supported, but not really an issue imo either. If we're totally lazy we could implement SYNC_WAIT internally using poll and shave off a few lines of the implementation. -Daniel