[PATCH] drm/qxl: Pin buffer objects for internal mappings

[PATCH] drm/qxl: Pin buffer objects for internal mappings

[Thomas Zimmermann]

Add qxl_bo_pin_and_vmap() that pins and vmaps a buffer object in one
step. Update callers of the regular qxl_bo_vmap(). Fixes a bug where
qxl accesses an unpinned buffer object while it is being moved; such
as with the monitor-description BO. An typical error is shown below.

[    4.303586] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0
[    4.586883] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0
[    4.904036] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65335296x16777216+0+0
[    5.374347] [drm:qxl_release_from_id_locked] *ERROR* failed to find id in release_idr

Commit b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap")
removed the implicit pin operation from qxl's vmap code. This is the
correct behavior for GEM and PRIME interfaces, but the pin is still
needed for qxl internal operation.

Also add a corresponding function qxl_bo_vunmap_and_unpin() and remove
the old qxl_bo_vmap() helpers.

Future directions: BOs should not be pinned or vmapped unnecessarily.
The pin-and-vmap operation should be removed from the driver and a
temporary mapping should be established with a vmap_local-like helper.
See the client helper drm_client_buffer_vmap_local() for semantics.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap")
Reported-by: David Kaplan <david.kaplan@amd.com>
Closes: https://lore.kernel.org/dri-devel/ab0fb17d-0f96-4ee6-8b21-65d02bb02655@suse.de/
Tested-by: David Kaplan <david.kaplan@amd.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Zack Rusin <zack.rusin@broadcom.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: virtualization@lists.linux.dev
Cc: spice-devel@lists.freedesktop.org
---
 drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++-------
 drivers/gpu/drm/qxl/qxl_object.c  | 11 +++++++++--
 drivers/gpu/drm/qxl/qxl_object.h  |  4 ++--
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 86a5dea710c0..bc24af08dfcd 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -584,11 +584,11 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
 	if (ret)
 		goto err;
 
-	ret = qxl_bo_vmap(cursor_bo, &cursor_map);
+	ret = qxl_bo_pin_and_vmap(cursor_bo, &cursor_map);
 	if (ret)
 		goto err_unref;
 
-	ret = qxl_bo_vmap(user_bo, &user_map);
+	ret = qxl_bo_pin_and_vmap(user_bo, &user_map);
 	if (ret)
 		goto err_unmap;
 
@@ -614,12 +614,12 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
 		       user_map.vaddr, size);
 	}
 
-	qxl_bo_vunmap(user_bo);
-	qxl_bo_vunmap(cursor_bo);
+	qxl_bo_vunmap_and_unpin(user_bo);
+	qxl_bo_vunmap_and_unpin(cursor_bo);
 	return cursor_bo;
 
 err_unmap:
-	qxl_bo_vunmap(cursor_bo);
+	qxl_bo_vunmap_and_unpin(cursor_bo);
 err_unref:
 	qxl_bo_unpin(cursor_bo);
 	qxl_bo_unref(&cursor_bo);
@@ -1205,7 +1205,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
 	}
 	qdev->monitors_config_bo = gem_to_qxl_bo(gobj);
 
-	ret = qxl_bo_vmap(qdev->monitors_config_bo, &map);
+	ret = qxl_bo_pin_and_vmap(qdev->monitors_config_bo, &map);
 	if (ret)
 		return ret;
 
@@ -1236,7 +1236,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
 	qdev->monitors_config = NULL;
 	qdev->ram_header->monitors_config = 0;
 
-	ret = qxl_bo_vunmap(qdev->monitors_config_bo);
+	ret = qxl_bo_vunmap_and_unpin(qdev->monitors_config_bo);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 5893e27a7ae5..cb1b7c2580ae 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -182,7 +182,7 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map)
 	return 0;
 }
 
-int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
+int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map)
 {
 	int r;
 
@@ -190,7 +190,13 @@ int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
 	if (r)
 		return r;
 
+	r = qxl_bo_pin_locked(bo);
+	if (r)
+		return r;
+
 	r = qxl_bo_vmap_locked(bo, map);
+	if (r)
+		qxl_bo_unpin_locked(bo);
 	qxl_bo_unreserve(bo);
 	return r;
 }
@@ -241,7 +247,7 @@ void qxl_bo_vunmap_locked(struct qxl_bo *bo)
 	ttm_bo_vunmap(&bo->tbo, &bo->map);
 }
 
-int qxl_bo_vunmap(struct qxl_bo *bo)
+int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo)
 {
 	int r;
 
@@ -250,6 +256,7 @@ int qxl_bo_vunmap(struct qxl_bo *bo)
 		return r;
 
 	qxl_bo_vunmap_locked(bo);
+	qxl_bo_unpin_locked(bo);
 	qxl_bo_unreserve(bo);
 	return 0;
 }
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index 1cf5bc759101..875f63221074 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -59,9 +59,9 @@ extern int qxl_bo_create(struct qxl_device *qdev,
 			 u32 priority,
 			 struct qxl_surface *surf,
 			 struct qxl_bo **bo_ptr);
-int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map);
+int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map);
 int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map);
-int qxl_bo_vunmap(struct qxl_bo *bo);
+int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo);
 void qxl_bo_vunmap_locked(struct qxl_bo *bo);
 void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset);
 void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map);
-- 
2.45.2

Re: [PATCH] drm/qxl: Pin buffer objects for internal mappings

[Thomas Zimmermann]

Hi,

ping for a review. This is a bugfix for a serious problem.

Best regards
Thomas

Am 02.07.24 um 16:20 schrieb Thomas Zimmermann:
> Add qxl_bo_pin_and_vmap() that pins and vmaps a buffer object in one
> step. Update callers of the regular qxl_bo_vmap(). Fixes a bug where
> qxl accesses an unpinned buffer object while it is being moved; such
> as with the monitor-description BO. An typical error is shown below.
>
> [    4.303586] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0
> [    4.586883] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0
> [    4.904036] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65335296x16777216+0+0
> [    5.374347] [drm:qxl_release_from_id_locked] *ERROR* failed to find id in release_idr
>
> Commit b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap")
> removed the implicit pin operation from qxl's vmap code. This is the
> correct behavior for GEM and PRIME interfaces, but the pin is still
> needed for qxl internal operation.
>
> Also add a corresponding function qxl_bo_vunmap_and_unpin() and remove
> the old qxl_bo_vmap() helpers.
>
> Future directions: BOs should not be pinned or vmapped unnecessarily.
> The pin-and-vmap operation should be removed from the driver and a
> temporary mapping should be established with a vmap_local-like helper.
> See the client helper drm_client_buffer_vmap_local() for semantics.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap")
> Reported-by: David Kaplan <david.kaplan@amd.com>
> Closes: https://lore.kernel.org/dri-devel/ab0fb17d-0f96-4ee6-8b21-65d02bb02655@suse.de/
> Tested-by: David Kaplan <david.kaplan@amd.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Zack Rusin <zack.rusin@broadcom.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: virtualization@lists.linux.dev
> Cc: spice-devel@lists.freedesktop.org
> ---
>   drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++-------
>   drivers/gpu/drm/qxl/qxl_object.c  | 11 +++++++++--
>   drivers/gpu/drm/qxl/qxl_object.h  |  4 ++--
>   3 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index 86a5dea710c0..bc24af08dfcd 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -584,11 +584,11 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
>   	if (ret)
>   		goto err;
>   
> -	ret = qxl_bo_vmap(cursor_bo, &cursor_map);
> +	ret = qxl_bo_pin_and_vmap(cursor_bo, &cursor_map);
>   	if (ret)
>   		goto err_unref;
>   
> -	ret = qxl_bo_vmap(user_bo, &user_map);
> +	ret = qxl_bo_pin_and_vmap(user_bo, &user_map);
>   	if (ret)
>   		goto err_unmap;
>   
> @@ -614,12 +614,12 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
>   		       user_map.vaddr, size);
>   	}
>   
> -	qxl_bo_vunmap(user_bo);
> -	qxl_bo_vunmap(cursor_bo);
> +	qxl_bo_vunmap_and_unpin(user_bo);
> +	qxl_bo_vunmap_and_unpin(cursor_bo);
>   	return cursor_bo;
>   
>   err_unmap:
> -	qxl_bo_vunmap(cursor_bo);
> +	qxl_bo_vunmap_and_unpin(cursor_bo);
>   err_unref:
>   	qxl_bo_unpin(cursor_bo);
>   	qxl_bo_unref(&cursor_bo);
> @@ -1205,7 +1205,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>   	}
>   	qdev->monitors_config_bo = gem_to_qxl_bo(gobj);
>   
> -	ret = qxl_bo_vmap(qdev->monitors_config_bo, &map);
> +	ret = qxl_bo_pin_and_vmap(qdev->monitors_config_bo, &map);
>   	if (ret)
>   		return ret;
>   
> @@ -1236,7 +1236,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
>   	qdev->monitors_config = NULL;
>   	qdev->ram_header->monitors_config = 0;
>   
> -	ret = qxl_bo_vunmap(qdev->monitors_config_bo);
> +	ret = qxl_bo_vunmap_and_unpin(qdev->monitors_config_bo);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
> index 5893e27a7ae5..cb1b7c2580ae 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -182,7 +182,7 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map)
>   	return 0;
>   }
>   
> -int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
> +int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map)
>   {
>   	int r;
>   
> @@ -190,7 +190,13 @@ int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
>   	if (r)
>   		return r;
>   
> +	r = qxl_bo_pin_locked(bo);
> +	if (r)
> +		return r;
> +
>   	r = qxl_bo_vmap_locked(bo, map);
> +	if (r)
> +		qxl_bo_unpin_locked(bo);
>   	qxl_bo_unreserve(bo);
>   	return r;
>   }
> @@ -241,7 +247,7 @@ void qxl_bo_vunmap_locked(struct qxl_bo *bo)
>   	ttm_bo_vunmap(&bo->tbo, &bo->map);
>   }
>   
> -int qxl_bo_vunmap(struct qxl_bo *bo)
> +int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo)
>   {
>   	int r;
>   
> @@ -250,6 +256,7 @@ int qxl_bo_vunmap(struct qxl_bo *bo)
>   		return r;
>   
>   	qxl_bo_vunmap_locked(bo);
> +	qxl_bo_unpin_locked(bo);
>   	qxl_bo_unreserve(bo);
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
> index 1cf5bc759101..875f63221074 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.h
> +++ b/drivers/gpu/drm/qxl/qxl_object.h
> @@ -59,9 +59,9 @@ extern int qxl_bo_create(struct qxl_device *qdev,
>   			 u32 priority,
>   			 struct qxl_surface *surf,
>   			 struct qxl_bo **bo_ptr);
> -int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map);
> +int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map);
>   int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map);
> -int qxl_bo_vunmap(struct qxl_bo *bo);
> +int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo);
>   void qxl_bo_vunmap_locked(struct qxl_bo *bo);
>   void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset);
>   void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH] drm/qxl: Pin buffer objects for internal mappings

[Daniel Vetter]

On Mon, Jul 08, 2024 at 10:09:44AM +0200, Thomas Zimmermann wrote:
> Hi,
> 
> ping for a review. This is a bugfix for a serious problem.

I tried to look around whether there's any place where we could WARN_ON if
we create a vmap but it's not pinned. But there's lots of places where we
want the vmap only for the duration of the dma_resv locked section, so
really can't do that. And your patch removes the unlocked vmap
implementation, which would be the only place really.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> Best regards
> Thomas
> 
> Am 02.07.24 um 16:20 schrieb Thomas Zimmermann:
> > Add qxl_bo_pin_and_vmap() that pins and vmaps a buffer object in one
> > step. Update callers of the regular qxl_bo_vmap(). Fixes a bug where
> > qxl accesses an unpinned buffer object while it is being moved; such
> > as with the monitor-description BO. An typical error is shown below.
> > 
> > [    4.303586] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0
> > [    4.586883] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0
> > [    4.904036] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65335296x16777216+0+0
> > [    5.374347] [drm:qxl_release_from_id_locked] *ERROR* failed to find id in release_idr
> > 
> > Commit b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap")
> > removed the implicit pin operation from qxl's vmap code. This is the
> > correct behavior for GEM and PRIME interfaces, but the pin is still
> > needed for qxl internal operation.
> > 
> > Also add a corresponding function qxl_bo_vunmap_and_unpin() and remove
> > the old qxl_bo_vmap() helpers.
> > 
> > Future directions: BOs should not be pinned or vmapped unnecessarily.
> > The pin-and-vmap operation should be removed from the driver and a
> > temporary mapping should be established with a vmap_local-like helper.
> > See the client helper drm_client_buffer_vmap_local() for semantics.
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Fixes: b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap")
> > Reported-by: David Kaplan <david.kaplan@amd.com>
> > Closes: https://lore.kernel.org/dri-devel/ab0fb17d-0f96-4ee6-8b21-65d02bb02655@suse.de/
> > Tested-by: David Kaplan <david.kaplan@amd.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Zack Rusin <zack.rusin@broadcom.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: virtualization@lists.linux.dev
> > Cc: spice-devel@lists.freedesktop.org
> > ---
> >   drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++-------
> >   drivers/gpu/drm/qxl/qxl_object.c  | 11 +++++++++--
> >   drivers/gpu/drm/qxl/qxl_object.h  |  4 ++--
> >   3 files changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> > index 86a5dea710c0..bc24af08dfcd 100644
> > --- a/drivers/gpu/drm/qxl/qxl_display.c
> > +++ b/drivers/gpu/drm/qxl/qxl_display.c
> > @@ -584,11 +584,11 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
> >   	if (ret)
> >   		goto err;
> > -	ret = qxl_bo_vmap(cursor_bo, &cursor_map);
> > +	ret = qxl_bo_pin_and_vmap(cursor_bo, &cursor_map);
> >   	if (ret)
> >   		goto err_unref;
> > -	ret = qxl_bo_vmap(user_bo, &user_map);
> > +	ret = qxl_bo_pin_and_vmap(user_bo, &user_map);
> >   	if (ret)
> >   		goto err_unmap;
> > @@ -614,12 +614,12 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
> >   		       user_map.vaddr, size);
> >   	}
> > -	qxl_bo_vunmap(user_bo);
> > -	qxl_bo_vunmap(cursor_bo);
> > +	qxl_bo_vunmap_and_unpin(user_bo);
> > +	qxl_bo_vunmap_and_unpin(cursor_bo);
> >   	return cursor_bo;
> >   err_unmap:
> > -	qxl_bo_vunmap(cursor_bo);
> > +	qxl_bo_vunmap_and_unpin(cursor_bo);
> >   err_unref:
> >   	qxl_bo_unpin(cursor_bo);
> >   	qxl_bo_unref(&cursor_bo);
> > @@ -1205,7 +1205,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
> >   	}
> >   	qdev->monitors_config_bo = gem_to_qxl_bo(gobj);
> > -	ret = qxl_bo_vmap(qdev->monitors_config_bo, &map);
> > +	ret = qxl_bo_pin_and_vmap(qdev->monitors_config_bo, &map);
> >   	if (ret)
> >   		return ret;
> > @@ -1236,7 +1236,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
> >   	qdev->monitors_config = NULL;
> >   	qdev->ram_header->monitors_config = 0;
> > -	ret = qxl_bo_vunmap(qdev->monitors_config_bo);
> > +	ret = qxl_bo_vunmap_and_unpin(qdev->monitors_config_bo);
> >   	if (ret)
> >   		return ret;
> > diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
> > index 5893e27a7ae5..cb1b7c2580ae 100644
> > --- a/drivers/gpu/drm/qxl/qxl_object.c
> > +++ b/drivers/gpu/drm/qxl/qxl_object.c
> > @@ -182,7 +182,7 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map)
> >   	return 0;
> >   }
> > -int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
> > +int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map)
> >   {
> >   	int r;
> > @@ -190,7 +190,13 @@ int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
> >   	if (r)
> >   		return r;
> > +	r = qxl_bo_pin_locked(bo);
> > +	if (r)
> > +		return r;
> > +
> >   	r = qxl_bo_vmap_locked(bo, map);
> > +	if (r)
> > +		qxl_bo_unpin_locked(bo);
> >   	qxl_bo_unreserve(bo);
> >   	return r;
> >   }
> > @@ -241,7 +247,7 @@ void qxl_bo_vunmap_locked(struct qxl_bo *bo)
> >   	ttm_bo_vunmap(&bo->tbo, &bo->map);
> >   }
> > -int qxl_bo_vunmap(struct qxl_bo *bo)
> > +int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo)
> >   {
> >   	int r;
> > @@ -250,6 +256,7 @@ int qxl_bo_vunmap(struct qxl_bo *bo)
> >   		return r;
> >   	qxl_bo_vunmap_locked(bo);
> > +	qxl_bo_unpin_locked(bo);
> >   	qxl_bo_unreserve(bo);
> >   	return 0;
> >   }
> > diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
> > index 1cf5bc759101..875f63221074 100644
> > --- a/drivers/gpu/drm/qxl/qxl_object.h
> > +++ b/drivers/gpu/drm/qxl/qxl_object.h
> > @@ -59,9 +59,9 @@ extern int qxl_bo_create(struct qxl_device *qdev,
> >   			 u32 priority,
> >   			 struct qxl_surface *surf,
> >   			 struct qxl_bo **bo_ptr);
> > -int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map);
> > +int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map);
> >   int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map);
> > -int qxl_bo_vunmap(struct qxl_bo *bo);
> > +int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo);
> >   void qxl_bo_vunmap_locked(struct qxl_bo *bo);
> >   void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset);
> >   void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map);
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/qxl: Pin buffer objects for internal mappings

[Thomas Zimmermann]

Hi

Am 08.07.24 um 10:58 schrieb Daniel Vetter:
> On Mon, Jul 08, 2024 at 10:09:44AM +0200, Thomas Zimmermann wrote:
>> Hi,
>>
>> ping for a review. This is a bugfix for a serious problem.
> I tried to look around whether there's any place where we could WARN_ON if
> we create a vmap but it's not pinned. But there's lots of places where we
> want the vmap only for the duration of the dma_resv locked section, so
> really can't do that. And your patch removes the unlocked vmap
> implementation, which would be the only place really.

Yeah, we often don't want to pin for a vmap. The driver should hold the 
reservation lock while vmap-ing a buffer. That's why I suggested adding 
a local-map interface that creates and removes mappings within the same 
function. It's something for a separate patchset.

>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks.

Best regards
Thomas


>
>> Best regards
>> Thomas
>>
>> Am 02.07.24 um 16:20 schrieb Thomas Zimmermann:
>>> Add qxl_bo_pin_and_vmap() that pins and vmaps a buffer object in one
>>> step. Update callers of the regular qxl_bo_vmap(). Fixes a bug where
>>> qxl accesses an unpinned buffer object while it is being moved; such
>>> as with the monitor-description BO. An typical error is shown below.
>>>
>>> [    4.303586] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0
>>> [    4.586883] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65376256x16777216+0+0
>>> [    4.904036] [drm:drm_atomic_helper_commit_planes] *ERROR* head 1 wrong: 65335296x16777216+0+0
>>> [    5.374347] [drm:qxl_release_from_id_locked] *ERROR* failed to find id in release_idr
>>>
>>> Commit b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap")
>>> removed the implicit pin operation from qxl's vmap code. This is the
>>> correct behavior for GEM and PRIME interfaces, but the pin is still
>>> needed for qxl internal operation.
>>>
>>> Also add a corresponding function qxl_bo_vunmap_and_unpin() and remove
>>> the old qxl_bo_vmap() helpers.
>>>
>>> Future directions: BOs should not be pinned or vmapped unnecessarily.
>>> The pin-and-vmap operation should be removed from the driver and a
>>> temporary mapping should be established with a vmap_local-like helper.
>>> See the client helper drm_client_buffer_vmap_local() for semantics.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Fixes: b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap")
>>> Reported-by: David Kaplan <david.kaplan@amd.com>
>>> Closes: https://lore.kernel.org/dri-devel/ab0fb17d-0f96-4ee6-8b21-65d02bb02655@suse.de/
>>> Tested-by: David Kaplan <david.kaplan@amd.com>
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Zack Rusin <zack.rusin@broadcom.com>
>>> Cc: Dave Airlie <airlied@redhat.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: virtualization@lists.linux.dev
>>> Cc: spice-devel@lists.freedesktop.org
>>> ---
>>>    drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++-------
>>>    drivers/gpu/drm/qxl/qxl_object.c  | 11 +++++++++--
>>>    drivers/gpu/drm/qxl/qxl_object.h  |  4 ++--
>>>    3 files changed, 18 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
>>> index 86a5dea710c0..bc24af08dfcd 100644
>>> --- a/drivers/gpu/drm/qxl/qxl_display.c
>>> +++ b/drivers/gpu/drm/qxl/qxl_display.c
>>> @@ -584,11 +584,11 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
>>>    	if (ret)
>>>    		goto err;
>>> -	ret = qxl_bo_vmap(cursor_bo, &cursor_map);
>>> +	ret = qxl_bo_pin_and_vmap(cursor_bo, &cursor_map);
>>>    	if (ret)
>>>    		goto err_unref;
>>> -	ret = qxl_bo_vmap(user_bo, &user_map);
>>> +	ret = qxl_bo_pin_and_vmap(user_bo, &user_map);
>>>    	if (ret)
>>>    		goto err_unmap;
>>> @@ -614,12 +614,12 @@ static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
>>>    		       user_map.vaddr, size);
>>>    	}
>>> -	qxl_bo_vunmap(user_bo);
>>> -	qxl_bo_vunmap(cursor_bo);
>>> +	qxl_bo_vunmap_and_unpin(user_bo);
>>> +	qxl_bo_vunmap_and_unpin(cursor_bo);
>>>    	return cursor_bo;
>>>    err_unmap:
>>> -	qxl_bo_vunmap(cursor_bo);
>>> +	qxl_bo_vunmap_and_unpin(cursor_bo);
>>>    err_unref:
>>>    	qxl_bo_unpin(cursor_bo);
>>>    	qxl_bo_unref(&cursor_bo);
>>> @@ -1205,7 +1205,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>>>    	}
>>>    	qdev->monitors_config_bo = gem_to_qxl_bo(gobj);
>>> -	ret = qxl_bo_vmap(qdev->monitors_config_bo, &map);
>>> +	ret = qxl_bo_pin_and_vmap(qdev->monitors_config_bo, &map);
>>>    	if (ret)
>>>    		return ret;
>>> @@ -1236,7 +1236,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
>>>    	qdev->monitors_config = NULL;
>>>    	qdev->ram_header->monitors_config = 0;
>>> -	ret = qxl_bo_vunmap(qdev->monitors_config_bo);
>>> +	ret = qxl_bo_vunmap_and_unpin(qdev->monitors_config_bo);
>>>    	if (ret)
>>>    		return ret;
>>> diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
>>> index 5893e27a7ae5..cb1b7c2580ae 100644
>>> --- a/drivers/gpu/drm/qxl/qxl_object.c
>>> +++ b/drivers/gpu/drm/qxl/qxl_object.c
>>> @@ -182,7 +182,7 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map)
>>>    	return 0;
>>>    }
>>> -int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
>>> +int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map)
>>>    {
>>>    	int r;
>>> @@ -190,7 +190,13 @@ int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
>>>    	if (r)
>>>    		return r;
>>> +	r = qxl_bo_pin_locked(bo);
>>> +	if (r)
>>> +		return r;
>>> +
>>>    	r = qxl_bo_vmap_locked(bo, map);
>>> +	if (r)
>>> +		qxl_bo_unpin_locked(bo);
>>>    	qxl_bo_unreserve(bo);
>>>    	return r;
>>>    }
>>> @@ -241,7 +247,7 @@ void qxl_bo_vunmap_locked(struct qxl_bo *bo)
>>>    	ttm_bo_vunmap(&bo->tbo, &bo->map);
>>>    }
>>> -int qxl_bo_vunmap(struct qxl_bo *bo)
>>> +int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo)
>>>    {
>>>    	int r;
>>> @@ -250,6 +256,7 @@ int qxl_bo_vunmap(struct qxl_bo *bo)
>>>    		return r;
>>>    	qxl_bo_vunmap_locked(bo);
>>> +	qxl_bo_unpin_locked(bo);
>>>    	qxl_bo_unreserve(bo);
>>>    	return 0;
>>>    }
>>> diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
>>> index 1cf5bc759101..875f63221074 100644
>>> --- a/drivers/gpu/drm/qxl/qxl_object.h
>>> +++ b/drivers/gpu/drm/qxl/qxl_object.h
>>> @@ -59,9 +59,9 @@ extern int qxl_bo_create(struct qxl_device *qdev,
>>>    			 u32 priority,
>>>    			 struct qxl_surface *surf,
>>>    			 struct qxl_bo **bo_ptr);
>>> -int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map);
>>> +int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map);
>>>    int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map);
>>> -int qxl_bo_vunmap(struct qxl_bo *bo);
>>> +int qxl_bo_vunmap_and_unpin(struct qxl_bo *bo);
>>>    void qxl_bo_vunmap_locked(struct qxl_bo *bo);
>>>    void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset);
>>>    void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map);
>> -- 
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH] drm/qxl: Pin buffer objects for internal mappings

[Dmitry Osipenko]

On 7/2/24 17:20, Thomas Zimmermann wrote:
> @@ -190,7 +190,13 @@ int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
>  	if (r)
>  		return r;
>  
> +	r = qxl_bo_pin_locked(bo);
> +	if (r)
> +		return r;

Is qxl_bo_unreserve() missing here?

> +
>  	r = qxl_bo_vmap_locked(bo, map);
> +	if (r)
> +		qxl_bo_unpin_locked(bo);
>  	qxl_bo_unreserve(bo);
>  	return r;

-- 
Best regards,
Dmitry

Re: [PATCH] drm/qxl: Pin buffer objects for internal mappings

[Thomas Zimmermann]

Hi

Am 08.07.24 um 13:55 schrieb Dmitry Osipenko:
> On 7/2/24 17:20, Thomas Zimmermann wrote:
>> @@ -190,7 +190,13 @@ int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
>>   	if (r)
>>   		return r;
>>   
>> +	r = qxl_bo_pin_locked(bo);
>> +	if (r)
>> +		return r;
> Is qxl_bo_unreserve() missing here?

Yeah, indeed. Thanks for reporting.

Best regards
Thomas

>
>> +
>>   	r = qxl_bo_vmap_locked(bo, map);
>> +	if (r)
>> +		qxl_bo_unpin_locked(bo);
>>   	qxl_bo_unreserve(bo);
>>   	return r;

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)