Re: [RFC 1/6] drm/fence: add FENCE_FD property to planes

[Rob Clark]

On Tue, Apr 5, 2016 at 8:57 AM, Daniel Stone <daniel@xxxxxxxxxxxxx> wrote:
> Hi,
>
> On 5 April 2016 at 13:36, Rob Clark <robdclark@xxxxxxxxx> 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?

no, don't see a use-case for it.. but this patch didn't seem to be
preventing it.  And it was storing the fence_fd on the kernel side
which is a no-no as well.

>> 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?

well, what I mean is you can't keep around the int fd on the kernel
side, like this patch does

A write-only property, which immediately (ie. during the ioctl call)
is converted into a fence object, would work.  Although given that we
need to handle fences differently (ie. not a property) for out-fences,
it seems odd to shoehorn them into a property for in-fences.

>> 1) 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.
>
>> 2) 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 ...

hmm, I'm assuming that the in/out arrays are handled in
drm_mode_atomic_ioctl() and the drivers never see 'em..

BR,
-R

> Cheers,
> Daniel