On Tue, 18 Jul 2017 14:14:31 +0100 Daniel Stone <dani...@collabora.com> wrote:
> Change the type of cursor_plane from a weston_plane (base tracking
> structure) to a drm_plane (wrapper containing additional DRM-specific
> details), and make it a dynamically-allocated pointer.
>
> Using the standard drm_plane allows us to reuse code which already deals
> with drm_planes, e.g. a common cleanup function.
>
> This patch introduces a 'special plane' helper, creating a drm_plane
> either from a real KMS plane when using universal planes, or a fake plane
> otherwise. Without universal planes, the cursor and primary planes are
> hidden from us; this helper allows us to pretend otherwise.
Hi,
I found a couple of holes in the pretending: NULL dereference I think
you're guaranteed to hit, and missing fake plane teardown. We are going
to need a testing switch to make Weston believe universal planes are
not available so we can test that path, or stop pretending. :-)
But the universal plane path looks ok.
>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
> libweston/compositor-drm.c | 395
> ++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 304 insertions(+), 91 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 6fdf31a7..2ede76b6 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -2363,6 +2435,19 @@ drm_assign_planes(struct weston_output *output_base,
> void *repaint_data)
> pixman_region32_fini(&surface_overlap);
> }
> pixman_region32_fini(&overlap);
> +
> + /* We rely on ev->cursor_view being both an accurate reflection of the
ev->cursor_view should be output->cursor_view.
> + * cursor plane's state, but also being maintained across repaints to
> + * avoid unnecessary damage uploads, per the comment in
> + * drm_output_prepare_cursor_view. In the event that we go from having
> + * a cursor view to not having a cursor view, we need to clear it. */
> + if (output->cursor_view) {
> + plane_state =
> + drm_output_state_get_existing_plane(state,
> +
> output->cursor_plane);
> + if (!plane_state || !plane_state->fb)
> + output->cursor_view = NULL;
> + }
> }
>
> /**
> @@ -2628,19 +2713,30 @@ init_pixman(struct drm_backend *b)
> * Creates one drm_plane structure for a hardware plane, and initialises its
> * properties and formats.
> *
> + * In the absence of universal plane support, where KMS does not explicitly
> + * expose the primary and cursor planes to userspace, this may also create
> + * an 'internal' plane for internal management.
> + *
> * This function does not add the plane to the list of usable planes in
> Weston
> * itself; the caller is responsible for this.
> *
> * Call drm_plane_destroy to clean up the plane.
> *
> + * @sa drm_output_find_special_plane
> * @param b DRM compositor backend
> - * @param kplane DRM plane to create
> + * @param kplane DRM plane to create, or NULL if creating internal plane
> + * @param output Output to create internal plane for, or NULL
> + * @param type Type to use when creating internal plane, or invalid
> + * @param format Format to use for internal planes, or 0
> */
> static struct drm_plane *
> -drm_plane_create(struct drm_backend *b, const drmModePlane *kplane)
> +drm_plane_create(struct drm_backend *b, const drmModePlane *kplane,
> + struct drm_output *output, enum wdrm_plane_type type,
> + uint32_t format)
> {
> struct drm_plane *plane;
> drmModeObjectProperties *props;
> + int num_formats = (kplane) ? kplane->count_formats : 1;
>
> static struct drm_property_enum_info plane_type_enums[] = {
> [WDRM_PLANE_TYPE_PRIMARY] = {
> @@ -2661,21 +2757,38 @@ drm_plane_create(struct drm_backend *b, const
> drmModePlane *kplane)
> },
> };
>
> - plane = zalloc(sizeof(*plane) + ((sizeof(uint32_t)) *
> - kplane->count_formats));
> + /* With universal planes, everything is a DRM plane; without
> + * universal planes, the only DRM planes are overlay planes. */
> + if (b->universal_planes)
> + assert(b->universal_planes && kplane);
> + else
> + assert(!b->universal_planes &&
> + (type == WDRM_PLANE_TYPE_OVERLAY || (output && format)));
Any reason for the redundancy of the 'if' condition in the asserts?
> +
> + plane = zalloc(sizeof(*plane) +
> + (sizeof(uint32_t) * num_formats));
> if (!plane) {
> weston_log("%s: out of memory\n", __func__);
> return NULL;
> }
>
> plane->backend = b;
> - plane->possible_crtcs = kplane->possible_crtcs;
> - plane->plane_id = kplane->plane_id;
> - plane->count_formats = kplane->count_formats;
> plane->state_cur = drm_plane_state_alloc(NULL, plane);
> plane->state_cur->complete = true;
> - memcpy(plane->formats, kplane->formats,
> - kplane->count_formats * sizeof(kplane->formats[0]));
> +
> + 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]));
> + }
Remove line break here.
> + else {
> + plane->possible_crtcs = (1 << output->pipe);
> + plane->plane_id = 0;
> + plane->count_formats = 1;
> + plane->formats[0] = format;
> + }
>
> props = drmModeObjectGetProperties(b->drm.fd, kplane->plane_id,
> DRM_MODE_OBJECT_PLANE);
Does drmModeObjectGetProperties() actually work without universal
planes for non-overlay planes?
Should we simply set plane->type = type; and skip all the property
stuff here?
Oh, but this is even a NULL-dereference when kplane is NULL. Medic! :-)
> @@ -2689,7 +2802,7 @@ drm_plane_create(struct drm_backend *b, const
> drmModePlane *kplane)
> plane->type =
> drm_property_get_value(&plane->props[WDRM_PLANE_TYPE],
> props,
> - WDRM_PLANE_TYPE_OVERLAY);
> + type);
> drmModeFreeObjectProperties(props);
>
> weston_plane_init(&plane->base, b->compositor, 0, 0);
> @@ -2699,6 +2812,88 @@ drm_plane_create(struct drm_backend *b, const
> drmModePlane *kplane)
> }
>
> /**
> + * Find, or create, a special-purpose plane
> + *
> + * Primary and cursor planes are a special case, in that before universal
> + * planes, they are driven by non-plane API calls. Without universal plane
> + * support, the only way to configure a primary plane is via drmModeSetCrtc,
> + * and the only way to configure a cursor plane is drmModeSetCursor2.
> + *
> + * Although they may actually be regular planes in the hardware, without
> + * universal plane support, these planes are not actually exposed to
> + * userspace in the regular plane list.
> + *
> + * However, for ease of internal tracking, we want to manage all planes
> + * through the same drm_plane structures. Therefore, when we are running
> + * without universal plane support, we create fake drm_plane structures
> + * to track these planes.
> + *
> + * @param b DRM backend
> + * @param output Output to use for plane
> + * @param type Type of plane
> + */
> +static struct drm_plane *
> +drm_output_find_special_plane(struct drm_backend *b, struct drm_output
> *output,
> + enum wdrm_plane_type type)
> +{
> + struct drm_plane *plane;
> +
> + if (!b->universal_planes) {
> + uint32_t format;
> +
> + switch (type) {
> + case WDRM_PLANE_TYPE_CURSOR:
> + format = GBM_FORMAT_ARGB8888;
> + break;
> + case WDRM_PLANE_TYPE_PRIMARY:
> + format = output->gbm_format;
> + break;
> + default:
> + assert(!"invalid type in
> drm_output_find_special_plane");
> + break;
> + }
> +
> + return drm_plane_create(b, NULL, output, type, format);
> + }
> +
> + wl_list_for_each(plane, &b->plane_list, link) {
> + struct drm_output *tmp;
> + bool found_elsewhere = false;
> +
> + if (plane->type != type)
> + continue;
> + if (!drm_plane_crtc_supported(output, plane))
> + continue;
> +
> + /* On some platforms, primary/cursor planes can roam
> + * between different CRTCs, so make sure we don't claim the
> + * same plane for two outputs. */
> + wl_list_for_each(tmp, &b->compositor->pending_output_list,
> + base.link) {
> + if (tmp->cursor_plane == plane) {
> + found_elsewhere = true;
> + break;
> + }
> + }
> + wl_list_for_each(tmp, &b->compositor->output_list,
> + base.link) {
> + if (tmp->cursor_plane == plane) {
> + found_elsewhere = true;
> + break;
> + }
> + }
> +
> + if (found_elsewhere)
> + continue;
> +
> + plane->possible_crtcs = (1 << output->pipe);
I'm a little confused here. You smash the possible_crtcs and you have
the search loops to see if the plane was already associated to some
output. Why both, isn't one enough?
> + return plane;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> * Destroy one DRM plane
> *
> * Destroy a DRM plane, removing it from screen and releasing its retained
> @@ -3634,10 +3844,11 @@ drm_output_deinit(struct weston_output *base)
> drm_output_fini_egl(output);
>
> weston_plane_release(&output->scanout_plane);
> - weston_plane_release(&output->cursor_plane);
>
> - /* Turn off hardware cursor */
> - drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
> + if (output->cursor_plane) {
> + /* Turn off hardware cursor */
> + drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
Are we not leaking the cursor plane if drm_output_find_special_plane()
created it in the no-universal-planes case?
Looks like drm_plane_destroy() would need a special path for the
special fake planes.
> + }
> }
>
> static void
> @@ -4040,7 +4251,9 @@ session_notify(struct wl_listener *listener, void *data)
>
> wl_list_for_each(output, &compositor->output_list, base.link) {
> output->base.repaint_needed = false;
> - drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
> + if (output->cursor_plane)
> + drmModeSetCursor(b->drm.fd, output->crtc_id,
> + 0, 0, 0);
> }
>
> output = container_of(compositor->output_list.next,
Thanks,
pq
pgpey30VnHV2h.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
