On Thu, 21 May 2015 08:29:09 +0100
Daniel Stone <daniels at collabora.com> wrote:
> We already keep a GBM BO for the cursor plane, but also keep a DRM FB,
> so we can reuse it for atomic modesetting.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
> src/compositor-drm.c | 28 ++++++++++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index d25100d..7d80af6 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -252,6 +252,7 @@ struct drm_output {
>
> struct gbm_surface *surface;
> struct gbm_bo *cursor_bo[2];
> + struct drm_fb *cursor_fb[2];
> struct drm_plane *cursor_plane;
> struct weston_plane fb_plane;
> struct weston_view *cursor_view;
> @@ -789,11 +790,15 @@ drm_output_release_fb(struct drm_output *output, struct drm_fb *fb)
> if (!fb)
> return;
>
> + /* XXX: These static checks to output->dumb and output->cursor_fb
> + * are really unpleasant; we should revisit this with an explicit
> + * type attribute. */
> if (fb->map &&
> (fb != output->dumb[0] && fb != output->dumb[1])) {
> drm_fb_destroy_dumb(fb);
> } else if (fb->bo) {
> - if (fb->is_client_buffer)
> + if (fb->is_client_buffer ||
> + (fb == output->cursor_fb[0] || fb == output->cursor_fb[1]))
> gbm_bo_destroy(fb->bo);
> else
> gbm_surface_release_buffer(output->surface,
> @@ -1447,8 +1452,14 @@ drm_output_set_cursor(struct drm_output *output)
> pixman_region32_not_empty(&output->cursor_plane->base.damage)) {
> pixman_region32_fini(&output->cursor_plane->base.damage);
> pixman_region32_init(&output->cursor_plane->base.damage);
> + assert(output->cursor_plane->current ==
> + output->cursor_fb[output->current_cursor]);
> + output->cursor_plane->next =
> + output->cursor_fb[output->current_cursor];
> output->current_cursor ^= 1;
> bo = output->cursor_bo[output->current_cursor];
> + output->cursor_plane->current =
> + output->cursor_fb[output->current_cursor];
This logic looks funny to me. I think setting cursor_plane->next should
be in drm_output_prepare_cursor_view(), like it is for both
scanout_view (fullscreen) and overlay_view.
So, if prepare_cursor_view() set
output->cursor_plane->next =
output->cursor_fb[output->current_cursor];
then I think we see an issue here: it should use the other drm_fb.
Then, drm_output_set_cursor() woud take ->next and make it ->current.
Which means moving the current_cursor logic into prepare_cursor_view().
But.
The code written as is actually works. It writes to the bo that is not
currently on screen, just like it should. Just the assignment to
cursor_plane->next totally confuses me.
>
> cursor_bo_update(bo, ev);
> handle = gbm_bo_get_handle(bo).s32;
> @@ -2082,6 +2093,10 @@ drm_output_init_egl(struct drm_output *output, struct drm_compositor *ec)
> return -1;
> }
>
> + /* No point creating cursors if we don't have a plane for them. */
> + if (!output->cursor_plane)
> + return 0;
> +
> flags = GBM_BO_USE_CURSOR | GBM_BO_USE_WRITE;
>
> for (i = 0; i < 2; i++) {
> @@ -2091,9 +2106,18 @@ drm_output_init_egl(struct drm_output *output, struct drm_compositor *ec)
> output->cursor_bo[i] =
> gbm_bo_create(ec->gbm, ec->cursor_width, ec->cursor_height,
> GBM_FORMAT_ARGB8888, flags);
> + if (!output->cursor_bo[i])
> + break;
> +
> + output->cursor_fb[i] =
> + drm_fb_get_from_bo(output->cursor_bo[i], ec,
> + GBM_FORMAT_ARGB8888);
> + if (!output->cursor_fb[i])
> + break;
> }
>
> - if (output->cursor_bo[0] == NULL || output->cursor_bo[1] == NULL) {
> + if (output->cursor_bo[0] == NULL || output->cursor_bo[1] == NULL ||
> + output->cursor_fb[0] == NULL || output->cursor_fb[1] == NULL) {
> weston_log("cursor buffers unavailable, using gl cursors\n");
> ec->cursors_are_broken = 1;
> }
Hmm... nothing ever destroys cursor_fb? Or even cursor_bo to begin with?
Thanks,
pq