[PATCH weston v12 04/40] compositor-drm: Use drm_plane for cursor plane
Pekka Paalanen
ppaalanen at gmail.com
Fri Sep 29 13:46:39 UTC 2017
On Tue, 26 Sep 2017 18:15:37 +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.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
> libweston/compositor-drm.c | 423 ++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 322 insertions(+), 101 deletions(-)
Hi Daniel,
this patch looks mostly good, but there are lifetime issues.
> static struct weston_plane *
> drm_output_prepare_cursor_view(struct drm_output_state *output_state,
> struct weston_view *ev)
> {
> struct drm_output *output = output_state->output;
> struct drm_backend *b = to_drm_backend(output->base.compositor);
> + struct drm_plane *plane = output->cursor_plane;
> + struct drm_plane_state *plane_state;
> struct weston_buffer_viewport *viewport = &ev->surface->buffer_viewport;
> struct wl_shm_buffer *shmbuf;
> + bool needs_update = false;
> float x, y;
>
> + if (!plane)
> + return NULL;
> +
> if (b->cursors_are_broken)
> return NULL;
>
> - if (output->cursor_view)
> + if (!plane->state_cur->complete)
> + return NULL;
> +
> + if (plane->state_cur->output && plane->state_cur->output != output)
> return NULL;
Just a thought: would it make sense to wrap the two above to something
like
bool
drm_plane_is_available(struct drm_plane *plane, struct drm_output *output)
{
if (!plane->state_cur->complete)
return false;
if (plane->state_cur->output && plane->state_cur->output != output)
return false;
return drm_plane_crtc_supported(output, plane);
}
That would make a nice place to explain what the conditions mean:
!plane->state_cur->complete
The plane is needed for something else already programmed and
not yet hit the screen.
state_cur->output && state_cur->output != output
Plane is in use through a different CRTC, so cannot take it
into use here and now, would need a disabled cycle first.
Also prevents plane stealing that might otherwise happen
between outputs updating at different times.
drm_plane_crtc_supported()
The CRTC and the plane can actually work together
hardware-wise, at all.
Is that correct?
The cursor path doesn't really need the drm_plane_crtc_supported()
check because that is implied by the drm_output::cursor_plane
assignment, but it doesn't hurt. The overlay path needs it.
>
> /* Don't import buffers which span multiple outputs. */
> @@ -2215,89 +2277,101 @@ drm_output_prepare_cursor_view(struct drm_output_state *output_state,
> ev->surface->height > b->cursor_height)
> return NULL;
>
> - output->cursor_view = ev;
> - weston_view_to_global_float(ev, 0, 0, &x, &y);
> - output->cursor_plane.x = x;
> - output->cursor_plane.y = y;
> + plane_state =
> + drm_output_state_get_plane(output_state, output->cursor_plane);
>
> - return &output->cursor_plane;
> -}
> -
> -/**
> - * Update the image for the current cursor surface
> - *
> - * @param b DRM backend structure
> - * @param bo GBM buffer object to write into
> - * @param ev View to use for cursor image
> - */
> -static void
> -cursor_bo_update(struct drm_backend *b, struct gbm_bo *bo,
> - struct weston_view *ev)
> -{
> - struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
> - uint32_t buf[b->cursor_width * b->cursor_height];
> - int32_t stride;
> - uint8_t *s;
> - int i;
> -
> - assert(buffer && buffer->shm_buffer);
> - assert(buffer->shm_buffer == wl_shm_buffer_get(buffer->resource));
> - assert(ev->surface->width <= b->cursor_width);
> - assert(ev->surface->height <= b->cursor_height);
> -
> - memset(buf, 0, sizeof buf);
> - stride = wl_shm_buffer_get_stride(buffer->shm_buffer);
> - s = wl_shm_buffer_get_data(buffer->shm_buffer);
> + if (plane_state && plane_state->fb)
> + return NULL;
I think plane_state cannot be NULL, so it should be not checked here.
It's not checked in the below code either.
plane_state->fb check prevents the same cursor plane for being used
multiple times on this repaint cycle, correct?
Checks on plane->state_cur are checking what has been submitted to the
kernel which is a different thing.
>
> - wl_shm_buffer_begin_access(buffer->shm_buffer);
> - for (i = 0; i < ev->surface->height; i++)
> - memcpy(buf + i * b->cursor_width,
> - s + i * stride,
> - ev->surface->width * 4);
> - wl_shm_buffer_end_access(buffer->shm_buffer);
> + /* Since we're setting plane state up front, we need to work out
> + * whether or not we need to upload a new cursor. We can't use the
> + * plane damage, since the planes haven't actually been calculated
> + * yet: instead try to figure it out directly. KMS cursor planes are
> + * pretty unique here, in that they lie partway between a Weston plane
> + * (direct scanout) and a renderer. */
> + if (ev != output->cursor_view ||
> + pixman_region32_not_empty(&ev->surface->damage)) {
> + output->current_cursor++;
> + output->current_cursor =
> + output->current_cursor %
> + ARRAY_LENGTH(output->gbm_cursor_fb);
> + needs_update = true;
> + }
>
> - if (gbm_bo_write(bo, buf, sizeof buf) < 0)
> - weston_log("failed update cursor: %m\n");
> + output->cursor_view = ev;
> + weston_view_to_global_float(ev, 0, 0, &x, &y);
> + plane->base.x = x;
> + plane->base.y = y;
> +
> + plane_state->fb =
> + drm_fb_ref(output->gbm_cursor_fb[output->current_cursor]);
> + plane_state->output = output;
> + plane_state->src_x = 0;
> + plane_state->src_y = 0;
> + plane_state->src_w = b->cursor_width << 16;
> + plane_state->src_h = b->cursor_height << 16;
> + plane_state->dest_x = (x - output->base.x) * output->base.current_scale;
> + plane_state->dest_y = (y - output->base.y) * output->base.current_scale;
> + plane_state->dest_w = b->cursor_width;
> + plane_state->dest_h = b->cursor_height;
> +
> + if (needs_update)
> + cursor_bo_update(b, plane_state->fb->bo, ev);
> +
> + return &plane->base;
> }
> @@ -2659,19 +2744,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.
When doing this, should you not also modify drm_plane_destroy() to do
the right thing for the 'internal' planes?
No need to drm_property_info_free() and should not drmModeSetPlane().
> + *
> * 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] = {
> @@ -2692,36 +2788,61 @@ drm_plane_create(struct drm_backend *b, const drmModePlane *kplane)
> },
> };
>
> - plane = zalloc(sizeof(*plane) + ((sizeof(uint32_t)) *
> - kplane->count_formats));
> + 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]));
>
> - props = drmModeObjectGetProperties(b->drm.fd, kplane->plane_id,
> - DRM_MODE_OBJECT_PLANE);
> - if (!props) {
> - weston_log("couldn't get plane properties\n");
> + 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);
> + if (!props) {
> + weston_log("couldn't get plane properties\n");
> + free(plane);
> + return NULL;
> + }
> + drm_property_info_populate(b, plane_props, plane->props,
> + WDRM_PLANE__COUNT, props);
> + plane->type =
> + drm_property_get_value(&plane->props[WDRM_PLANE_TYPE],
> + props,
> + WDRM_PLANE_TYPE__COUNT);
> + drmModeFreeObjectProperties(props);
> + }
> + else {
> + plane->possible_crtcs = (1 << output->pipe);
> + plane->plane_id = 0;
> + plane->count_formats = 1;
> + plane->formats[0] = format;
> + plane->type = type;
> + }
> +
> + if (plane->type == WDRM_PLANE_TYPE__COUNT) {
This seems to leak plane->props and plane->state_cur.
> free(plane);
> return NULL;
> }
> - drm_property_info_populate(b, plane_props, plane->props,
> - WDRM_PLANE__COUNT, props);
> - plane->type =
> - drm_property_get_value(&plane->props[WDRM_PLANE_TYPE],
> - props,
> - WDRM_PLANE_TYPE_OVERLAY);
> - drmModeFreeObjectProperties(props);
> +
> + /* With universal planes, everything is a DRM plane; without
> + * universal planes, the only DRM planes are overlay planes. */
> + if (b->universal_planes) {
> + assert(kplane);
> + } else {
> + assert((kplane && plane->type == WDRM_PLANE_TYPE_OVERLAY) ||
> + (!kplane && plane->type != WDRM_PLANE_TYPE_OVERLAY &&
> + output));
> + }
>
> weston_plane_init(&plane->base, b->compositor, 0, 0);
> wl_list_insert(&b->plane_list, &plane->link);
> @@ -2729,6 +2850,88 @@ drm_plane_create(struct drm_backend *b, const drmModePlane *kplane)
> return plane;
> }
>
> +/**
> + * 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;
There were discussions in IRC that output->gbm_format is not set at
this time yet, so we can't use it here.
I would propose to just hardcode the list of formats that
parse_gbm_format() supports. I presume the hardcoding here is because
we don't get it from the kernel, so it's no better or worse than before.
> + 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);
> + return plane;
> + }
> +
> + return NULL;
> +}
> +
> /**
> * Destroy one DRM plane
> *
> @@ -2778,7 +2981,8 @@ create_sprites(struct drm_backend *b)
> if (!kplane)
> continue;
>
> - drm_plane = drm_plane_create(b, kplane);
> + drm_plane = drm_plane_create(b, kplane, NULL,
> + WDRM_PLANE_TYPE__COUNT, 0);
> drmModeFreePlane(kplane);
> if (!drm_plane)
> continue;
> @@ -3044,6 +3248,10 @@ drm_output_init_cursor_egl(struct drm_output *output, struct drm_backend *b)
> {
> unsigned int i;
>
> + /* No point creating cursors if we don't have a plane for them. */
> + if (!output->cursor_plane)
> + return 0;
> +
> for (i = 0; i < ARRAY_LENGTH(output->gbm_cursor_fb); i++) {
> struct gbm_bo *bo;
>
> @@ -3631,11 +3839,15 @@ drm_output_enable(struct weston_output *base)
> output->connector->connector_type == DRM_MODE_CONNECTOR_eDP)
> output->base.connection_internal = true;
>
> - weston_plane_init(&output->cursor_plane, b->compositor,
> - INT32_MIN, INT32_MIN);
> weston_plane_init(&output->scanout_plane, b->compositor, 0, 0);
>
> - weston_compositor_stack_plane(b->compositor, &output->cursor_plane, NULL);
> + if (output->cursor_plane)
> + weston_compositor_stack_plane(b->compositor,
> + &output->cursor_plane->base,
> + NULL);
> + else
> + b->cursors_are_broken = 1;
> +
> weston_compositor_stack_plane(b->compositor, &output->scanout_plane,
> &b->compositor->primary_plane);
>
> @@ -3678,10 +3890,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);
This weston_plane_release() call is removed here.
To remain symmetric with drm_output_enable(), I think you should add
wl_list_remove(&output->cursor_plane->base.link);
wl_list_init(&output->cursor_plane->base.link);
here. It is theoretically possible to disable and then enable the same
output.
This is necessary also because the drm_plane for the cursor is not
destroyed when the respective drm_output is destroyed. The drm_plane
remains in the drm_backend::plane_list. When the same connector becomes
connected again and a new drm_output is created,
drm_output_find_special_plane() will find it again, and
drm_output_enable() will call
weston_compositor_stack_plane(cursor_plane) again, leading to list
corruption.
There is a catch. On shutdown, destroy_sprites() will destroy the
drm_planes, leaving drm_output::cursor_plane dangling. Then we destroy
the outputs, and the list removal hits a use-after-free.
Should destroy_sprites() be called after weston_compositor_shutdown()?
>
> - /* 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);
> + }
> }
>
> static void
> @@ -3845,6 +4058,12 @@ create_output_for_connector(struct drm_backend *b,
> }
> }
>
> + /* Failing to find a cursor plane is not fatal, as we'll fall back
> + * to software cursor. */
> + output->cursor_plane =
> + drm_output_find_special_plane(b, output,
> + WDRM_PLANE_TYPE_CURSOR);
> +
> weston_compositor_add_pending_output(&output->base, b->compositor);
>
> return 0;
> @@ -4083,7 +4302,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/20170929/edb584cf/attachment-0001.sig>
More information about the wayland-devel
mailing list