[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

typo: lo -> lot

> page flip code endeavoured to workaround this with a copy operation.
> 

not clear. Do you mean a memcpy like operation or a DRAW_COPY ?

> 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.
> 

This sentence contains lot of hopes :-)

> 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;
>  	}
> +
>  	qxl_draw_dirty_fb(qdev, qfb, bo, 0, 0, &norect, 1, 1);
>  }
>  

What is actually supposed to do this function?
It looks weird. Looks like it's mixing drawing and surfaces together.
I don't see the reason to issue a DRAW_COPY (qxl_draw_dirty_fb) if
you just created the primary surface with data.
Also there's a big difference with and without this patch.
You are not changing the primary surface so the frame buffer (exactly
where spice-server draws the commands in QXL memory and where a screen
capture is supposed to find updated pixel after an update command)
still refers to bo_old object. However I don't see a related change in
the life management of bo_old. So or is going to be broken or was broken
before.

Frediano