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

[Pekka Paalanen]

On Tue, 10 Jul 2018 08:41:46 +0100
Daniel Stone <daniel at fooishbar.org> wrote:

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

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180710/8ea79517/attachment-0001.sig>