Re: [PATCH v16 14/23] compositor-drm: Support plane IN_FORMATS
On Mon, 9 Jul 2018 13:42:45 +0100 Daniel Stone wrote: > Hi, > On Mon, 9 Jul 2018 at 11:08, Pekka Paalanen wrote: > > On Thu, 5 Jul 2018 18:16:41 +0100 Daniel Stone > > wrote: > > > 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. > > > > I can see the early-out test, but where does this patch affect feeding > > GBM? > > I've clarified the commit message. > > > > + blob = drmModeGetPropertyBlob(plane->backend->drm.fd, blob_id); > > > + if (!blob) > > > + goto fallback; > > > + > > > + 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); > > > > I don't think this should be an assert. We are comparing to something > > the kernel just told us, so if they don't match then it is either a > > wrong assumption or a kernel bug. Which one is it? > > Almost: we're comparing the number of formats inside the format-blob > container, to the number of formats from drmModeGetPlane. Any mismatch > between them is a kernel bug, as it's an invariant of the API that the > two must be equal. Nothing we can do about it if so, but given the > result would be overwriting random bits from the heap, and that quite > a few driver developers use Weston as a test, leaving the assert in > seemed wise. > > I'm not hugely attached to it though, so even though I've left it in > for v17, do feel free to remove it when pushing. I mean it should be turned into a weston_log() and maybe a abort(). Asserts can be disabled, this should not be. Thanks, pq > > > + /* No IN_MODIFIERS blob available, so just use the old. */ > > > + for (i = 0; i < kplane->count_formats; i++) > > > + plane->formats[i].format = kplane->formats[i]; > > > + > > > + return 0; > > > +} > > > > Otherwise the patch looks fine. > > I've fixed up the references to 'IN_MODIFIERS' (sic: IN_FORMAT) whilst here. > > Cheers, > Daniel pgp2TC6sFOJzY.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v16 14/23] compositor-drm: Support plane IN_FORMATS
Hi, On Mon, 9 Jul 2018 at 11:08, Pekka Paalanen wrote: > On Thu, 5 Jul 2018 18:16:41 +0100 Daniel Stone wrote: > > 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. > > I can see the early-out test, but where does this patch affect feeding > GBM? I've clarified the commit message. > > + blob = drmModeGetPropertyBlob(plane->backend->drm.fd, blob_id); > > + if (!blob) > > + goto fallback; > > + > > + 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); > > I don't think this should be an assert. We are comparing to something > the kernel just told us, so if they don't match then it is either a > wrong assumption or a kernel bug. Which one is it? Almost: we're comparing the number of formats inside the format-blob container, to the number of formats from drmModeGetPlane. Any mismatch between them is a kernel bug, as it's an invariant of the API that the two must be equal. Nothing we can do about it if so, but given the result would be overwriting random bits from the heap, and that quite a few driver developers use Weston as a test, leaving the assert in seemed wise. I'm not hugely attached to it though, so even though I've left it in for v17, do feel free to remove it when pushing. > > + /* No IN_MODIFIERS blob available, so just use the old. */ > > + for (i = 0; i < kplane->count_formats; i++) > > + plane->formats[i].format = kplane->formats[i]; > > + > > + return 0; > > +} > > Otherwise the patch looks fine. I've fixed up the references to 'IN_MODIFIERS' (sic: IN_FORMAT) whilst here. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v16 14/23] compositor-drm: Support plane IN_FORMATS
On Thu, 5 Jul 2018 18:16:41 +0100
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.
Hi,
I can see the early-out test, but where does this patch affect feeding
GBM?
>
> Signed-off-by: Sergi Granell
> Reviewed-by: Daniel Stone
> Tested-by: Emre Ucan
> ---
> configure.ac | 3 +
> libweston/compositor-drm.c | 125 ++---
> 2 files changed, 119 insertions(+), 9 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index ef55ace36..c550198ae 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 98c8ed584..3f8e77554 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -131,6 +131,7 @@ enum wdrm_plane_property {
> WDRM_PLANE_CRTC_H,
> WDRM_PLANE_FB_ID,
> WDRM_PLANE_CRTC_ID,
> + WDRM_PLANE_IN_FORMATS,
> WDRM_PLANE__COUNT
> };
>
> @@ -172,6 +173,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" },
> };
>
> /**
> @@ -406,7 +408,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_head {
> @@ -2908,7 +2914,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)
> @@ -3505,6 +3523,91 @@ init_pixman(struct drm_backend *b)
> return pixman_renderer_init(b->compositor);
> }
>
> +#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
> +
> +/**
> + * Populates the plane's formats array, using either the IN_MODIFIERS blob
> + * property (if available), or the plane's format list if not.
> + */
> +static int
> +drm_plane_populate_formats(struct drm_plane *plane, const drmModePlane
> *kplane,
> +const drmModeObjectProperties *props)
> +{
> + unsigned i;
> +#ifdef HAVE_DRM_FORMATS_BLOB
> + drmModePropertyBlobRes *blob;
> + struct drm_format_modifier_blob *fmt_mod_blob;
> + struct drm_format_modifier *blob_modifiers;
> + uint32_t *blob_formats;
> + uint32_t blob_id;
> +
> + blob_id = drm_property_get_value(&plane->props[WDRM_PLANE_IN_FORMATS],
> + props,
> + 0);
> + if (blob_id == 0)
> + goto fallback;
> +
> + blob = drmModeGetPr
[PATCH v16 14/23] 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
Tested-by: Emre Ucan
---
configure.ac | 3 +
libweston/compositor-drm.c | 125 ++---
2 files changed, 119 insertions(+), 9 deletions(-)
diff --git a/configure.ac b/configure.ac
index ef55ace36..c550198ae 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 98c8ed584..3f8e77554 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -131,6 +131,7 @@ enum wdrm_plane_property {
WDRM_PLANE_CRTC_H,
WDRM_PLANE_FB_ID,
WDRM_PLANE_CRTC_ID,
+ WDRM_PLANE_IN_FORMATS,
WDRM_PLANE__COUNT
};
@@ -172,6 +173,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" },
};
/**
@@ -406,7 +408,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_head {
@@ -2908,7 +2914,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)
@@ -3505,6 +3523,91 @@ init_pixman(struct drm_backend *b)
return pixman_renderer_init(b->compositor);
}
+#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
+
+/**
+ * Populates the plane's formats array, using either the IN_MODIFIERS blob
+ * property (if available), or the plane's format list if not.
+ */
+static int
+drm_plane_populate_formats(struct drm_plane *plane, const drmModePlane *kplane,
+ const drmModeObjectProperties *props)
+{
+ unsigned i;
+#ifdef HAVE_DRM_FORMATS_BLOB
+ drmModePropertyBlobRes *blob;
+ struct drm_format_modifier_blob *fmt_mod_blob;
+ struct drm_format_modifier *blob_modifiers;
+ uint32_t *blob_formats;
+ uint32_t blob_id;
+
+ blob_id = drm_property_get_value(&plane->props[WDRM_PLANE_IN_FORMATS],
+props,
+0);
+ if (blob_id == 0)
+ goto fallback;
+
+ blob = drmModeGetPropertyBlob(plane->backend->drm.fd, blob_id);
+ if (!blob)
+ goto fallback;
+
+ 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_
