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

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

[Dave Airlie]

From: Dave Airlie <airlied@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@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);
 }
 
-- 
2.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/qxl: workaround broken qxl hw primary setting.

[Frediano Ziglio]

> 
> From: Dave Airlie <airlied@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@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
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> From: Dave Airlie <airlied@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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Gerd Hoffmann]

On Thu, 2017-10-19 at 07:28 -0400, Frediano Ziglio wrote:

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

Yes.

So it is bug compatible with commit 058e9f5c82, as the commit message
claims ;)

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

https://lists.freedesktop.org/archives/dri-devel/2017-October/155541.ht
ml

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel