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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 8, 2024 at 10:22 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> 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.
>
> v2:
> - unreserve BO on errors in qxl_bo_pin_and_vmap() (Dmitry)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Fixes: b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap")
> Reported-by: David Kaplan <david.kaplan@xxxxxxx>
> Closes: https://lore.kernel.org/dri-devel/ab0fb17d-0f96-4ee6-8b21-65d02bb02655@xxxxxxx/
> Tested-by: David Kaplan <david.kaplan@xxxxxxx>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Cc: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Zack Rusin <zack.rusin@xxxxxxxxxxxx>
> Cc: Dave Airlie <airlied@xxxxxxxxxx>
> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> Cc: virtualization@xxxxxxxxxxxxxxx
> Cc: spice-devel@xxxxxxxxxxxxxxxxxxxxx
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 14 +++++++-------
>  drivers/gpu/drm/qxl/qxl_object.c  | 13 +++++++++++--
>  drivers/gpu/drm/qxl/qxl_object.h  |  4 ++--
>  3 files changed, 20 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..66635c55cf85 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,15 @@ int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
>         if (r)
>                 return r;
>
> +       r = qxl_bo_pin_locked(bo);
> +       if (r) {
> +               qxl_bo_unreserve(bo);
> +               return r;
> +       }
> +
>         r = qxl_bo_vmap_locked(bo, map);
> +       if (r)
> +               qxl_bo_unpin_locked(bo);
>         qxl_bo_unreserve(bo);
>         return r;
>  }
> @@ -241,7 +249,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 +258,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
>

That looks good to me.

Reviewed-by: Zack Rusin <zack.rusin@xxxxxxxxxxxx>

z




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]