[PATCH weston v2 2/3] compositor-drm: Add scanout support for linux_dmabuf buffers
Fabien DESSENNE
fabien.dessenne at st.com
Tue Oct 4 15:45:19 UTC 2016
Hi
Thank you for the patch. Please, find some minor comments below.
On 09/30/2016 11:28 AM, Tomohito Esaki wrote:
> This implementations bypasses gbm and passes the dmabuf handles directly
> to libdrm for composition.
Maybe you can split the patch in two parts:
- one part dealing with drm_fb_create_dmabuf (since this part is used by
both scanout and sprites)
- a second part dealing with scanout
>
> Signed-off-by: Tomohito Esaki <etom at igel.co.jp>
> ---
> libweston/compositor-drm.c | 125 ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 107 insertions(+), 18 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index a707fc4..b15fa01 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -151,6 +151,9 @@ struct drm_fb {
>
> /* Used by dumb fbs */
> void *map;
> +
> + /* Used by dmabuf */
> + bool is_dmabuf;
> };
>
> struct drm_edid {
> @@ -389,6 +392,76 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
> drmIoctl(fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_arg);
> }
>
> +static inline void
> +close_drm_handle(int fd, uint32_t handle)
> +{
> + struct drm_gem_close gem_close = { .handle = handle };
> + int ret;
> +
> + ret = drmIoctl(fd, DRM_IOCTL_GEM_CLOSE, &gem_close);
> + if (ret)
> + weston_log("DRM_IOCTL_GEM_CLOSE failed.(%s)\n",
> + strerror(errno));
> +}
> +
> +static struct drm_fb *
> +drm_fb_create_dmabuf(struct linux_dmabuf_buffer *dmabuf,
> + struct drm_backend *backend, uint32_t format)
> +{
> + struct drm_fb *fb = NULL;
> + uint32_t width, height, fb_id, handles[4] = {0};
> + int i, ret;
> +
> + if (!format)
> + return NULL;
I do not think that this check is needed (you already checked format in
the caller function + you can delegate this error handling to drmModeAddFB2)
> +
> + width = dmabuf->attributes.width;
> + height = dmabuf->attributes.height;
> + if (backend->min_width > width ||
> + width > backend->max_width ||
> + backend->min_height > height ||
> + height > backend->max_height)
> + return NULL;
Just like in drm_fb_get_from_bo(), add an error log "bo geometry out of
bounds"
> +
> + for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> + ret = drmPrimeFDToHandle(backend->drm.fd,
> + dmabuf->attributes.fd[i],
> + &handles[i]);
> + if (ret)
Add an error log here too (ex "drmPrimeFDToHandle failed")
> + goto done;
> + }
> +
> + ret = drmModeAddFB2(backend->drm.fd, width, height,
> + format, handles, dmabuf->attributes.stride,
> + dmabuf->attributes.offset, &fb_id, 0);
Flags are ignore here (using "0"). Suggesting to use something like this:
if (dmabuf->attributes.flags &
ZWP_LINUX_BUFFER_PARAMS_V1_FLAGS_INTERLACED)
flags |= DRM_MODE_FB_INTERLACED;
> + if (ret)
> + goto done;
Just like in drm_fb_get_from_bo(), add an error log "addfb2 failed"
> +
> + fb = zalloc(sizeof *fb);
> + if (!fb)
> + goto done;
> +
> + fb->fb_id = fb_id;
> + fb->stride = dmabuf->attributes.stride[0];
> + fb->fd = backend->drm.fd;
> + fb->is_dmabuf = true;
> +
> +done:
> + for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> + if (!handles[i])
> + continue;
> + close_drm_handle(backend->drm.fd, handles[i]);
> + }
> +
> + return fb;
> +}
> +
> +static void
> +drm_fb_destroy_dmabuf(struct drm_fb *fb)
> +{
> + drm_fb_destroy(fb, fb->fd);
> +}
> +
> static struct drm_fb *
> drm_fb_get_from_bo(struct gbm_bo *bo,
> struct drm_backend *backend, uint32_t format)
> @@ -475,6 +548,8 @@ drm_output_release_fb(struct drm_output *output, struct drm_fb *fb)
> if (fb->map &&
> (fb != output->dumb[0] && fb != output->dumb[1])) {
> drm_fb_destroy_dumb(fb);
> + } else if (fb->is_dmabuf) {
> + drm_fb_destroy_dmabuf(fb);
> } else if (fb->bo) {
> if (fb->is_client_buffer)
> gbm_bo_destroy(fb->bo);
> @@ -486,12 +561,12 @@ drm_output_release_fb(struct drm_output *output, struct drm_fb *fb)
>
> static uint32_t
> drm_output_check_scanout_format(struct drm_output *output,
> - struct weston_surface *es, struct gbm_bo *bo)
> + struct weston_surface *es, uint32_t format)
> {
> - uint32_t format;
> pixman_region32_t r;
>
> - format = gbm_bo_get_format(bo);
> + /* We relay on the GBM format enum and DRM format enum to be
> + identical */
>
> if (format == GBM_FORMAT_ARGB8888) {
> /* We can scanout an ARGB buffer if the surface's
> @@ -521,12 +596,13 @@ drm_output_prepare_scanout_view(struct drm_output *output,
> struct drm_backend *b = to_drm_backend(output->base.compositor);
> struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
> struct weston_buffer_viewport *viewport = &ev->surface->buffer_viewport;
> - struct gbm_bo *bo;
> + struct linux_dmabuf_buffer *dmabuf;
> + struct gbm_bo *bo = NULL;
> uint32_t format;
>
> if (ev->geometry.x != output->base.x ||
> ev->geometry.y != output->base.y ||
> - buffer == NULL || b->gbm == NULL ||
> + buffer == NULL ||
> buffer->width != output->base.current_mode->width ||
> buffer->height != output->base.current_mode->height ||
> output->base.transform != viewport->buffer.transform ||
> @@ -536,22 +612,35 @@ drm_output_prepare_scanout_view(struct drm_output *output,
> if (ev->geometry.scissor_enabled)
> return NULL;
>
> - bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
> - buffer->resource, GBM_BO_USE_SCANOUT);
> + dmabuf = linux_dmabuf_buffer_get(buffer->resource);
> + if (dmabuf) {
> + format = drm_output_check_scanout_format(
> + output, ev->surface, dmabuf->attributes.format);
> + if (format == 0)
> + return NULL;
>
> - /* Unable to use the buffer for scanout */
> - if (!bo)
> - return NULL;
> + output->next = drm_fb_create_dmabuf(dmabuf, b, format);
> + if (!output->next)
> + return NULL;
> + } else if (b->gbm) {
> + bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
> + buffer->resource, GBM_BO_USE_SCANOUT);
>
> - format = drm_output_check_scanout_format(output, ev->surface, bo);
> - if (format == 0) {
> - gbm_bo_destroy(bo);
> - return NULL;
> - }
> + /* Unable to use the buffer for scanout */
> + if (!bo)
> + return NULL;
>
> - output->next = drm_fb_get_from_bo(bo, b, format);
> - if (!output->next) {
> - gbm_bo_destroy(bo);
> + format = drm_output_check_scanout_format(
> + output, ev->surface, gbm_bo_get_format(bo));
> + if (format == 0)
> + return NULL;
> +
> + output->next = drm_fb_get_from_bo(bo, b, format);
> + if (!output->next) {
> + gbm_bo_destroy(bo);
> + return NULL;
> + }
> + } else {
> return NULL;
> }
>
More information about the wayland-devel
mailing list