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

[Pekka Paalanen]

On Tue, 16 Dec 2014 13:19:53 -0800
Bill Spitzak <spitzak at gmail.com> wrote:

> It looks like the purpose of "dmabuf_batch" is to send a more complex 
> set of arguments to the dmabuf::create_buffer request. This is a 
> variable-sized list of fd's, each containing a variable-sized list of 
> planes. The Wayland protocol does not have a method of passing this as a 
> request argument, so this object is used to build it with multiple 
> requests. Is this correct? And is this considered the correct way to do 
> this sort of thing in Wayland?

We could use wl_array, except it does not work with fd's. So yes, this
is the only way to get a variable number of fd's into a single
final request.

> It is not clear but I assume you must add at least one fd to the 
> dmabuf_batch for this to work, right?

Yes.

> Assuming this is correct, I think some consolidation of the objects 
> would help. A few ideas:
> 
> - Make create_buffer a method on dmabuf_batch, not on dmabuf.

I'm not sure what would be the difference. You can't do it without a
dmabuf_batch anyway (the spec does not have allow-null="true").

> - Get rid of dmabuf_create_feedback object, and just reuse the 
> already-existing dmabuf_batch object to deliver the success/failure event.

Then it's not a dmabuf_batch anymore, you need to think of a new name
that describes it. Using a one-shot feedback object is a standard
Wayland design pattern.

> - I am a bit suspicious of the exactly 3 planes, there are 4:2:2 formats 
> with alpha channels. I think it would be better if there was a request 
> per-plane.

This is modelled after
https://www.khronos.org/registry/egl/extensions/EXT/EGL_EXT_image_dma_buf_import.txt

The EGL extension could be extended to fourth plane though, so maybe we
could use wl_array here to achieve easier extendability.

The DRM user ABI seems to use 4 indeed.

> - Isn't there a problem with having an event deliver a new object (ie 
> the wl_buffer in the create_successful event)? Can the dmabuf_batch 
> object just "be" a wl_buffer, but you cannot use it as a wl_buffer until 
> you get the success event?

There is no problem in creating a new object by an event. It is rarely
used, but it must work.

> So in the most-cleaned-up version I can come up with the api is more 
> like this:
> 
> dmabuf - singleton factory used to create dmabuf_buffer.
> 
> dmabuf::create_buffer - create a new dmabuf_buffer. You must then do 
> some requests on it before it can be used as a wl_buffer.
> 
> dmabuf::format event - same as you describe it
> 
> dmabuf_buffer - A subclass of wl_buffer representing a (potential) dma 
> buffer.

There is no such thing as sub-classing, all object types are strict and
must match exactly in the protocol (no inheritance or is-a). The
wl_buffer object must be created at some point.

> dmabuf_buffer::add_fd - Add a new fd describing a dmabuf
> 
> dmabuf_buffer::add_plane - Add a plane (offset + stride) to map from the 
> most recent fd.
> 
> dmabuf_buffer::create - try to create the dma buffer. After this you 
> cannot do the add_fd/plane requests.
> 
> dmabuf_buffer::create_successful event - you can now use the 
> dmabuf_buffer as a wl_buffer (for instance to attach it to a wl_surface).
> 
> dmabuf_buffer::create_failed event - It did not work. The only thing you 
> are allowed to do is destroy the dmabuf_buffer.

I wonder if that is simpler of just different, assuming you fix the
sub-classing.

I'd rather not do "cosmetic" changes yet, because we are likely going
to need more parameters as the dma_buf kernel interfaces are
being developed.


Thanks,
pq