On Tue, Jan 07, 2014 at 07:37:11PM +0100, Thomas Hellstrom wrote: > > 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.
Fully agreed. Furthermore there's not even a generic way to allocate dma-bufs (see my other mail) so without a real userspace gfx driver compononent on each side (compositor+client), which then also might need to share other metadata about a buffer I don't really see how this is useful. What's your intended use-case here? -Daniel > > Thanks, > Thomas > > > On 01/07/2014 06:41 PM, [email protected] wrote: > > From: Benjamin Gaignard <[email protected]> > > > > 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 <[email protected]> > > --- > > 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 > _______________________________________________ > wayland-devel mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
