[PATCH] drm/qxl: Pin buffer objects for internal mappings
Thomas Zimmermann
tzimmermann at suse.de
Mon Jul 8 11:49:04 UTC 2024
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 at 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 at suse.de>
>>> Fixes: b33651a5c98d ("drm/qxl: Do not pin buffer objects for vmap")
>>> Reported-by: David Kaplan <david.kaplan at amd.com>
>>> Closes: https://lore.kernel.org/dri-devel/ab0fb17d-0f96-4ee6-8b21-65d02bb02655@suse.de/
>>> Tested-by: David Kaplan <david.kaplan at amd.com>
>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>>> Cc: Dmitry Osipenko <dmitry.osipenko at collabora.com>
>>> Cc: Christian König <christian.koenig at amd.com>
>>> Cc: Zack Rusin <zack.rusin at broadcom.com>
>>> Cc: Dave Airlie <airlied at redhat.com>
>>> Cc: Gerd Hoffmann <kraxel at redhat.com>
>>> Cc: virtualization at lists.linux.dev
>>> Cc: spice-devel at 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)
More information about the Spice-devel
mailing list