Re: [PATCH v16 15/23] compositor-drm: Support modifiers with GBM
On Thu, 5 Jul 2018 18:16:42 +0100
Daniel Stone wrote:
> Use the extended GBM allocator interface to support modifiers and
> multi-planar BOs.
>
> Signed-off-by: Daniel Stone
> Tested-by: Emre Ucan
> ---
> configure.ac | 3 ++
> libweston/compositor-drm.c | 61 +++---
> 2 files changed, 53 insertions(+), 11 deletions(-)
>
Hi,
I feel there are two parts in this patch which are not clearly
discussed in the commit message. The changes to drm_fb_get_from_bo()
affect all three of BUFFER_CLIENT, BUFFER_GBM_SURFACE and BUFFER_CURSOR
cases. The changes to drm_output_init_egl() affect only the
BUFFER_GBM_SURFACE case.
If I understand right, this mainly affects GBM import from wl_buffer
and GBM surface use cases.
Is this the final patch that makes etnaviv work? That would be nice to
mention.
> diff --git a/configure.ac b/configure.ac
> index c550198ae..357b6471e 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])])
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 3f8e77554..3471d9b0f 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -871,7 +871,7 @@ drm_fb_addfb(struct drm_fb *fb)
> int ret = -EINVAL;
> #ifdef HAVE_DRM_ADDFB2_MODIFIERS
> uint64_t mods[4] = { };
> - int i;
> + size_t i;
> #endif
>
> /* If we have a modifier set, we must only use the WithModifiers
> @@ -880,7 +880,7 @@ drm_fb_addfb(struct drm_fb *fb)
> /* KMS demands that if a modifier is set, it must be the same
>* for all planes. */
> #ifdef HAVE_DRM_ADDFB2_MODIFIERS
> - for (i = 0; i < (int) ARRAY_LENGTH(mods) && fb->handles[i]; i++)
> + for (i = 0; i < ARRAY_LENGTH(mods) && fb->handles[i]; i++)
This is an unrelated change and would ideally be in a separate patch so
it could land ahead of time.
> mods[i] = fb->modifier;
> ret = drmModeAddFB2WithModifiers(fb->fd, fb->width, fb->height,
>fb->format->format,
> @@ -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;
Making the size zero should have a rationale in the commit message.
> +
> +#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",
> @@ -4212,13 +4225,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)
> + break;
> + }
> +
> + if (i == plane->count_formats) {
> +
Re: [PATCH v16 15/23] compositor-drm: Support modifiers with GBM
Hey Emil, On Fri, 6 Jul 2018 at 11:02, Emil Velikov wrote: > On 5 July 2018 at 18:16, Daniel Stone wrote: > > Use the extended GBM allocator interface to support modifiers and > > multi-planar BOs. > > Instead of such lovely checks and multiple #ifdef blocks through in > the code, one can use weak symbols. > See kmscube code has some examples. > > That said, it's something that could be handled at a later stage. > There's no point in delaying the series over that detail. Yeah, all the options are lovely in their own way. I'd happily check and review a patch to make it use weak functions to cover all the new GBM API though. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v16 15/23] compositor-drm: Support modifiers with GBM
Hi Dan, On 5 July 2018 at 18:16, Daniel Stone wrote: > Use the extended GBM allocator interface to support modifiers and > multi-planar BOs. > > Signed-off-by: Daniel Stone > Tested-by: Emre Ucan > --- > configure.ac | 3 ++ > libweston/compositor-drm.c | 61 +++--- > 2 files changed, 53 insertions(+), 11 deletions(-) > > diff --git a/configure.ac b/configure.ac > index c550198ae..357b6471e 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])]) Instead of such lovely checks and multiple #ifdef blocks through in the code, one can use weak symbols. See kmscube code has some examples. That said, it's something that could be handled at a later stage. There's no point in delaying the series over that detail. Thanks Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v16 15/23] compositor-drm: Support modifiers with GBM
Use the extended GBM allocator interface to support modifiers and
multi-planar BOs.
Signed-off-by: Daniel Stone
Tested-by: Emre Ucan
---
configure.ac | 3 ++
libweston/compositor-drm.c | 61 +++---
2 files changed, 53 insertions(+), 11 deletions(-)
diff --git a/configure.ac b/configure.ac
index c550198ae..357b6471e 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])])
diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 3f8e77554..3471d9b0f 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -871,7 +871,7 @@ drm_fb_addfb(struct drm_fb *fb)
int ret = -EINVAL;
#ifdef HAVE_DRM_ADDFB2_MODIFIERS
uint64_t mods[4] = { };
- int i;
+ size_t i;
#endif
/* If we have a modifier set, we must only use the WithModifiers
@@ -880,7 +880,7 @@ drm_fb_addfb(struct drm_fb *fb)
/* KMS demands that if a modifier is set, it must be the same
* for all planes. */
#ifdef HAVE_DRM_ADDFB2_MODIFIERS
- for (i = 0; i < (int) ARRAY_LENGTH(mods) && fb->handles[i]; i++)
+ for (i = 0; i < ARRAY_LENGTH(mods) && fb->handles[i]; i++)
mods[i] = fb->modifier;
ret = drmModeAddFB2WithModifiers(fb->fd, fb->width, fb->height,
fb->format->format,
@@ -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",
@@ -4212,13 +4225,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)
+ break;
+ }
+
+ if (i == plane->count_formats) {
+ weston_log("format 0x%x not supported by output %s\n",
+ output->gbm_format, output->base.name);
+ 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,
+ output->gbm_format,
+
plane->formats[i].modifiers,
+
plane->formats[i].count_modifiers);
+ } else
+#endif
+ {
+
