[PATCH v14 41/41] compositor-drm: Support modifiers with GBM

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:58 +0000
Daniel Stone <daniels at collabora.com> wrote:

> Use the extended GBM allocator interface to support modifiers and
> multi-planar BOs.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  configure.ac               |  3 +++
>  libweston/compositor-drm.c | 57 ++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index bc6fefc1e..9bad4e25a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -215,6 +215,9 @@ if test x$enable_drm_compositor = xyes; then
>    PKG_CHECK_MODULES(DRM_COMPOSITOR_FORMATS_BLOB, [libdrm >= 2.4.83],
>  		    [AC_DEFINE([HAVE_DRM_FORMATS_BLOB], 1, [libdrm supports modifier advertisement])],
>  		    [AC_MSG_WARN([libdrm does not support modifier advertisement])])
> +  PKG_CHECK_MODULES(DRM_COMPOSITOR_GBM_MODIFIERS, [gbm >= 17.1],
> +		    [AC_DEFINE([HAVE_GBM_MODIFIERS], 1, [GBM supports modifiers])],
> +		    [AC_MSG_WARN([GBM does not support modifiers])])
>    PKG_CHECK_MODULES(DRM_COMPOSITOR_GBM, [gbm >= 17.2],
>  		    [AC_DEFINE([HAVE_GBM_FD_IMPORT], 1, [gbm supports import with modifiers])],
>  		    [AC_MSG_WARN([GBM does not support dmabuf import, will omit that capability])])

We're getting so many feature toggles here that I'm beginning to wonder
if we should log them at runtime as well, to help debugging.

> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index ba2217fc0..65e24cbb2 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1114,6 +1114,9 @@ drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_backend *backend,
>  		   bool is_opaque, enum drm_fb_type type)
>  {
>  	struct drm_fb *fb = gbm_bo_get_user_data(bo);
> +#ifdef HAVE_GBM_MODIFIERS
> +	int i;
> +#endif
>  
>  	if (fb) {
>  		assert(fb->type == type);
> @@ -1127,15 +1130,25 @@ drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_backend *backend,
>  	fb->type = type;
>  	fb->refcnt = 1;
>  	fb->bo = bo;
> +	fb->fd = backend->drm.fd;
>  
>  	fb->width = gbm_bo_get_width(bo);
>  	fb->height = gbm_bo_get_height(bo);
> +	fb->format = pixel_format_get_info(gbm_bo_get_format(bo));
> +	fb->size = 0;
> +
> +#ifdef HAVE_GBM_MODIFIERS
> +	fb->modifier = gbm_bo_get_modifier(bo);
> +	for (i = 0; i < gbm_bo_get_plane_count(bo); i++) {
> +		fb->strides[i] = gbm_bo_get_stride_for_plane(bo, i);
> +		fb->handles[i] = gbm_bo_get_handle_for_plane(bo, i).u32;
> +		fb->offsets[i] = gbm_bo_get_offset(bo, i);
> +	}
> +#else
>  	fb->strides[0] = gbm_bo_get_stride(bo);
>  	fb->handles[0] = gbm_bo_get_handle(bo).u32;
> -	fb->format = pixel_format_get_info(gbm_bo_get_format(bo));
>  	fb->modifier = DRM_FORMAT_MOD_INVALID;
> -	fb->size = fb->strides[0] * fb->height;
> -	fb->fd = backend->drm.fd;
> +#endif
>  
>  	if (!fb->format) {
>  		weston_log("couldn't look up format 0x%lx\n",
> @@ -4346,13 +4359,39 @@ drm_output_init_egl(struct drm_output *output, struct drm_backend *b)
>  		fallback_format_for(output->gbm_format),
>  	};
>  	int n_formats = 1;
> +	struct weston_mode *mode = output->base.current_mode;
> +	struct drm_plane *plane = output->scanout_plane;
> +	unsigned int i;
> +
> +	for (i = 0; i < plane->count_formats; i++) {
> +		if (plane->formats[i].format == output->gbm_format)

Here it is output->gbm_format...

> +			break;
> +	}
> +
> +	if (i == plane->count_formats) {
> +		/* XXX: better error message */
> +		weston_log("can't use format for output\n");

XXX :-)

Maybe I could add name strings to the pixel format list and a helper
that would be safe for arbitrary and even invalid format codes for
turning a format into pretty string.

Should this fall back to the fallback format as well?

> +		return -1;
> +	}
> +
> +#ifdef HAVE_GBM_MODIFIERS
> +	if (plane->formats[i].count_modifiers > 0) {
> +		output->gbm_surface =
> +			gbm_surface_create_with_modifiers(b->gbm,
> +							  mode->width,
> +							  mode->height,
> +							  format[0],

..and here it is format[0]. I'd prefer more consistency, or even
plane->formats[i].format.

> +							  plane->formats[i].modifiers,
> +							  plane->formats[i].count_modifiers);
> +	} else
> +#endif
> +	{
> +		output->gbm_surface =
> +		    gbm_surface_create(b->gbm, mode->width, mode->height,
> +				       format[0],
> +				       GBM_BO_USE_RENDERING | GBM_BO_USE_SCANOUT);
> +	}
>  
> -	output->gbm_surface = gbm_surface_create(b->gbm,
> -					     output->base.current_mode->width,
> -					     output->base.current_mode->height,
> -					     format[0],
> -					     GBM_BO_USE_SCANOUT |
> -					     GBM_BO_USE_RENDERING);
>  	if (!output->gbm_surface) {
>  		weston_log("failed to create gbm surface\n");
>  		return -1;

This is not new in this patch, but the GBM surface is created with
gbm_format, but the EGLConfig may be chosen by gbm_format or its
fallback format. If it picks with the fallback format, do we have a
problem?


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180130/41646f6e/attachment.sig>