On Thu, 5 Jul 2018 18:16:41 +0100 Daniel Stone <[email protected]> wrote:
> From: Sergi Granell <[email protected]> > > 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. Hi, I can see the early-out test, but where does this patch affect feeding GBM? > > Signed-off-by: Sergi Granell <[email protected]> > Reviewed-by: Daniel Stone <[email protected]> > Tested-by: Emre Ucan <[email protected]> > --- > configure.ac | 3 + > libweston/compositor-drm.c | 125 ++++++++++++++++++++++++++++++++++--- > 2 files changed, 119 insertions(+), 9 deletions(-) > > diff --git a/configure.ac b/configure.ac > index ef55ace36..c550198ae 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -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])]) > diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c > index 98c8ed584..3f8e77554 100644 > --- a/libweston/compositor-drm.c > +++ b/libweston/compositor-drm.c > @@ -131,6 +131,7 @@ enum wdrm_plane_property { > WDRM_PLANE_CRTC_H, > WDRM_PLANE_FB_ID, > WDRM_PLANE_CRTC_ID, > + WDRM_PLANE_IN_FORMATS, > WDRM_PLANE__COUNT > }; > > @@ -172,6 +173,7 @@ static const struct drm_property_info plane_props[] = { > [WDRM_PLANE_CRTC_H] = { .name = "CRTC_H", }, > [WDRM_PLANE_FB_ID] = { .name = "FB_ID", }, > [WDRM_PLANE_CRTC_ID] = { .name = "CRTC_ID", }, > + [WDRM_PLANE_IN_FORMATS] = { .name = "IN_FORMATS" }, > }; > > /** > @@ -406,7 +408,11 @@ struct drm_plane { > > struct wl_list link; > > - uint32_t formats[]; > + struct { > + uint32_t format; > + uint32_t count_modifiers; > + uint64_t *modifiers; > + } formats[]; > }; > > struct drm_head { > @@ -2908,7 +2914,19 @@ drm_output_prepare_overlay_view(struct > drm_output_state *output_state, > > /* Check whether the format is supported */ > for (i = 0; i < p->count_formats; i++) { > - if (p->formats[i] == fb->format->format) > + unsigned int j; > + > + if (p->formats[i].format != fb->format->format) > + continue; > + > + if (fb->modifier == DRM_FORMAT_MOD_INVALID) > + break; > + > + for (j = 0; j < p->formats[i].count_modifiers; j++) { > + if (p->formats[i].modifiers[j] == fb->modifier) > + break; > + } > + if (j != p->formats[i].count_modifiers) > break; > } > if (i == p->count_formats) > @@ -3505,6 +3523,91 @@ init_pixman(struct drm_backend *b) > return pixman_renderer_init(b->compositor); > } > > +#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); > +} > +#endif > + > +/** > + * Populates the plane's formats array, using either the IN_MODIFIERS blob > + * property (if available), or the plane's format list if not. > + */ > +static int > +drm_plane_populate_formats(struct drm_plane *plane, const drmModePlane > *kplane, > + const drmModeObjectProperties *props) > +{ > + unsigned i; > +#ifdef HAVE_DRM_FORMATS_BLOB > + drmModePropertyBlobRes *blob; > + struct drm_format_modifier_blob *fmt_mod_blob; > + struct drm_format_modifier *blob_modifiers; > + uint32_t *blob_formats; > + uint32_t blob_id; > + > + blob_id = drm_property_get_value(&plane->props[WDRM_PLANE_IN_FORMATS], > + props, > + 0); > + if (blob_id == 0) > + goto fallback; > + > + 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? > + > + for (i = 0; i < fmt_mod_blob->count_formats; i++) { > + uint32_t count_modifiers = 0; > + uint64_t *modifiers = NULL; > + unsigned j; > + > + 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])); > + assert(modifiers); > + modifiers[count_modifiers++] = mod->modifier; > + } > + > + plane->formats[i].format = blob_formats[i]; > + plane->formats[i].modifiers = modifiers; > + plane->formats[i].count_modifiers = count_modifiers; > + } > + > + drmModeFreePropertyBlob(blob); > + > + return 0; > + > +fallback: > +#endif This OTOH could use a simple assert since any failure would be due to our own code: assert(plane->count_formats == kplane->count_formats); > + /* 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. Thanks, pq > + > /** > * Create a drm_plane for a hardware plane > * > @@ -3534,25 +3637,23 @@ drm_plane_create(struct drm_backend *b, const > drmModePlane *kplane, > { > struct drm_plane *plane; > drmModeObjectProperties *props; > - int num_formats = (kplane) ? kplane->count_formats : 1; > + uint32_t num_formats = (kplane) ? kplane->count_formats : 1; > > plane = zalloc(sizeof(*plane) + > - (sizeof(uint32_t) * num_formats)); > + (sizeof(plane->formats[0]) * num_formats)); > if (!plane) { > weston_log("%s: out of memory\n", __func__); > return NULL; > } > > plane->backend = b; > + plane->count_formats = num_formats; > plane->state_cur = drm_plane_state_alloc(NULL, plane); > plane->state_cur->complete = true; > > if (kplane) { > plane->possible_crtcs = kplane->possible_crtcs; > plane->plane_id = kplane->plane_id; > - plane->count_formats = kplane->count_formats; > - memcpy(plane->formats, kplane->formats, > - kplane->count_formats * sizeof(kplane->formats[0])); > > props = drmModeObjectGetProperties(b->drm.fd, kplane->plane_id, > DRM_MODE_OBJECT_PLANE); > @@ -3566,13 +3667,19 @@ drm_plane_create(struct drm_backend *b, const > drmModePlane *kplane, > drm_property_get_value(&plane->props[WDRM_PLANE_TYPE], > props, > WDRM_PLANE_TYPE__COUNT); > + > + if (drm_plane_populate_formats(plane, kplane, props) < 0) { > + drmModeFreeObjectProperties(props); > + goto err; > + } > + > drmModeFreeObjectProperties(props); > } > else { > plane->possible_crtcs = (1 << output->pipe); > plane->plane_id = 0; > plane->count_formats = 1; > - plane->formats[0] = format; > + plane->formats[0].format = format; > plane->type = type; > } > > @@ -4820,7 +4927,7 @@ drm_output_set_gbm_format(struct weston_output *base, > * supported by the primary plane; we just hope that the GBM format > * works. */ > if (!b->universal_planes) > - output->scanout_plane->formats[0] = output->gbm_format; > + output->scanout_plane->formats[0].format = output->gbm_format; > } > > static void
pgpTXnr42aKVo.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
