Hi Pekka, On Mon, 29 Jan 2018 at 15:01, Pekka Paalanen <ppaala...@gmail.com> wrote: > On Wed, 20 Dec 2017 12:26:57 +0000 Daniel Stone <dani...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel