On Mon, 9 Jul 2018 13:42:45 +0100 Daniel Stone <[email protected]> wrote:
> Hi, > On Mon, 9 Jul 2018 at 11:08, Pekka Paalanen <[email protected]> wrote: > > On Thu, 5 Jul 2018 18:16:41 +0100 Daniel Stone <[email protected]> > > 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
pgp2TC6sFOJzY.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
