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

[Daniel Stone]

Hi,

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

Yeah, both should be fixed. :)

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

Depends on how you look at it, I guess. From the point of view of all
fence-like things being consistent, yes, it falls down. But from the
point of view of in-fences being attached to an FB, and out-fences
(like events) being separately attached to a request, it's a lot more
consistent.

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

True, but it complicates the (already not hugely straightforward)
parsing that the ioctl has to do. It also makes extending the ioctl a
little harder to do in future, because you're adding in two
variable-size elements, and have to do some fairly complicated parsing
to even figure out what the size _should_ be. So I'd rather not do it
if there was any way out of it; at the very least it'd have to be
userptr rather than array.

Cheers,
Daniel