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

[Pekka Paalanen]

On Mon, 9 Jul 2018 13:42:45 +0100
Daniel Stone <daniel at fooishbar.org> wrote:

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

I mean it should be turned into a weston_log() and maybe a abort().

Asserts can be disabled, this should not be.


Thanks,
pq

> > > +     /* 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

-------------- 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/20180709/0818b9a3/attachment.sig>