Re: [PATCH v14 40/41] compositor-drm: Support plane IN_FORMATS
On Tue, 10 Jul 2018 08:41:46 +0100 Daniel Stone wrote: > Hi Pekka, > > On Mon, 29 Jan 2018 at 15:01, Pekka Paalanen wrote: > > On Wed, 20 Dec 2017 12:26:57 + Daniel Stone > > wrote: > > > @@ -212,6 +212,9 @@ if test x$enable_drm_compositor = xyes; then > > >PKG_CHECK_MODULES(DRM_COMPOSITOR_ATOMIC, [libdrm >= 2.4.78], > > > [AC_DEFINE([HAVE_DRM_ATOMIC], 1, [libdrm supports > > > atomic API])], > > > [AC_MSG_WARN([libdrm does not support atomic > > > modesetting, will omit that capability])]) > > > + 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, [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])]) > > > > Hi, > > > > I wonder, we are getting more and more of these "libdrm has ..." > > feature checks. How about merging some to produce fewer build types? > > Just a general idea, not specifically for this patch. > > Assuming enough distros support the new bits, I'd be open to having > two levels of libdrm support: ancient or supports-everything. > Annoyingly the gbm check is separate, but I suppose we could connect, > e.g., both of libdrm >= 2.4.83 + gbm >= 17.2 to modifiers, rather than > having modifiers half-supported when only one of them is sufficiently > new. That sounds reasonable to me. Now that we've heard of the weak functions trick, I'd be happy to remove some autoconf checks for library functions and replace them with runtime weak function checks that would also be reported in the log. Maybe that can cut the number of #ifdefs and checks. Let's keep these ideas for follow-up. They would help the Meson migration as well, having less dependency checks to cross-check. > > > + unsigned i, j; > > > + drmModePropertyBlobRes *blob; > > > + struct drm_format_modifier_blob *fmt_mod_blob; > > > + uint32_t *blob_formats; > > > + struct drm_format_modifier *blob_modifiers; > > > + > > > + blob = drmModeGetPropertyBlob(plane->backend->drm.fd, blob_id); > > > + if (!blob) > > > + return false; > > > + > > > + fmt_mod_blob = blob->data; > > > > Should we not check that fmt_mod_blob.version == 1? > > No. Future versions can add new bits to the end of the blob, but they > can't break compatibility - doing so would require a new property > name. Usually DRM uses size to do this, but as we have two > variable-length arrays whose size can't be trivially determined by the > client, we use version to indicate any possible future extensions to > the structure, and the offset fields to point to the VLAs. > > The current version is 1; I guess we could check for version 0 and > bail out if we see it, but no kernel has ever had it and it seems a > bit pathological. Future versions will still be parseable as the > original, so those are fine. Understood. Thanks, pq pgp_1lW0E5AnD.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v14 40/41] compositor-drm: Support plane IN_FORMATS
Hi Pekka,
On Mon, 29 Jan 2018 at 15:01, Pekka Paalanen wrote:
> On Wed, 20 Dec 2017 12:26:57 + Daniel Stone wrote:
> > @@ -212,6 +212,9 @@ if test x$enable_drm_compositor = xyes; then
> >PKG_CHECK_MODULES(DRM_COMPOSITOR_ATOMIC, [libdrm >= 2.4.78],
> > [AC_DEFINE([HAVE_DRM_ATOMIC], 1, [libdrm supports atomic
> > API])],
> > [AC_MSG_WARN([libdrm does not support atomic modesetting,
> > will omit that capability])])
> > + 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, [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])])
>
> Hi,
>
> I wonder, we are getting more and more of these "libdrm has ..."
> feature checks. How about merging some to produce fewer build types?
> Just a general idea, not specifically for this patch.
Assuming enough distros support the new bits, I'd be open to having
two levels of libdrm support: ancient or supports-everything.
Annoyingly the gbm check is separate, but I suppose we could connect,
e.g., both of libdrm >= 2.4.83 + gbm >= 17.2 to modifiers, rather than
having modifiers half-supported when only one of them is sufficiently
new.
> > +#ifdef HAVE_DRM_FORMATS_BLOB
> > +static inline uint32_t *
> > +formats_ptr(struct drm_format_modifier_blob *blob)
> > +{
> > + return (uint32_t *)(((char *)blob) + blob->formats_offset);
> > +}
> > +
> > +static inline struct drm_format_modifier *
> > +modifiers_ptr(struct drm_format_modifier_blob *blob)
> > +{
> > + return (struct drm_format_modifier *)(((char *)blob) +
> > blob->modifiers_offset);
> > +}
>
> These two functions are unlikely to be used anywhere else than
> populate_format_modifiers(), so they might as well be in the same #ifdef
> block. If even functions at all. A long line.
Both done.
> > @@ -3635,6 +3667,61 @@ init_pixman(struct drm_backend *b)
> > return pixman_renderer_init(b->compositor);
> > }
> >
> > +/**
> > + * Populates the formats array, and the modifiers of each format for a
> > drm_plane.
> > + */
> > +#ifdef HAVE_DRM_FORMATS_BLOB
> > +static bool
> > +populate_format_modifiers(struct drm_plane *plane, const drmModePlane
> > *kplane,
> > + uint32_t blob_id)
>
> I think this should be:
>
> static int
> drm_plane_populate_format_modifiers(...
>
> I believe we tend to use int for success/failure returns, where
> negative is a failure. Boolean return values are more used for
> functions that return a truth value, like drm_device_is_kms().
Both done.
> > + unsigned i, j;
> > + drmModePropertyBlobRes *blob;
> > + struct drm_format_modifier_blob *fmt_mod_blob;
> > + uint32_t *blob_formats;
> > + struct drm_format_modifier *blob_modifiers;
> > +
> > + blob = drmModeGetPropertyBlob(plane->backend->drm.fd, blob_id);
> > + if (!blob)
> > + return false;
> > +
> > + fmt_mod_blob = blob->data;
>
> Should we not check that fmt_mod_blob.version == 1?
No. Future versions can add new bits to the end of the blob, but they
can't break compatibility - doing so would require a new property
name. Usually DRM uses size to do this, but as we have two
variable-length arrays whose size can't be trivially determined by the
client, we use version to indicate any possible future extensions to
the structure, and the offset fields to point to the VLAs.
The current version is 1; I guess we could check for version 0 and
bail out if we see it, but no kernel has ever had it and it seems a
bit pathological. Future versions will still be parseable as the
original, so those are fine.
> > + blob_formats = formats_ptr(fmt_mod_blob);
> > + blob_modifiers = modifiers_ptr(fmt_mod_blob);
> > +
> > + assert(plane->count_formats == fmt_mod_blob->count_formats);
> > +
> > + for (i = 0; i < fmt_mod_blob->count_formats; i++) {
> > + uint32_t count_modifiers = 0;
> > + uint64_t *modifiers = NULL;
> > +
> > + for (j = 0; j < fmt_mod_blob->count_modifiers; j++) {
> > + struct drm_format_modifier *mod = &blob_modifiers[j];
> > +
> > + if ((i < mod->offset) || (i > mod->offset + 63))
> > + continue;
> > + if (!(mod->formats & (1 << (i - mod->offset
> > + continue;
> > +
> > + modifiers = realloc(modifiers, (count_modifiers + 1)
> > * sizeof(modifiers[0]));
>
> Split line, please.
Done.
> > + if (!modifiers) {
>
> Realloc is a bit inconvenient in that if it fails, the original
> allocation
Re: [PATCH v14 40/41] compositor-drm: Support plane IN_FORMATS
On Wed, 20 Dec 2017 12:26:57 +
Daniel Stone wrote:
> From: Sergi Granell
>
> The per-plane IN_FORMATS property adds information about format
> modifiers; we can use these to both feed GBM with the set of modifiers
> we want to use for rendering, and also as an early-out test when we're
> trying to see if a FB will go on a particular plane.
>
> Signed-off-by: Sergi Granell
> Reviewed-by: Daniel Stone
> Signed-off-by: Daniel Stone
> ---
> configure.ac | 3 ++
> libweston/compositor-drm.c | 128
> +
> 2 files changed, 122 insertions(+), 9 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 8d0d6582a..bc6fefc1e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -212,6 +212,9 @@ if test x$enable_drm_compositor = xyes; then
>PKG_CHECK_MODULES(DRM_COMPOSITOR_ATOMIC, [libdrm >= 2.4.78],
> [AC_DEFINE([HAVE_DRM_ATOMIC], 1, [libdrm supports atomic
> API])],
> [AC_MSG_WARN([libdrm does not support atomic modesetting,
> will omit that capability])])
> + 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, [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])])
Hi,
I wonder, we are getting more and more of these "libdrm has ..."
feature checks. How about merging some to produce fewer build types?
Just a general idea, not specifically for this patch.
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index cb1c23b03..ba2217fc0 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -83,6 +83,20 @@
> #define GBM_BO_USE_CURSOR GBM_BO_USE_CURSOR_64X64
> #endif
>
> +#ifdef HAVE_DRM_FORMATS_BLOB
> +static inline uint32_t *
> +formats_ptr(struct drm_format_modifier_blob *blob)
> +{
> + return (uint32_t *)(((char *)blob) + blob->formats_offset);
> +}
> +
> +static inline struct drm_format_modifier *
> +modifiers_ptr(struct drm_format_modifier_blob *blob)
> +{
> + return (struct drm_format_modifier *)(((char *)blob) +
> blob->modifiers_offset);
> +}
These two functions are unlikely to be used anywhere else than
populate_format_modifiers(), so they might as well be in the same #ifdef
block. If even functions at all. A long line.
> +#endif
> +
> /**
> * Represents the values of an enum-type KMS property
> */
> @@ -127,6 +141,7 @@ enum wdrm_plane_property {
> WDRM_PLANE_CRTC_H,
> WDRM_PLANE_FB_ID,
> WDRM_PLANE_CRTC_ID,
> + WDRM_PLANE_IN_FORMATS,
> WDRM_PLANE__COUNT
> };
>
> @@ -168,6 +183,7 @@ static const struct drm_property_info plane_props[] = {
> [WDRM_PLANE_CRTC_H] = { .name = "CRTC_H", },
> [WDRM_PLANE_FB_ID] = { .name = "FB_ID", },
> [WDRM_PLANE_CRTC_ID] = { .name = "CRTC_ID", },
> + [WDRM_PLANE_IN_FORMATS] = { .name = "IN_FORMATS" },
> };
>
> /**
> @@ -403,7 +419,11 @@ struct drm_plane {
>
> struct wl_list link;
>
> - uint32_t formats[];
> + struct {
> + uint32_t format;
> + uint32_t count_modifiers;
> + uint64_t *modifiers;
> + } formats[];
> };
>
> struct drm_output {
> @@ -2879,7 +2899,19 @@ drm_output_prepare_overlay_view(struct
> drm_output_state *output_state,
>
> /* Check whether the format is supported */
> for (i = 0; i < p->count_formats; i++) {
> - if (p->formats[i] == fb->format->format)
> + unsigned int j;
> +
> + if (p->formats[i].format != fb->format->format)
> + continue;
> +
> + if (fb->modifier == DRM_FORMAT_MOD_INVALID)
> + break;
> +
> + for (j = 0; j < p->formats[i].count_modifiers; j++) {
> + if (p->formats[i].modifiers[j] == fb->modifier)
> + break;
> + }
> + if (j != p->formats[i].count_modifiers)
> break;
> }
> if (i == p->count_formats)
> @@ -3635,6 +3667,61 @@ init_pixman(struct drm_backend *b)
> return pixman_renderer_init(b->compositor);
> }
>
> +/**
> + * Populates the formats array, and the modifiers of each format for a
> drm_plane.
> + */
> +#ifdef HAVE_DRM_FORMATS_BLOB
> +static bool
> +populate_format_modifiers(struct drm_plane *plane, const drmModePlane
> *kplane,
> + uint32_t blob_id)
I think this should be:
static int
drm_plane_populate_format_modifiers(...
I believe we tend to use int
[PATCH v14 40/41] compositor-drm: Support plane IN_FORMATS
From: Sergi Granell
The per-plane IN_FORMATS property adds information about format
modifiers; we can use these to both feed GBM with the set of modifiers
we want to use for rendering, and also as an early-out test when we're
trying to see if a FB will go on a particular plane.
Signed-off-by: Sergi Granell
Reviewed-by: Daniel Stone
Signed-off-by: Daniel Stone
---
configure.ac | 3 ++
libweston/compositor-drm.c | 128 +
2 files changed, 122 insertions(+), 9 deletions(-)
diff --git a/configure.ac b/configure.ac
index 8d0d6582a..bc6fefc1e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -212,6 +212,9 @@ if test x$enable_drm_compositor = xyes; then
PKG_CHECK_MODULES(DRM_COMPOSITOR_ATOMIC, [libdrm >= 2.4.78],
[AC_DEFINE([HAVE_DRM_ATOMIC], 1, [libdrm supports atomic
API])],
[AC_MSG_WARN([libdrm does not support atomic modesetting,
will omit that capability])])
+ 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, [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 cb1c23b03..ba2217fc0 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -83,6 +83,20 @@
#define GBM_BO_USE_CURSOR GBM_BO_USE_CURSOR_64X64
#endif
+#ifdef HAVE_DRM_FORMATS_BLOB
+static inline uint32_t *
+formats_ptr(struct drm_format_modifier_blob *blob)
+{
+ return (uint32_t *)(((char *)blob) + blob->formats_offset);
+}
+
+static inline struct drm_format_modifier *
+modifiers_ptr(struct drm_format_modifier_blob *blob)
+{
+ return (struct drm_format_modifier *)(((char *)blob) +
blob->modifiers_offset);
+}
+#endif
+
/**
* Represents the values of an enum-type KMS property
*/
@@ -127,6 +141,7 @@ enum wdrm_plane_property {
WDRM_PLANE_CRTC_H,
WDRM_PLANE_FB_ID,
WDRM_PLANE_CRTC_ID,
+ WDRM_PLANE_IN_FORMATS,
WDRM_PLANE__COUNT
};
@@ -168,6 +183,7 @@ static const struct drm_property_info plane_props[] = {
[WDRM_PLANE_CRTC_H] = { .name = "CRTC_H", },
[WDRM_PLANE_FB_ID] = { .name = "FB_ID", },
[WDRM_PLANE_CRTC_ID] = { .name = "CRTC_ID", },
+ [WDRM_PLANE_IN_FORMATS] = { .name = "IN_FORMATS" },
};
/**
@@ -403,7 +419,11 @@ struct drm_plane {
struct wl_list link;
- uint32_t formats[];
+ struct {
+ uint32_t format;
+ uint32_t count_modifiers;
+ uint64_t *modifiers;
+ } formats[];
};
struct drm_output {
@@ -2879,7 +2899,19 @@ drm_output_prepare_overlay_view(struct drm_output_state
*output_state,
/* Check whether the format is supported */
for (i = 0; i < p->count_formats; i++) {
- if (p->formats[i] == fb->format->format)
+ unsigned int j;
+
+ if (p->formats[i].format != fb->format->format)
+ continue;
+
+ if (fb->modifier == DRM_FORMAT_MOD_INVALID)
+ break;
+
+ for (j = 0; j < p->formats[i].count_modifiers; j++) {
+ if (p->formats[i].modifiers[j] == fb->modifier)
+ break;
+ }
+ if (j != p->formats[i].count_modifiers)
break;
}
if (i == p->count_formats)
@@ -3635,6 +3667,61 @@ init_pixman(struct drm_backend *b)
return pixman_renderer_init(b->compositor);
}
+/**
+ * Populates the formats array, and the modifiers of each format for a
drm_plane.
+ */
+#ifdef HAVE_DRM_FORMATS_BLOB
+static bool
+populate_format_modifiers(struct drm_plane *plane, const drmModePlane *kplane,
+ uint32_t blob_id)
+{
+ unsigned i, j;
+ drmModePropertyBlobRes *blob;
+ struct drm_format_modifier_blob *fmt_mod_blob;
+ uint32_t *blob_formats;
+ struct drm_format_modifier *blob_modifiers;
+
+ blob = drmModeGetPropertyBlob(plane->backend->drm.fd, blob_id);
+ if (!blob)
+ return false;
+
+ fmt_mod_blob = blob->data;
+ blob_formats = formats_ptr(fmt_mod_blob);
+ blob_modifiers = modifiers_ptr(fmt_mod_blob);
+
+ assert(plane->count_formats == fmt_mod_blob->count_formats);
+
+ for (i = 0; i < fmt_mod_blob->count_formats; i++) {
+ uint32_t count_modifiers = 0;
+ uint64_t *modifiers = NULL;
+
+ for (
