[PATCH weston v11 10/13] compositor-drm: Use drm_plane for scanout plane
Pekka Paalanen
ppaalanen at gmail.com
Fri Jul 21 13:59:49 UTC 2017
On Tue, 18 Jul 2017 14:14:32 +0100
Daniel Stone <daniels at collabora.com> wrote:
> Use a real drm_plane to back the scanout plane, displacing
> output->fb_{last,cur,pending} to their plane-tracked equivalents.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
> libweston/compositor-drm.c | 157 ++++++++++++++++++++++++++-------------------
> 1 file changed, 91 insertions(+), 66 deletions(-)
Hi,
the biggest issue with this patch seems to be the missing drm_plane
teardown. There's also some wrong output/buffer sizes being used.
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 2ede76b6..bf3273ba 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -356,17 +356,8 @@ struct drm_output {
> struct gbm_surface *gbm_surface;
> uint32_t gbm_format;
>
> - /* Plane for a fullscreen direct scanout view */
> - struct weston_plane scanout_plane;
> -
> - /* The last framebuffer submitted to the kernel for this CRTC. */
> - struct drm_fb *fb_current;
> - /* The previously-submitted framebuffer, where the hardware has not
> - * yet acknowledged display of fb_current. */
> - struct drm_fb *fb_last;
> - /* Framebuffer we are going to submit to the kernel when the current
> - * repaint is flushed. */
> - struct drm_fb *fb_pending;
> + /* Plane being displayed directly on the CRTC */
> + struct drm_plane *scanout_plane;
>
> /* The last state submitted to the kernel for this CRTC. */
> struct drm_output_state *state_cur;
> @@ -1324,6 +1315,8 @@ drm_output_assign_state(struct drm_output_state *state,
>
> if (plane->type == WDRM_PLANE_TYPE_OVERLAY)
> output->vblank_pending++;
> + else if (plane->type == WDRM_PLANE_TYPE_PRIMARY)
> + output->page_flip_pending = 1;
> }
> }
>
> @@ -1371,6 +1364,8 @@ drm_output_prepare_scanout_view(struct drm_output_state *output_state,
> {
> struct drm_output *output = output_state->output;
> struct drm_backend *b = to_drm_backend(output->base.compositor);
> + struct drm_plane *scanout_plane = output->scanout_plane;
> + struct drm_plane_state *state;
> struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
> struct weston_buffer_viewport *viewport = &ev->surface->buffer_viewport;
> struct gbm_bo *bo;
> @@ -1424,15 +1419,29 @@ drm_output_prepare_scanout_view(struct drm_output_state *output_state,
> return NULL;
> }
>
> - output->fb_pending = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT);
> - if (!output->fb_pending) {
> + state = drm_output_state_get_plane(output_state, scanout_plane);
Is it not possible that state->fb is already non-NULL? I suppose not,
because drm_output_state_duplicate() is always called with
DRM_OUTPUT_STATE_CLEAR_PLANES in any path that may lead here.
Maybe an assert just in case?
> + state->fb = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT);
> + if (!state->fb) {
> + drm_plane_state_put_back(state);
> gbm_bo_destroy(bo);
> return NULL;
> }
>
> - drm_fb_set_buffer(output->fb_pending, buffer);
> + drm_fb_set_buffer(state->fb, buffer);
> +
> + state->output = output;
> +
> + state->src_x = 0;
> + state->src_y = 0;
> + state->src_w = ev->surface->width << 16;
> + state->src_h = ev->surface->height << 16;
You need to use buffer size here, not the surface size. We allow
non-identity scale and transform, as long as they are the same between
the buffer and the output. Surface size would be incorrect.
>
> - return &output->scanout_plane;
> + state->dest_x = 0;
> + state->dest_y = 0;
> + state->dest_w = output->base.width;
> + state->dest_h = output->base.height;
Destination size should probably be taken from current_mode, in case we
are temporarily in a non-native video mode. That would make the output
size different from the mode. And the output size is wrong here anyway,
if output transform and scale are not identity.
> +
> + return &scanout_plane->base;
> }
>
> static struct drm_fb *
> @@ -1497,12 +1506,15 @@ drm_output_render(struct drm_output_state *state, pixman_region32_t *damage)
> {
> struct drm_output *output = state->output;
> struct weston_compositor *c = output->base.compositor;
> + struct drm_plane_state *scanout_state;
> struct drm_backend *b = to_drm_backend(c);
> struct drm_fb *fb;
>
> /* If we already have a client buffer promoted to scanout, then we don't
> * want to render. */
> - if (output->fb_pending)
> + scanout_state = drm_output_state_get_plane(state,
> + output->scanout_plane);
> + if (scanout_state->fb)
> return;
>
> if (b->use_pixman)
> @@ -1510,9 +1522,24 @@ drm_output_render(struct drm_output_state *state, pixman_region32_t *damage)
> else
> fb = drm_output_render_gl(state, damage);
>
> - if (!fb)
> + if (!fb) {
> + drm_plane_state_put_back(scanout_state);
> return;
> - output->fb_pending = fb;
> + }
> +
> + scanout_state->fb = fb;
> + scanout_state->output = output;
> +
> + scanout_state->src_x = 0;
> + scanout_state->src_y = 0;
> + scanout_state->src_w = output->base.current_mode->width << 16;
> + scanout_state->src_h = output->base.current_mode->height << 16;
> +
> + scanout_state->dest_x = 0;
> + scanout_state->dest_y = 0;
> + scanout_state->dest_w = scanout_state->src_w >> 16;
> + scanout_state->dest_h = scanout_state->src_h >> 16;
> +
>
> pixman_region32_subtract(&c->primary_plane.damage,
> &c->primary_plane.damage, damage);
> @@ -1575,6 +1602,8 @@ drm_output_repaint(struct weston_output *output_base,
> struct drm_output *output = to_drm_output(output_base);
> struct drm_backend *backend =
> to_drm_backend(output->base.compositor);
> + struct drm_plane *scanout_plane = output->scanout_plane;
> + struct drm_plane_state *scanout_state;
> struct drm_plane_state *ps;
> struct drm_plane *p;
> struct drm_mode *mode;
> @@ -1594,7 +1623,6 @@ drm_output_repaint(struct weston_output *output_base,
> pending_state,
> DRM_OUTPUT_STATE_CLEAR_PLANES);
>
> - assert(!output->fb_last);
>
> /* If disable_planes is set then assign_planes() wasn't
> * called for this render, so we could still have a stale
> @@ -1607,14 +1635,29 @@ drm_output_repaint(struct weston_output *output_base,
> }
>
> drm_output_render(state, damage);
> - if (!output->fb_pending)
> + scanout_state = drm_output_state_get_plane(state, scanout_plane);
> + if (!scanout_state || !scanout_state->fb)
> goto err;
>
> + /* The legacy SetCrtc API doesn't allow us to do scaling, and the
> + * legacy PageFlip API doesn't allow us to do clipping either. */
> + assert(scanout_state->src_x == 0);
> + assert(scanout_state->src_y == 0);
> + assert(scanout_state->src_w ==
> + (unsigned) (output->base.current_mode->width << 16));
> + assert(scanout_state->src_h ==
> + (unsigned) (output->base.current_mode->height << 16));
> + assert(scanout_state->dest_x == 0);
> + assert(scanout_state->dest_y == 0);
> + assert(scanout_state->src_w == scanout_state->dest_w << 16);
> + assert(scanout_state->src_h == scanout_state->dest_h << 16);
The two asserts you said you already fixed. Technically I think the
code was correct, but definitely not easy to read right. :-D
> +
> mode = container_of(output->base.current_mode, struct drm_mode, base);
> - if (!output->fb_current ||
> - output->fb_current->stride != output->fb_pending->stride) {
> + if (!scanout_plane->state_cur->fb ||
> + scanout_plane->state_cur->fb->stride != scanout_state->fb->stride) {
> ret = drmModeSetCrtc(backend->drm.fd, output->crtc_id,
> - output->fb_pending->fb_id, 0, 0,
> + scanout_state->fb->fb_id,
> + 0, 0,
> &output->connector_id, 1,
> &mode->mode_info);
> if (ret) {
> @@ -1625,18 +1668,13 @@ drm_output_repaint(struct weston_output *output_base,
> }
>
> if (drmModePageFlip(backend->drm.fd, output->crtc_id,
> - output->fb_pending->fb_id,
> + scanout_state->fb->fb_id,
> DRM_MODE_PAGE_FLIP_EVENT, output) < 0) {
> weston_log("queueing pageflip failed: %m\n");
> goto err;
> }
>
> - output->fb_last = output->fb_current;
> - output->fb_current = output->fb_pending;
> - output->fb_pending = NULL;
> -
> assert(!output->page_flip_pending);
> - output->page_flip_pending = 1;
>
> if (output->pageflip_timer)
> wl_event_source_timer_update(output->pageflip_timer,
> @@ -1696,10 +1734,7 @@ drm_output_repaint(struct weston_output *output_base,
Heh, twice I have wondered why does drm_output_repaint() not call
drm_output_assign_state(), and then I remember it is done in
drm_repaint_flush(), and the reason for that is that with atomic
modesetting that really is where it will happen.
>
> err:
> output->cursor_view = NULL;
> - if (output->fb_pending) {
> - drm_fb_unref(output->fb_pending);
> - output->fb_pending = NULL;
> - }
> +
> drm_output_state_free(state);
>
> return -1;
> @@ -1712,6 +1747,7 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> struct drm_pending_state *pending_state;
> struct drm_output_state *state;
> struct drm_plane_state *plane_state;
> + struct drm_plane *scanout_plane = output->scanout_plane;
> struct drm_backend *backend =
> to_drm_backend(output_base->compositor);
> uint32_t fb_id;
> @@ -1728,11 +1764,13 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> if (output->disable_pending || output->destroy_pending)
> return;
>
> - if (!output->fb_current) {
> + if (!output->scanout_plane->state_cur->fb) {
> /* We can't page flip if there's no mode set */
> goto finish_frame;
> }
>
> + assert(scanout_plane->state_cur->output == output);
> +
> /* Try to get current msc and timestamp via instant query */
> vbl.request.type |= drm_waitvblank_pipe(output);
> ret = drmWaitVBlank(backend->drm.fd, &vbl);
> @@ -1762,10 +1800,9 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> /* Immediate query didn't provide valid timestamp.
> * Use pageflip fallback.
> */
> - fb_id = output->fb_current->fb_id;
> + fb_id = scanout_plane->state_cur->fb->fb_id;
>
> assert(!output->page_flip_pending);
> - assert(!output->fb_last);
> assert(!output->state_last);
>
> pending_state = drm_pending_state_alloc(backend);
> @@ -1783,9 +1820,6 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> wl_event_source_timer_update(output->pageflip_timer,
> backend->pageflip_timeout);
>
> - output->fb_last = drm_fb_ref(output->fb_current);
> - output->page_flip_pending = 1;
> -
> wl_list_for_each(plane_state, &state->plane_list, link) {
> if (plane_state->plane->type != WDRM_PLANE_TYPE_OVERLAY)
> continue;
> @@ -1858,9 +1892,6 @@ page_flip_handler(int fd, unsigned int frame,
> assert(output->page_flip_pending);
> output->page_flip_pending = 0;
>
> - drm_fb_unref(output->fb_last);
> - output->fb_last = NULL;
> -
> if (output->vblank_pending)
> return;
>
> @@ -2536,10 +2567,6 @@ drm_output_switch_mode(struct weston_output *output_base, struct weston_mode *mo
> * sledgehammer modeswitch first, and only later showing new
> * content.
> */
> - drm_fb_unref(output->fb_current);
> - assert(!output->fb_last);
> - assert(!output->fb_pending);
> - output->fb_last = output->fb_current = NULL;
I think the comment above might need updating, but I suppose it's safe
to say the switch_mode hook is just plain broken for now, to be fixed
in the future. Or is it broken?
We choose a new mode, we reinitialize the renderer... maybe it works
most of the time by luck? OTOH, not sure we even have a way to trigger
this to begin with.
>
> if (b->use_pixman) {
> drm_output_fini_pixman(output);
> @@ -2870,14 +2897,16 @@ drm_output_find_special_plane(struct drm_backend *b, struct drm_output *output,
> * same plane for two outputs. */
> wl_list_for_each(tmp, &b->compositor->pending_output_list,
> base.link) {
> - if (tmp->cursor_plane == plane) {
> + if (tmp->cursor_plane == plane ||
> + tmp->scanout_plane == plane) {
> found_elsewhere = true;
> break;
> }
> }
> wl_list_for_each(tmp, &b->compositor->output_list,
> base.link) {
> - if (tmp->cursor_plane == plane) {
> + if (tmp->cursor_plane == plane ||
> + tmp->scanout_plane == plane) {
Ah, here they are.
> found_elsewhere = true;
> break;
> }
> @@ -3755,6 +3784,15 @@ drm_output_enable(struct weston_output *base)
> if (b->pageflip_timeout)
> drm_output_pageflip_timer_create(output);
>
> + output->scanout_plane =
> + drm_output_find_special_plane(b, output,
> + WDRM_PLANE_TYPE_PRIMARY);
> + if (!output->scanout_plane) {
> + weston_log("Failed to find primary plane for output %s\n",
> + output->base.name);
> + goto err;
The error path (not this jump but the later ones) forgets to clean up
scanout_plane.
> + }
> +
> /* Failing to find a cursor plane is not fatal, as we'll fall back
> * to software cursor. */
> output->cursor_plane =
> @@ -3795,8 +3833,6 @@ drm_output_enable(struct weston_output *base)
> output->connector->connector_type == DRM_MODE_CONNECTOR_eDP)
> output->base.connection_internal = 1;
>
> - weston_plane_init(&output->scanout_plane, b->compositor, 0, 0);
> -
> if (output->cursor_plane)
> weston_compositor_stack_plane(b->compositor,
> &output->cursor_plane->base,
> @@ -3804,7 +3840,8 @@ drm_output_enable(struct weston_output *base)
> else
> b->cursors_are_broken = 1;
>
> - weston_compositor_stack_plane(b->compositor, &output->scanout_plane,
> + weston_compositor_stack_plane(b->compositor,
> + &output->scanout_plane->base,
> &b->compositor->primary_plane);
The scanout plane is a little strange. It is used for both fullscreen
direct scanout as well as the renderer output for the primary plane.
But, I guess that makes no harm. It's probably just another rabbit hole.
>
> weston_log("Output %s, (connector %d, crtc %d)\n",
> @@ -3831,20 +3868,11 @@ drm_output_deinit(struct weston_output *base)
> struct drm_output *output = to_drm_output(base);
> struct drm_backend *b = to_drm_backend(base->compositor);
>
> - /* output->fb_last and output->fb_pending must not be set here;
> - * destroy_pending/disable_pending exist to guarantee exactly this. */
> - assert(!output->fb_last);
> - assert(!output->fb_pending);
> - drm_fb_unref(output->fb_current);
> - output->fb_current = NULL;
> -
> if (b->use_pixman)
> drm_output_fini_pixman(output);
> else
> drm_output_fini_egl(output);
>
> - weston_plane_release(&output->scanout_plane);
I believe Philipp is right, the plane teardown is missing, just like
for the cursor plane.
> -
> if (output->cursor_plane) {
> /* Turn off hardware cursor */
> drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
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/20932360/attachment-0001.sig>
More information about the wayland-devel
mailing list