After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 785262 - zwp_linux_dmabuf_v1 support
zwp_linux_dmabuf_v1 support
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2017-07-22 07:39 UTC by Daniel Stone
Modified: 2017-08-01 11:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wayland: Add zwp_linux_dmabuf_v1 support (33.08 KB, patch)
2017-07-22 07:39 UTC, Daniel Stone
none Details | Review
wayland: Add zwp_linux_dmabuf_v1 support (32.82 KB, patch)
2017-07-24 15:45 UTC, Daniel Stone
committed Details | Review

Description Daniel Stone 2017-07-22 07:39:09 UTC
dmabuf is a much nicer interface than wl_drm. It means people can do zero-copy buffer stuff externally (e.g. GStreamer/VA-API) without just copying wl_drm and making sure that can never change.

It also supports multi-planar buffers better, as well as buffer modifiers for exotic tiling / renderbuffer compression / etc modes.

Mesa EGL already preferentially uses zwp_linux_dmabuf_v1 over wl_drm for buffer transport where available, and there are patches on the list to do the same for Vulkan (both ANV/Intel and RADV/AMD); I tested with both and it works fine.

The code for image attribs was cribbed from Weston, where we (Collabora) also wrote it.
Comment 1 Daniel Stone 2017-07-22 07:39:53 UTC
Created attachment 356160 [details] [review]
wayland: Add zwp_linux_dmabuf_v1 support
Comment 2 Jonas Ådahl 2017-07-22 09:04:51 UTC
Review of attachment 356160 [details] [review]:

::: src/wayland/meta-wayland-dma-buf.c
@@ +67,3 @@
+   * at that point, it becomes owned by the MetaWaylandBuffer, which will
+   * unref this object for us. */
+  struct wl_listener destroy_listener;

This doesn't seem to be used anymore.

@@ +103,3 @@
+      break;
+    case DRM_FORMAT_ARGB2101010:
+      cogl_format = COGL_PIXEL_FORMAT_ARGB_2101010_PRE;

Do we actually always support this format?

@@ +123,3 @@
+
+  /* We can't import no planes. */
+  g_return_val_if_fail (dma_buf->fds[0] >= 0, FALSE);

This is a "soft assert" kind of macro, but this check looks like it should be done no matter whether those macros are enabled or not, so I suspect this should be a real if (foo) { set_error(...); return FALSE; } (also, it could cause crash in the caller if it fails since error is not set)

@@ +210,3 @@
+params_add (struct wl_client *client, struct wl_resource *resource,
+            int32_t fd, uint32_t plane_idx, uint32_t offset, uint32_t stride,
+            uint32_t drm_modifier_hi, uint32_t drm_modifier_lo)

Coding style: one argument per line, variable names aligned

@@ +280,3 @@
+
+static void
+buffer_destroy_req (struct wl_client *client, struct wl_resource *resource)

same about coding style

@@ +291,3 @@
+
+MetaWaylandDmaBufBuffer *
+meta_wayland_dma_buf_new (MetaWaylandBuffer *buffer, GError **error)

Coding style again

@@ +296,3 @@
+                               &dma_buf_buffer_impl))
+      return wl_resource_get_user_data (buffer->resource);
+

This shouldn't be called _new() since its not a constructor. Call it meta_wayland_dma_buf_from_buffer or something, and skip the error.

@@ +304,3 @@
+                      struct wl_resource *params_resource, uint32_t buffer_id,
+                      int32_t width, int32_t height,
+                      uint32_t drm_format, uint32_t flags)

Argument coding style

@@ +366,3 @@
+                                  dma_buf, NULL);
+  wl_resource_add_destroy_listener (buffer->resource,
+                                    &buffer->destroy_listener);

Not a fan of poking those MetaWaylandBuffer internals from outside. Why not just do

struct wl_resource *resource;

resource = wl_resource_create (...);
wl_resource_set_implementation (...);
buffer = meta_wayland_buffer_from_resource (resource);

? It'd add the listener and construct the MetaWaylandBuffer as is done elsewhere.

@@ +397,3 @@
+               struct wl_resource *params_resource,
+               int32_t width, int32_t height,
+               uint32_t format, uint32_t flags)

Coding style.

@@ +407,3 @@
+                     struct wl_resource *params_resource, uint32_t buffer_id,
+                     int32_t width, int32_t height,
+                     uint32_t format, uint32_t flags)

Here too.

@@ +414,3 @@
+
+static const struct zwp_linux_buffer_params_v1_interface params_implementation = {
+  params_destroy_req,

Call this params_destroy (or maybe even buffer_params_...), and ...

@@ +421,3 @@
+
+static void
+dma_buf_handle_destroy (struct wl_client *client, struct wl_resource *resource)

(coding style)

@@ +429,3 @@
+dma_buf_handle_create_params (struct wl_client *client,
+                              struct wl_resource *dma_buf_resource,
+                              uint32_t params_id)

(coding style)

@@ +442,3 @@
+                        params_id);
+  wl_resource_set_implementation (params_resource, &params_implementation,
+                                  dma_buf, params_destroy);

... call this (wp_linux_)buffer_params_destructor.

@@ +451,3 @@
+
+static void
+send_modifiers (struct wl_resource *resource, uint32_t format)

Argument alignment

@@ +472,3 @@
+  modifiers = g_malloc (sizeof (*modifiers) * num_modifiers);
+  if (!modifiers)
+    return;

Use g_new0() and don't OOM check. We don't support non-over-commit.

@@ +476,3 @@
+  ret = meta_egl_query_dma_buf_modifiers (egl, egl_display, format,
+                                          num_modifiers, modifiers, NULL,
+                                          &num_modifiers, NULL);

Shouldn't we log the error here?

@@ +487,3 @@
+      zwp_linux_dmabuf_v1_send_modifier (resource, format,
+                                         (modifiers[i] >> 32),
+                                         (modifiers[i] & 0xffffffff));

Not sure why you put () around these but ok..

@@ +495,3 @@
+static void
+dma_buf_bind (struct wl_client *client, void *data,
+              uint32_t version, uint32_t id)

(coding style)

@@ +541,3 @@
+
+  for (i = 0; i < META_WAYLAND_DMA_BUF_MAX_FDS; i++)
+    close (dma_buf->fds[i]);

Only close if its a valid fd.

@@ +558,3 @@
+
+static void
+meta_wayland_dma_buf_buffer_class_init(MetaWaylandDmaBufBufferClass *klass)

space before (

::: src/wayland/meta-wayland-egl-stream.c
@@ +27,3 @@
 #include "wayland/meta-wayland-egl-stream.h"
 
+#include "cogl/cogl.h"

Why is this suddenly needed?
Comment 3 Daniel Stone 2017-07-22 12:27:29 UTC
Thanks for the quick review Jonas! I'll fix all the style bits; still trying to get used to the style. At least I've stopped screwing up the braces. :)

(In reply to Jonas Ådahl from comment #2)
> @@ +103,3 @@
> +      break;
> +    case DRM_FORMAT_ARGB2101010:
> +      cogl_format = COGL_PIXEL_FORMAT_ARGB_2101010_PRE;
> 
> Do we actually always support this format?

I assume Cogl always supports it? Short of that, we only advertise it if the EGL implementation reports it as being supported. Except we forget to check if it is, so we never advertise it ... need to fix that.

> ::: src/wayland/meta-wayland-egl-stream.c
> @@ +27,3 @@
>  #include "wayland/meta-wayland-egl-stream.h"
>  
> +#include "cogl/cogl.h"
> 
> Why is this suddenly needed?

Because I started putting it into Cogl out of habit, before I realised it all avoided Cogl now. ;)
Comment 4 Daniel Stone 2017-07-24 15:45:41 UTC
Created attachment 356314 [details] [review]
wayland: Add zwp_linux_dmabuf_v1 support
Comment 5 Daniel Stone 2017-07-24 15:46:05 UTC
Think I've sorted everything you flagged up now.
Comment 6 Jonas Ådahl 2017-08-01 09:54:18 UTC
Review of attachment 356314 [details] [review]:

lgtm. some whitespace issues, but I'll amend those before landing.
Comment 7 Jonas Ådahl 2017-08-01 11:11:52 UTC
Attachment 356314 [details] pushed as b7b5fb2 - wayland: Add zwp_linux_dmabuf_v1 support