[PATCH weston v2 2/3] compositor-drm: Add scanout support for linux_dmabuf buffers

[Eric Engestrom]

On Fri, Sep 30, 2016 at 06:28:52PM +0900, Tomohito Esaki wrote:
> This implementations bypasses gbm and passes the dmabuf handles directly
> to libdrm for composition.
> 
> 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;
> +
> +	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;
> +
> +	for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> +		ret = drmPrimeFDToHandle(backend->drm.fd,
> +					 dmabuf->attributes.fd[i],
> +					 &handles[i]);
> +		if (ret)
> +			goto done;
> +	}
> +
> +	ret = drmModeAddFB2(backend->drm.fd, width, height,
> +			    format, handles, dmabuf->attributes.stride,
> +			    dmabuf->attributes.offset, &fb_id, 0);
> +	if (ret)
> +		goto done;
> +
> +	fb = zalloc(sizeof *fb);
> +	if (!fb)
> +	    goto done;

Nit: indentation

> +
> +	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]);

No need for `continue`, you can remove it and invert the if.

> +	}
> +
> +	return fb;
> +}
> +
> +static void
> +drm_fb_destroy_dmabuf(struct drm_fb *fb)
> +{
> +	drm_fb_destroy(fb, fb->fd);
> +}

Not sure this is really needed :)

> +
>  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

"rely"

Otherwise it looks good :)

Cheers,
  Eric

> +	   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;
>  	}
>  
> -- 
> 2.7.4