drm/qxl: workaround broken qxl hw primary setting.
Subsystems (3)
Dave Airlie
maintainerPATCH 1/1 drm/qxl: workaround broken qxl hw primary setting.
From: Dave Airlie <[email protected]>
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.
--- drivers/gpu/drm/qxl/qxl_display.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
drivers/gpu/drm/qxl/qxl_display.c +19 -2
@@ -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 ); }
Signature · 6 lines
2.9.5 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 5 lines
From: Dave Airlie <[email protected]>
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
Quoted 2 lines
page flip code endeavoured to workaround this with a copy operation.
not clear. Do you mean a memcpy like operation or a DRAW_COPY ?
Quoted 9 lines
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 :-)
Quoted 44 lines
Signed-off-by Dave Airlie <[email protected]>--- drivers/gpu/drm/qxl/qxl_display.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)drivers/gpu/drm/qxl/qxl_display.c +1
-0@@ -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 ,drivers/gpu/drm/qxl/qxl_display.c +15
-1.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; }
drivers/gpu/drm/qxl/qxl_display.c static void qxl_primary_atomic_update(struct drm_plane · line 550
Quoted 10 lines
drivers/gpu/drm/qxl/qxl_display.c +3
-1if (!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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Quoted 55 lines
From: Dave Airlie <[email protected]>
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 <[email protected]>--- drivers/gpu/drm/qxl/qxl_display.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)drivers/gpu/drm/qxl/qxl_display.c +1
-0@@ -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 ,drivers/gpu/drm/qxl/qxl_display.c +13
-1.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);
drivers/gpu/drm/qxl/qxl_display.c static void qxl_primary_atomic_update(struct drm_plane · line 546
Quoted 10 lines
drivers/gpu/drm/qxl/qxl_display.c +4
-1+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.
drivers/gpu/drm/qxl/qxl_display.c static void qxl_primary_atomic_update(struct drm_plane *plane, · line 548
Quoted 3 lines
drivers/gpu/drm/qxl/qxl_display.c +1
-0} +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.
drivers/gpu/drm/qxl/qxl_display.c static void qxl_primary_atomic_update(struct drm_plane · line 550
Quoted 2 lines
drivers/gpu/drm/qxl/qxl_display.c +0
-0}
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Gerd Hoffmann
maintainerQuoted 56 lines · Frediano Ziglio
Quoted 56 lines
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 <[email protected]>---
drivers/gpu/drm/qxl/qxl_display.c | 21 +++++++++++++++++++--1 file changed, 19 insertions(+), 2 deletions(-)drivers/gpu/drm/qxl/qxl_display.c +0
-0@@ -502,6 +502,7 @@ static void qxl_primary_atomic_update(struct
drm_plane
*plane,
structqxl_framebuffer*qfb_old; structqxl_bo*bo =gem_to_qxl_bo(qfb->obj); structqxl_bo*bo_old;drivers/gpu/drm/qxl/qxl_display.c +1
-0+bool update_primary = true;struct
drm_clip_rectnorect = {
.x1 = 0,
.y1 = 0,drivers/gpu/drm/qxl/qxl_display.c +0
-0@@ -519,15 +520,31 @@ static void qxl_primary_atomic_update(struct
drm_plane
*plane,
if (bo ==bo_old)
return;drivers/gpu/drm/qxl/qxl_display.c +13
-0+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) {drivers/gpu/drm/qxl/qxl_display.c +2
-1-qxl_io_destroy_primary (qdev); +if (update_primary) +qxl_io_destroy_primary (qdev);
bo_old->is_primary= false;
}
if (!bo->is_primary) {drivers/gpu/drm/qxl/qxl_display.c +2
-1-qxl_io_create_primary (qdev,0 , bo); +if (update_primary) +qxl_io_create_primary (qdev,0 , bo);
Quoted 8 lines
Quoted 1 line
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 patchbo_oldobject) remains available.
Yes.
So it is bug compatible with commit 058e9f5c82, as the commit message claims ;)
Quoted 6 lines
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_COPYoperation.
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel