Re: [Linaro-mm-sig] [PATCH v4 3/3] compositor-drm: allow to be a wl_dmabuf server

I think dma-buf mmap() used without care like this should be avoided at all cost, because it will make people happily start using it without thinking about non-coherent architectures where mmap would be painfully inefficient: This is because when the accelerator uses the dma-buf it has no idea what part of it was dirtied and may need to flush a much larger region than was actually touched by the CPU. Think of pixman writing a single char to a 1280x1024 canvas, which is then used by the compositor. A smart driver non-coherent would need to flush all pages touched. A not-so-smart driver would need to flush the whole buffer. For a single character.

Also even if a coherent driver implements mmap(), the underlying dma-buf may reside in write-combined memory and using it for compositing with pixman may actually be much slower than shmem, even with the extra shmem copy.

Conclusion: Avoid using dma-buf mmap() outside of drivers that know exactly what they are doing, and avoid it at all cost.

In particular, don't build it into generic code like this.

Thanks, Thomas

On 01/07/2014 06:41 PM, benjamin.gaignard@linaro.org wrote:

From: Benjamin Gaignard benjamin.gaignard@linaro.org

Make drm compositor aware of the wl_dmabuf protocol if pixman is used. Add functions to have a wl_dmabuf server inside drm compositor. Change pixman to let it know how use a wl_dmabuf buffer.

Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org

src/compositor-drm.c | 83 ++++++++++++++++++++++++++++++++++++++++--- src/compositor.c | 4 ++- src/compositor.h | 2 ++ src/pixman-renderer.c | 93 ++++++++++++++++++++++++++++++++++++------------- 4 files changed, 152 insertions(+), 30 deletions(-)

diff --git a/src/compositor-drm.c b/src/compositor-drm.c index 4f015d1..669ee45 100644 --- a/src/compositor-drm.c +++ b/src/compositor-drm.c @@ -49,6 +49,8 @@ #include "udev-seat.h" #include "launcher-util.h" #include "vaapi-recorder.h" +#include "wayland-dmabuf.h" +#include "wayland-dmabuf-server-protocol.h" #ifndef DRM_CAP_TIMESTAMP_MONOTONIC #define DRM_CAP_TIMESTAMP_MONOTONIC 0x6 @@ -826,7 +828,8 @@ drm_output_prepare_overlay_surface(struct weston_output *output_base, if (es->alpha != 1.0f) return NULL;

• if (wl_shm_buffer_get(es->buffer_ref.buffer->resource))

• if (wl_shm_buffer_get(es->buffer_ref.buffer->resource)

• || wl_dmabuf_buffer_get(es->buffer_ref.buffer->resource))
  

  return NULL;

if (!drm_surface_transform_supported(es)) @@ -951,7 +954,8 @@ drm_output_prepare_cursor_surface(struct weston_output *output_base, if (c->cursors_are_broken) return NULL; if (es->buffer_ref.buffer == NULL ||

•    !wl_shm_buffer_get(es->buffer_ref.buffer->resource) ||
  

•    (!wl_shm_buffer_get(es->buffer_ref.buffer->resource) &&
  

• !wl_dmabuf_buffer_get(es->buffer_ref.buffer->resource))||
  

  es->geometry.width > 64 || es->geometry.height > 64) return NULL;

@@ -985,12 +989,25 @@ drm_output_set_cursor(struct drm_output *output) output->current_cursor ^= 1; bo = output->cursor_bo[output->current_cursor]; memset(buf, 0, sizeof buf);

• stride = wl_shm_buffer_get_stride(es->buffer_ref.buffer->shm_buffer);
  

• s = wl_shm_buffer_get_data(es->buffer_ref.buffer->shm_buffer);
  

• if (wl_shm_buffer_get(es->buffer_ref.buffer->resource)) {
  

• 	stride = wl_shm_buffer_get_stride(es->buffer_ref.buffer->shm_buffer);
  

• 	s = wl_shm_buffer_get_data(es->buffer_ref.buffer->shm_buffer);
  

• }
  

• if (wl_dmabuf_buffer_get(es->buffer_ref.buffer->resource)) {
  

• 	stride = wl_dmabuf_buffer_get_stride(es->buffer_ref.buffer->dmabuf_buffer);
  

• 	s = wl_dmabuf_buffer_get_data(es->buffer_ref.buffer->dmabuf_buffer);
  

• 	if (!s) {
  

• 		weston_log("failed to get data from dmabuf buffer");
  

• 		return;
  

• 	}
  

• }
  

  for (i = 0; i < es->geometry.height; i++) memcpy(buf + i * 64, s + i * stride, es->geometry.width * 4);

• if (wl_dmabuf_buffer_get(es->buffer_ref.buffer->resource))
  

• 	wl_dmabuf_buffer_put_data(es->buffer_ref.buffer->dmabuf_buffer);
  

• if (gbm_bo_write(bo, buf, sizeof buf) < 0) weston_log("failed update cursor: %m\n");

@@ -1044,7 +1061,8 @@ drm_assign_planes(struct weston_output *output) * non-shm, or small enough to be a cursor */ if ((es->buffer_ref.buffer &&

•      !wl_shm_buffer_get(es->buffer_ref.buffer->resource)) ||
  

•      (!wl_shm_buffer_get(es->buffer_ref.buffer->resource))
  

• 	 && !wl_dmabuf_buffer_get(es->buffer_ref.buffer->resource)) ||
  

  (es->geometry.width <= 64 && es->geometry.height <= 64)) es->keep_buffer = 1; else

@@ -1268,6 +1286,57 @@ init_drm(struct drm_compositor *ec, struct udev_device *device) return 0; } +static void dmabuf_send_supported_formats(void *user_data, struct wl_resource *resource) +{

• struct drm_compositor *ec = user_data;
• drmModePlaneRes *plane_resources;
• unsigned int i, j;
• weston_log("send supported formats\n");
• plane_resources = drmModeGetPlaneResources(ec->drm.fd);
• if (!plane_resources)

• return;
  

• for (i = 0; i < plane_resources->count_planes; i++) {

• drmModePlane *ovr = drmModeGetPlane(ec->drm.fd, plane_resources->planes[i]);
  

• for (j = 0 ; j < ovr->count_formats; j++) {
  

• 	weston_log("server support format %4.4s\n", (char *)&ovr->formats[j]);
  

• 	wl_dmabuf_send_format(resource, ovr->formats[j]);
  

• }
  

• drmModeFreePlane(ovr);
  

• }
• drmModeFreePlaneResources(plane_resources);

+}

+static void dmabuf_send_device_name(void *user_data, struct wl_resource *resource) +{

• struct drm_compositor *ec = user_data;
• weston_log("send device name %s\n", ec->drm.filename);
• wl_dmabuf_send_device(resource, ec->drm.filename);

+}

+static void dmabuf_send_server_info(void *user_data, struct wl_resource *resource) +{

• dmabuf_send_supported_formats(user_data, resource);
• dmabuf_send_device_name(user_data, resource);

+}

+static struct wl_dmabuf_callbacks wl_dmabuf_callbacks = {

• dmabuf_send_server_info

+};

+static int +init_dmabuf_protocol(struct drm_compositor *ec) +{

• wl_dmabuf_init(ec->base.wl_display,

• 	&wl_dmabuf_callbacks, ec, 0);
  

• return 0;

+}

static int init_egl(struct drm_compositor *ec) { @@ -1955,6 +2024,10 @@ create_output_for_connector(struct drm_compositor *ec, weston_log("Failed to init output pixman state\n"); goto err_output; }

• if (init_dmabuf_protocol(ec) < 0) {
  

• 	weston_log("Failed to init wl_dmabuf server\n");
  

• 	goto err_output;
  

• }
  

  } else if (drm_output_init_egl(output, ec) < 0) { weston_log("Failed to init output gl state\n"); goto err_output;

diff --git a/src/compositor.c b/src/compositor.c index 4cb44e5..037b695 100644 --- a/src/compositor.c +++ b/src/compositor.c @@ -1257,7 +1257,9 @@ surface_accumulate_damage(struct weston_surface *surface, pixman_region32_t *opaque) { if (surface->buffer_ref.buffer &&

•    wl_shm_buffer_get(surface->buffer_ref.buffer->resource))
  

•    (wl_shm_buffer_get(surface->buffer_ref.buffer->resource)
  

• || wl_dmabuf_buffer_get(surface->buffer_ref.buffer->resource))
  

• )
  

  surface->compositor->renderer->flush_damage(surface);

if (surface->transform.enabled) { diff --git a/src/compositor.h b/src/compositor.h index 0dcb604..537a9b7 100644 --- a/src/compositor.h +++ b/src/compositor.h @@ -33,6 +33,7 @@ extern "C" { #define WL_HIDE_DEPRECATED #include <wayland-server.h> +#include <wayland-dmabuf.h> #include "version.h" #include "matrix.h" @@ -626,6 +627,7 @@ struct weston_buffer { union { struct wl_shm_buffer *shm_buffer;

• struct wl_dmabuf_buffer *dmabuf_buffer;
  

  void *legacy_buffer; }; int32_t width, height;

diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c index 987c539..0a88c58 100644 --- a/src/pixman-renderer.c +++ b/src/pixman-renderer.c @@ -528,11 +528,19 @@ pixman_renderer_flush_damage(struct weston_surface *surface) /* No-op for pixman renderer */ } +static void wl_dmabuf_pixman_destroy (pixman_image_t *image, void *data) +{

• struct wl_dmabuf_buffer *dmabuf_buffer = data;
• wl_dmabuf_buffer_put_data(dmabuf_buffer);

+}

static void pixman_renderer_attach(struct weston_surface *es, struct weston_buffer *buffer) { struct pixman_surface_state *ps = get_surface_state(es); struct wl_shm_buffer *shm_buffer;

• struct wl_dmabuf_buffer *dmabuf_buffer;
• pixman_format_code_t pixman_format;

weston_buffer_reference(&ps->buffer_ref, buffer); @@ -544,40 +552,77 @@ pixman_renderer_attach(struct weston_surface *es, struct weston_buffer *buffer) if (!buffer) return;

• shm_buffer = wl_shm_buffer_get(buffer->resource);
• dmabuf_buffer = wl_dmabuf_buffer_get(buffer->resource);

• if (! shm_buffer) {

• weston_log("Pixman renderer supports only SHM buffers\n");
  

• if (!shm_buffer && !dmabuf_buffer) {

• weston_log("Pixman renderer supports only SHM and DMABUF buffers\n");
  

  weston_buffer_reference(&ps->buffer_ref, NULL); return; }

• switch (wl_shm_buffer_get_format(shm_buffer)) {
• case WL_SHM_FORMAT_XRGB8888:

• pixman_format = PIXMAN_x8r8g8b8;
  

• break;
  

• case WL_SHM_FORMAT_ARGB8888:

• pixman_format = PIXMAN_a8r8g8b8;
  

• break;
  

• case WL_SHM_FORMAT_RGB565:

• pixman_format = PIXMAN_r5g6b5;
  

• if (shm_buffer) {

• switch (wl_shm_buffer_get_format(shm_buffer)) {
  

• case WL_SHM_FORMAT_XRGB8888:
  

• 	pixman_format = PIXMAN_x8r8g8b8;
  

• 	break;
  

• case WL_SHM_FORMAT_ARGB8888:
  

• 	pixman_format = PIXMAN_a8r8g8b8;
  

• 	break;
  

• case WL_SHM_FORMAT_RGB565:
  

• 	pixman_format = PIXMAN_r5g6b5;
  

• 	break;
  

• default:
  

• 	weston_log("Unsupported SHM buffer format\n");
  

• 	weston_buffer_reference(&ps->buffer_ref, NULL);
  

• 	return;
  

  break;

• default:

• weston_log("Unsupported SHM buffer format\n");
  

• weston_buffer_reference(&ps->buffer_ref, NULL);
  

• return;
  

• break;

• }
  

• buffer->shm_buffer = shm_buffer;
  

• buffer->width = wl_shm_buffer_get_width(shm_buffer);
  

• buffer->height = wl_shm_buffer_get_height(shm_buffer);
  

• ps->image = pixman_image_create_bits(pixman_format,
  

• 	buffer->width, buffer->height,
  

• 	wl_shm_buffer_get_data(shm_buffer),
  

• 	wl_shm_buffer_get_stride(shm_buffer));
  

  }

• buffer->shm_buffer = shm_buffer;
• buffer->width = wl_shm_buffer_get_width(shm_buffer);
• buffer->height = wl_shm_buffer_get_height(shm_buffer);

• if (dmabuf_buffer) {

• void *data;
  

• switch (wl_dmabuf_buffer_get_format(dmabuf_buffer)) {
  

• case WL_DMABUF_FORMAT_XRGB8888:
  

• 	pixman_format = PIXMAN_x8r8g8b8;
  

• 	break;
  

• case WL_DMABUF_FORMAT_ARGB8888:
  

• 	pixman_format = PIXMAN_a8r8g8b8;
  

• 	break;
  

• case WL_DMABUF_FORMAT_RGB565:
  

• 	pixman_format = PIXMAN_r5g6b5;
  

• 	break;
  

• default:
  

• 	weston_log("Unsupported DMABUF buffer format\n");
  

• 	weston_buffer_reference(&ps->buffer_ref, NULL);
  

• 	return;
  

• break;
  

• }
  

• ps->image = pixman_image_create_bits(pixman_format,

• buffer->width, buffer->height,
  

• wl_shm_buffer_get_data(shm_buffer),
  

• wl_shm_buffer_get_stride(shm_buffer));
  

• buffer->dmabuf_buffer = dmabuf_buffer;
  

• buffer->width = wl_dmabuf_buffer_get_width(dmabuf_buffer);
  

• buffer->height = wl_dmabuf_buffer_get_height(dmabuf_buffer);
  

• data = wl_dmabuf_buffer_get_data(dmabuf_buffer);
  

• if (data) {
  

• 	ps->image = pixman_image_create_bits(pixman_format,
  

• 		buffer->width, buffer->height, data,
  

• 		wl_dmabuf_buffer_get_stride(dmabuf_buffer));
  

• 	pixman_image_set_destroy_function(ps->image, wl_dmabuf_pixman_destroy, dmabuf_buffer);
  

• } else
  

• 	weston_log("failed to get data from dmabuf buffer");
  

• }

} static int