| Message ID | 20190118122020.27596-11-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/qxl: ttm fixes, cleanups, allocation tweaks, multihead, fbdev | expand |
Den 18.01.2019 13.20, skrev Gerd Hoffmann: > The cursor must be set again after creating the primary surface. > Also drop the error message. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > drivers/gpu/drm/qxl/qxl_display.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c > index 86bfc19bea..1b700ef503 100644 > --- a/drivers/gpu/drm/qxl/qxl_display.c > +++ b/drivers/gpu/drm/qxl/qxl_display.c > @@ -533,7 +533,6 @@ static void qxl_primary_atomic_update(struct drm_plane *plane, > .x2 = plane->state->fb->width, > .y2 = plane->state->fb->height > }; > - int ret; > bool same_shadow = false; > > if (old_state->fb) { > @@ -554,16 +553,13 @@ static void qxl_primary_atomic_update(struct drm_plane *plane, > if (!same_shadow) > qxl_io_destroy_primary(qdev); > bo_old->is_primary = false; > - > - ret = qxl_primary_apply_cursor(plane); > - if (ret) > - DRM_ERROR( > - "could not set cursor after creating primary"); > } > > if (!bo->is_primary) { > - if (!same_shadow) > + if (!same_shadow) { > qxl_io_create_primary(qdev, 0, bo); > + qxl_primary_apply_cursor(plane); > + } > bo->is_primary = true; > } > > I don't see how the commit message matches what you're doing. It gives the impression that it must be applied under yet another condition, but the condition for applying the cursor is changed from bo_old->is_primary to !bo->is_primary. It probably makes sense to someone that knows the driver. Acked-by: Noralf Trønnes <noralf@tronnes.org>
> > The cursor must be set again after creating the primary surface. > > Also drop the error message. > > if (!bo->is_primary) { > > - if (!same_shadow) > > + if (!same_shadow) { > > qxl_io_create_primary(qdev, 0, bo); > > + qxl_primary_apply_cursor(plane); > > + } > > bo->is_primary = true; > > } > > > > > > I don't see how the commit message matches what you're doing. It gives > the impression that it must be applied under yet another condition, but > the condition for applying the cursor is changed from bo_old->is_primary > to !bo->is_primary. The qxl device ties the cursor to the primary surface. Therefore calling qxl_io_destroy_primary() and qxl_io_create_primary() to switch the framebuffer causes the cursor information being lost and the driver must re-apply it. The correct call order to do that is qxl_io_destroy_primary() + qxl_io_create_primary() + qxl_primary_apply_cursor(). The old code did qxl_io_destroy_primary() + qxl_primary_apply_cursor() + qxl_io_create_primary(). Due to qxl_primary_apply_cursor request being queued in a ringbuffer and qxl_io_create_primary() trapping to the hypervisor instantly there is a high chance that qxl_io_create_primary() is processed first even with the wrong call order. But it's racy and thus not reliable. > It probably makes sense to someone that knows the driver. If the above explains things better to you I should probably replace the commit message with that. > Acked-by: Noralf Trønnes <noralf@tronnes.org> thanks, Gerd
Den 28.01.2019 09.10, skrev Gerd Hoffmann: >>> The cursor must be set again after creating the primary surface. >>> Also drop the error message. > >>> if (!bo->is_primary) { >>> - if (!same_shadow) >>> + if (!same_shadow) { >>> qxl_io_create_primary(qdev, 0, bo); >>> + qxl_primary_apply_cursor(plane); >>> + } >>> bo->is_primary = true; >>> } >>> >>> >> >> I don't see how the commit message matches what you're doing. It gives >> the impression that it must be applied under yet another condition, but >> the condition for applying the cursor is changed from bo_old->is_primary >> to !bo->is_primary. > > The qxl device ties the cursor to the primary surface. Therefore > calling qxl_io_destroy_primary() and qxl_io_create_primary() to switch > the framebuffer causes the cursor information being lost and the driver > must re-apply it. > > The correct call order to do that is qxl_io_destroy_primary() + > qxl_io_create_primary() + qxl_primary_apply_cursor(). > > The old code did qxl_io_destroy_primary() + qxl_primary_apply_cursor() + > qxl_io_create_primary(). Due to qxl_primary_apply_cursor request being > queued in a ringbuffer and qxl_io_create_primary() trapping to the > hypervisor instantly there is a high chance that qxl_io_create_primary() > is processed first even with the wrong call order. But it's racy and > thus not reliable. > >> It probably makes sense to someone that knows the driver. > > If the above explains things better to you I should probably replace the > commit message with that. > This is actually my first review of a driver that I'm not familiar with. I'm not quite sure how much in depth understanding that is required to put my ack on it. Going further into the patchset I realised that there's no way that I can verify the logic without being intimate with the driver. So I have tried to verify things from a kms point of view. I liked your expanded explanation better. Noralf. >> Acked-by: Noralf Trønnes <noralf@tronnes.org> > > thanks, > Gerd >
Hi, > > If the above explains things better to you I should probably replace the > > commit message with that. > > This is actually my first review of a driver that I'm not familiar with. > I'm not quite sure how much in depth understanding that is required to > put my ack on it. Usually I try to show that by picking either Reviewed-by (I'm confident the changes are fine) or Acked-by (Changes look sane, but I don't know the code base and/or hardware good enough to be sure). > Going further into the patchset I realised that > there's no way that I can verify the logic without being intimate with > the driver. Yep. Same for me when looking at patches for other drivers. I think this is one reason why it isn't that easy to get patches for drivers reviewed where effectively only a single maintainer/developer is working on. > I liked your expanded explanation better. Updated the commit message. cheers, Gerd
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 86bfc19bea..1b700ef503 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -533,7 +533,6 @@ static void qxl_primary_atomic_update(struct drm_plane *plane, .x2 = plane->state->fb->width, .y2 = plane->state->fb->height }; - int ret; bool same_shadow = false; if (old_state->fb) { @@ -554,16 +553,13 @@ static void qxl_primary_atomic_update(struct drm_plane *plane, if (!same_shadow) qxl_io_destroy_primary(qdev); bo_old->is_primary = false; - - ret = qxl_primary_apply_cursor(plane); - if (ret) - DRM_ERROR( - "could not set cursor after creating primary"); } if (!bo->is_primary) { - if (!same_shadow) + if (!same_shadow) { qxl_io_create_primary(qdev, 0, bo); + qxl_primary_apply_cursor(plane); + } bo->is_primary = true; }
The cursor must be set again after creating the primary surface. Also drop the error message. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- drivers/gpu/drm/qxl/qxl_display.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)