Message-ID: <CAPj87rOWDdB0VD18tefvt_nLV3grNft_916XRKFjaadT3q6LGg@mail.gmail.com>
Date:	Tue, 5 Apr 2016 13:57:19 +0100
From:	Daniel Stone <daniel@...ishbar.org>
To:	Rob Clark <robdclark@...il.com>
Cc:	Gustavo Padovan <gustavo@...ovan.org>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Arve Hjønnevåg <arve@...roid.com>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Riley Andrews <riandrews@...roid.com>,
	Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
	John Harrison <John.C.Harrison@...el.com>
Subject: Re: [RFC 1/6] drm/fence: add FENCE_FD property to planes

Hi,

On 5 April 2016 at 13:36, Rob Clark <robdclark@...il.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?

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

Cheers,
Daniel

Powered by blists - more mailing lists