[PATCH v14 05/41] compositor-drm: Use drm_plane for cursor plane

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:22 +0000
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.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 463 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 358 insertions(+), 105 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 403438398..a600ef40b 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c

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

It seems output->gbm_format is not yet set at this time, so it cannot
be used here. However, this is fixed in the next patch, and before that
this code path is not reached, so technically it's ok.

> +			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_is_available(plane, output))
> +			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);
> +		return plane;
> +	}
> +
> +	return NULL;
>  }
>  


> @@ -3729,6 +3957,23 @@ drm_output_destroy(struct weston_output *base)
>  	if (output->base.enabled)
>  		drm_output_deinit(&output->base);
>  
> +	if (!b->universal_planes && !b->shutting_down) {
> +		/* With universal planes, the 'special' planes are allocated at
> +		 * startup, freed at shutdown, and live on the plane list in
> +		 * between. We want the planes to continue to exist and be freed
> +		 * up for other outputs.
> +		 *
> +		 * Without universal planes, our special planes are
> +		 * pseudo-planes allocated at output creation, freed at output
> +		 * destruction, and not usable by other outputs.
> +		 *
> +		 * On the other hand, if the compositor is already shutting down,
> +		 * the plane has already been destroyed.
> +		 */
> +		if (output->cursor_plane)
> +			drm_plane_destroy(output->cursor_plane);
> +	}
> +

Good catch.


All my comments from v12 seem to be addressed, so:

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


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/20180116/0097aab9/attachment-0001.sig>