[PATCH v16 14/23] compositor-drm: Support plane IN_FORMATS

[Daniel Stone]

Hi,
On Mon, 9 Jul 2018 at 11:08, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Thu,  5 Jul 2018 18:16:41 +0100 Daniel Stone <daniels at collabora.com> 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