[Spice-devel] [PATCH] drm/qxl: workaround broken qxl hw primary setting.

[Frediano Ziglio]

> 
> From: Dave Airlie <airlied at redhat.com>
> 
> QXL hw can't change primary surfaces easily, the server sends a msg
> and the client flickers a lo when it does. The old pre-atomic
> page flip code endeavoured to workaround this with a copy operation.
> 
> This worked by another accident of how the qxl virtual gpu is designed,
> it does lazy operation evaluation on the host, so this operation
> wouldn't generally trash the memory, and result in correct display.
> 
> This tries to work out when the commit is likely just a pageflip
> and avoid touching the primary surface, this might go wrong at
> some point but I believe it's the same level as wrong as the old code
> base.
> 
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index afbf50d..3b702d1 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -502,6 +502,7 @@ static void qxl_primary_atomic_update(struct drm_plane
> *plane,
>  	struct qxl_framebuffer *qfb_old;
>  	struct qxl_bo *bo = gem_to_qxl_bo(qfb->obj);
>  	struct qxl_bo *bo_old;
> +	bool update_primary = true;
>  	struct drm_clip_rect norect = {
>  	    .x1 = 0,
>  	    .y1 = 0,
> @@ -519,15 +520,31 @@ static void qxl_primary_atomic_update(struct drm_plane
> *plane,
>  	if (bo == bo_old)
>  		return;
>  
> +	if (bo && bo_old &&
> +	    plane->state->crtc == old_state->crtc &&
> +	    plane->state->crtc_w == old_state->crtc_w &&
> +	    plane->state->crtc_h == old_state->crtc_h &&
> +	    plane->state->src_x == old_state->src_x &&
> +	    plane->state->src_y == old_state->src_y &&
> +	    plane->state->src_w == old_state->src_w &&
> +	    plane->state->src_h == old_state->src_h &&
> +	    plane->state->rotation == old_state->rotation &&
> +	    plane->state->zpos == old_state->zpos)
> +		/* this is likely a pageflip */
> +		update_primary = false;
> +
>  	if (bo_old && bo_old->is_primary) {
> -		qxl_io_destroy_primary(qdev);
> +		if (update_primary)
> +			qxl_io_destroy_primary(qdev);
>  		bo_old->is_primary = false;
>  	}
>  
>  	if (!bo->is_primary) {
> -		qxl_io_create_primary(qdev, 0, bo);
> +		if (update_primary)
> +			qxl_io_create_primary(qdev, 0, bo);
>  		bo->is_primary = true;

Here the primary is still the old object but you are setting the
new.
Looking around the old is unpinned and the new one pinned which
is now wrong as QXL device suppose the memory pointer by the
primary surface (after your patch bo_old object) remains
available.

>  	}
> +
>  	qxl_draw_dirty_fb(qdev, qfb, bo, 0, 0, &norect, 1, 1);

I had a look at this function. It send a DRAW_COPY operation
passing the bo object as a Drawable. However does nothing to
keep the object allocated. The Drawable and its buffer (so
bo object) should be available to QXL and not touched (changed)
till a release is received.

>  }
>  

If we are not able to avoid the copy and we need to keep the old
surface in place maybe instead of creating the new object as SURFACE
we could just use for source for the Drawable for the DRAW_COPY operation.
In this way when release is received the object can be unpinned being
free to be moved.

Frediano