Hi,
On 5 April 2016 at 13:36, Rob Clark robdclark@gmail.com wrote:
ok, so I've been slacking on writing up the reasons that I don't like the idea of using a property for fd's (including fence fd's).. I did at one point assume we'd use properties for fence fd's, but that idea falls apart pretty quickly when you think about the 'struct file' vs fd lifecycle. It could *possibly* work if it was a write-only property, but I don't think that is a good solution.
I've been assuming that it would have to be write-only; I don't believe there would be any meaningful usecases for read. Do you have any in mind, or is it just a symmetry/cleanliness thing?
The issue is that 'struct file' / 'int fd' have a fundamentally different lifecycle compared to 'kms obj' / 'u32 obj_id'. For the kms objects (like framebuffers) where we can use them with properties, the id is tied to the kernel side object. This is not true for file descriptors. Resulting in a few problems:
Surely the fence FD tied to the kernel-side struct fence, in exactly the same way, no?
- if it was a readable property, reading it would (should) take a
reference.. we can't just return fence_fd that was previously set (since in the mean time userspace might have close()d the fd). So we have to return a fresh fd. And if userspace (like modetest) doesn't realize it is responsible to close() that fd we have a leak
Again, assuming that read would always return -1.
- basically we shouldn't be tracking fd's on the kernel side, since
we can only count on them being valid during the duration of the ioctl call. Once the call returns, you must assume userspace has close()d the fd. So in the ioctl we need to convert fd -> file, and from then on out track the file object (or in this case the underlying fence object).
Right, it would have to be the same as KMS objects, where userspace passes in an ID (in this case an entry in a non-IDR table, but still), and the kernel maps that to struct fence and handles the lifetime. So, almost exactly the same as what we do with KMS objects ...
I guess we *could* have something analogous to dmabuf, where we import the fence fd and get a kms drm_fence object (with an id tied to the kernel side object), and then use a property to attach the drm_fence object.. but that seems kind of pointless and just trying to force the 'everything is a property' mantra.
I think that would be pointless indirection as well.
I think it is really better to pass in an array of 'struct { u32 plane; int fd }' (which the atomic ioctl code converts into 'struct fence's and attaches to the plane state) and an array of 'struct { u32 crtc; int fd }' filled in on the kernel side for the out-fences.
Mmm, it definitely makes ioctl parsing harder, and still you rely on drivers to implement the more-difficult-to-not-screw-up part, which (analagous to crtc_state->event) is the driver managing the lifecycle of that component of the state. We already enforce this with events though, and the difficult part wasn't in the userspace interface, but the backend handling. This doesn't change at all regardless of whether we use a property or an external array, so ...
Cheers, Daniel