On Wed, 20 Dec 2017 12:26:57 +0000 Daniel Stone <dani...@collabora.com> wrote:
> From: Sergi Granell <xerpi.g...@gmail.com>
>
> 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 <xerpi.g...@gmail.com>
> Reviewed-by: Daniel Stone <dani...@collabora.com>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
> 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 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().
> +{
> + 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?
> + 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.
> + if (!modifiers) {
Realloc is a bit inconvenient in that if it fails, the original
allocation stays. Here I think it's such an unlikely occurrence that I
don't mind the leak. Everything would be failing anyway.
Either handle the original allocation staying, or don't bother handling
failure at all.
> + drmModeFreePropertyBlob(blob);
> + return false;
> + }
> + modifiers[count_modifiers++] = mod->modifier;
> + }
> +
> + plane->formats[i].format = blob_formats[i];
> + plane->formats[i].modifiers = modifiers;
> + plane->formats[i].count_modifiers = count_modifiers;
> + }
> +
> + drmModeFreePropertyBlob(blob);
> +
> + return true;
> +}
> +#endif
> +
> /**
> * Create a drm_plane for a hardware plane
> *
> @@ -3664,25 +3751,27 @@ drm_plane_create(struct drm_backend *b, const
> drmModePlane *kplane,
> {
> struct drm_plane *plane;
> drmModeObjectProperties *props;
> - int num_formats = (kplane) ? kplane->count_formats : 1;
> + uint32_t num_formats = (kplane) ? kplane->count_formats : 1;
>
> plane = zalloc(sizeof(*plane) +
> - (sizeof(uint32_t) * num_formats));
> + (sizeof(plane->formats[0]) * num_formats));
> if (!plane) {
> weston_log("%s: out of memory\n", __func__);
> return NULL;
> }
>
> plane->backend = b;
> + plane->count_formats = num_formats;
> plane->state_cur = drm_plane_state_alloc(NULL, plane);
> plane->state_cur->complete = true;
>
> if (kplane) {
> +#ifdef HAVE_DRM_FORMATS_BLOB
> + uint32_t blob_id;
> +#endif
> +
> plane->possible_crtcs = kplane->possible_crtcs;
> plane->plane_id = kplane->plane_id;
> - plane->count_formats = kplane->count_formats;
> - memcpy(plane->formats, kplane->formats,
> - kplane->count_formats * sizeof(kplane->formats[0]));
>
> props = drmModeObjectGetProperties(b->drm.fd, kplane->plane_id,
> DRM_MODE_OBJECT_PLANE);
> @@ -3696,13 +3785,34 @@ drm_plane_create(struct drm_backend *b, const
> drmModePlane *kplane,
> drm_property_get_value(&plane->props[WDRM_PLANE_TYPE],
> props,
> WDRM_PLANE_TYPE__COUNT);
> +
> +#ifdef HAVE_DRM_FORMATS_BLOB
> + blob_id =
> drm_property_get_value(&plane->props[WDRM_PLANE_IN_FORMATS],
> + props,
> + 0);
> + if (blob_id) {
> + if (!populate_format_modifiers(plane, kplane, blob_id))
> {
> + weston_log("%s: out of memory\n", __func__);
> + drm_property_info_free(plane->props,
> WDRM_PLANE__COUNT);
> + drmModeFreeObjectProperties(props);
> + free(plane);
> + return NULL;
> + }
> + } else
> +#endif
> + {
> + uint32_t i;
> + for (i = 0; i < kplane->count_formats; i++)
> + plane->formats[i].format = kplane->formats[i];
> + }
> +
I wonder if it would be more readable to have the above hunk in a
function
static int
drm_plane_populate_formats(struct drm_plane *plane,
const drmModePlane *kplane,
const drmModeObjectProperties *props);
That way the #ifdefs would be inside a smaller function, there would be
fewer clean-up paths, and more room in line length.
> drmModeFreeObjectProperties(props);
> }
> else {
> plane->possible_crtcs = (1 << output->pipe);
> plane->plane_id = 0;
> plane->count_formats = 1;
> - plane->formats[0] = format;
> + plane->formats[0].format = format;
> plane->type = type;
> }
>
> @@ -4768,7 +4878,7 @@ drm_output_set_gbm_format(struct weston_output *base,
> * supported by the primary plane; we just hope that the GBM format
> * works. */
> if (!b->universal_planes)
> - output->scanout_plane->formats[0] = output->gbm_format;
> + output->scanout_plane->formats[0].format = output->gbm_format;
> }
>
> static void
The code seems correct to me, mostly style'ish issues.
Thanks,
pq
pgpbrD6Iw43Lh.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
