| Message ID | 20171019015346.10145-1-airlied@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > 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
> > 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
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
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); }