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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
