[PATCH weston v11 09/13] compositor-drm: Track cursor_plane with a drm_plane

Pekka Paalanen ppaalanen at gmail.com
Fri Jul 21 11:56:53 UTC 2017


On Tue, 18 Jul 2017 14:14:31 +0100
Daniel Stone <daniels at 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 <daniels at 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
-------------- 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/20170721/2710861b/attachment.sig>


More information about the wayland-devel mailing list