[PATCH v14 40/41] compositor-drm: Support plane IN_FORMATS

[Daniel Stone]

Hi Pekka,

On Mon, 29 Jan 2018 at 15:01, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Wed, 20 Dec 2017 12:26:57 +0000 Daniel Stone <daniels at collabora.com> 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 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.

Done (just asserting).

> > @@ -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.

Yeah, this is what I did in the (merged) v17.

Cheers,
Daniel