[PATCH 1/7] protocol: add linux_dmabuf extension RFCv1

[Daniel Vetter]

On Tue, Jan 20, 2015 at 10:31:52AM +0200, Pekka Paalanen wrote:
> On Tue, 20 Jan 2015 07:41:52 +0100
> Daniel Vetter <daniel at ffwll.ch> wrote:
> 
> > On Thu, Jan 08, 2015 at 02:26:00PM +0200, Pekka Paalanen wrote:
> > > On Mon, 5 Jan 2015 11:44:49 +0100
> > > Daniel Vetter <daniel at ffwll.ch> wrote:
> > > 
> 
> > > > But as long as you don't do crazy things the drm dma-buf import will
> > > > notice that it's the same dma-buf and dtrt. Crazy = actively trying to
> > > > reimport buffers while some other thread is dropping the last drm
> > > > use-count for the same.
> > > 
> > > I'm not sure how one could even attempt such things.
> > 
> > You need your dma-buf to come from some other device (e.g. v2l pipeline),
> > otherwise it's impossible. And if it ever becomes an issue userspace
> > shouldn't ever care since the problem is in the kernel really.
> > 
> > > > > I assumed it could be a problem, so I designed a way that maintains the
> > > > > numerical identities between the fds.
> > > > > 
> > > > > Also, we don't have a mechanism to send "not a valid fd" for an fd
> > > > > field, in case there are less than 3 (4?) planes, so we need a separate
> > > > > request for each valid fd.
> > > > 
> > > > Maybe we could share the drm helpers to compute the number of planes
> > > > somehow and make it part of the proto? tbh no idea whether that would help
> > > > you (I have no clue about wl protos after all ...).
> > > 
> > > Do you mean compute the number of planes from a DRM fourcc code? I
> > > can't see how that would help protocol-wise. We have no such thing
> > > as variable arguments. Every argument listed in the XML
> > > specification of a message must be there exactly. If an argument is
> > > specified to be an fd, it must also be a valid fd at runtime AFAIK.
> > > 
> > > To send 1-4 fds, one needs to either pick a message type (request)
> > > that specifies exactly the right number of fds, or use a single
> > > message type to send each fd one by one. The difference in IPC
> > > efficiency is negligible, I believe.
> > > 
> > > I suppose one could use a single message type with 4 fds always,
> > > just repeating the unwanted arguments as one of the wanted fds, but
> > > it feels... ugly and inefficient (extra system calls to transmit
> > > extra fds), especially if we usually need just one fd.
> > > 
> > > At least we could simplify the dmabuf_batch interface by leaving
> > > just one offset/stride argument pair in the "add" request. That
> > > would make it a lot more clear.
> > > 
> > > And we should bump the limit to 4 dmabufs per wl_buffer, like you
> > > pointed out from AddFB2. The above simplification makes this very
> > > easy. In fact, we wouldn't even need to specify any max number in
> > > the protocol at all which is nice.
> > 
> > Well I was more thinking about the validation the wayland server is
> > supposed to do, not necessarily whether the protocol allows it. I.e. match
> > the input validation to what the drm core does (checks for number of
> > planes, that kind of stuff). Otoh I guess you could just do the addfb2
> > synchronously and reuse the checking the kernel does by forwarding the
> > -EINVAL.
> > 
> > I just thinking that some spec language along "the kernel's drm subsystem
> > is the authoritative sources for how these fourcc codes work" would be
> > good.
> 
> Ah. Foremost the compositor needs to guarantee that the wl_buffer
> (a set of dmabufs and metadata) is usable in GL, so I would expect
> the import checks done in EGL should be enough. If AddFB2 has
> additional restrictions, that's fine: the compositor just can't
> scan out the wl_buffer, but it can still composite it.
> 
> In the Wayland protocol extension, we would probably just say
> something like "if the compositor cannot use this buffer, you get a
> failure back instead of creating a wl_buffer object" or such.
> Practically the checking would be done by the EGL implementation.
> 
> Of course, using a DRM fourcc code with a wrong number of planes
> could be a fatal protocol error, just like using invalid offset or
> stride is an indication of a bug rather than just an unsupported
> format. That's a worthy idea to think about indeed.
> 
> How would we get this kind of centralized collection of DRM fourcc
> format descriptions to run checks with?

Copypaste from kernel maybe, perhaps after some refactoring to extract the
fourcc code checking into a mostly standalone drm_fourcc.c (functions like
drm_format_num_planes or anything else used by the input check code for
the addfb2 ioctl). Not a pretty approach, but I don't have a better idea
really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch