The cursor must be set again after creating the primary surface.
Also drop the error message.
Signed-off-by: Gerd Hoffmann <[email protected]>
---
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;
}
--
2.9.3
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 <[email protected]>
> ---
> 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 <[email protected]>
> > 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 <[email protected]>
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 <[email protected]>
>
> 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