From: Thomas Zimmermann <tzimmermann@suse.de>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: sean@poorly.run, dri-devel@lists.freedesktop.org,
virtualization@lists.linux-foundation.org,
linaro-mm-sig@lists.linaro.org, hdegoede@redhat.com,
kraxel@redhat.com, airlied@redhat.com, sam@ravnborg.org,
christian.koenig@amd.com, linux-media@vger.kernel.org
Subject: Re: [PATCH v4 04/13] drm/shmem-helper: Provide a vmap function for short-term mappings
Date: Tue, 12 Jan 2021 14:11:24 +0100 [thread overview]
Message-ID: <8d81f20b-3dc1-475b-2ee5-cff901db33c5@suse.de> (raw)
In-Reply-To: <X/yB3LC79f/zWTwG@phenom.ffwll.local>
[-- Attachment #1.1.1: Type: text/plain, Size: 10385 bytes --]
Hi
Am 11.01.21 um 17:50 schrieb Daniel Vetter:
> On Fri, Jan 08, 2021 at 10:43:31AM +0100, Thomas Zimmermann wrote:
>> Implementations of the vmap/vunmap GEM callbacks may perform pinning
>> of the BO and may acquire the associated reservation object's lock.
>> Callers that only require a mapping of the contained memory can thus
>> interfere with other tasks that require exact pinning, such as scanout.
>> This is less of an issue with private SHMEM buffers, but may happen
>> with imported ones.
>>
>> Therefore provide the new interfaces drm_gem_shmem_vmap_local() and
>> drm_gem_shmem_vunmap_local(), which only perform the vmap/vunmap
>> operations. Callers have to hold the reservation lock while the mapping
>> persists.
>>
>> This patch also connects GEM SHMEM helpers to GEM object functions with
>> equivalent functionality.
>>
>> v4:
>> * call dma_buf_{vmap,vunmap}_local() where necessary (Daniel)
>> * move driver changes into separate patches (Daniel)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/gpu/drm/drm_gem_shmem_helper.c | 90 +++++++++++++++++++++++---
>> include/drm/drm_gem_shmem_helper.h | 2 +
>> 2 files changed, 84 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index 9825c378dfa6..298832b2b43b 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -32,6 +32,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>> .get_sg_table = drm_gem_shmem_get_sg_table,
>> .vmap = drm_gem_shmem_vmap,
>> .vunmap = drm_gem_shmem_vunmap,
>> + .vmap_local = drm_gem_shmem_vmap_local,
>> + .vunmap_local = drm_gem_shmem_vunmap_local,
>> .mmap = drm_gem_shmem_mmap,
>> };
>>
>> @@ -261,7 +263,8 @@ void drm_gem_shmem_unpin(struct drm_gem_object *obj)
>> }
>> EXPORT_SYMBOL(drm_gem_shmem_unpin);
>>
>> -static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct dma_buf_map *map)
>> +static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct dma_buf_map *map,
>> + bool local)
>
> This is a bit spaghetti and also has the problem that we're not changing
> shmem->vmap_use_count under different locks, depending upon which path
> we're taking.
>
> I think the cleanest would be if we pull the if (import_attach) case out
> of the _locked() version completely, for all cases, and also outside of
> the shmem->vmap_lock. This means no caching of vmaps in the shmem layer
> anymore for imported buffers, but this is no longer a problem: We cache
> them in the exporters instead (I think at least, if not maybe need to fix
> that where it's expensive).
If we do that, what protects shmem->vaddr from concurrent access near
line 281? would it be kept NULL then?
Also, we have some stats in debugfs (see drm_gem_shmem_print_info) which
would be incorrect (or misleading at least).
Given all that, would it be possible to remove vmap_lock in favor of
taking the resv lock in vmap/vunmap?
Best regards
Thomas
>
> Other option would be to unly pull it out for the _vmap_local case, but
> that's a bit ugly because no longer symmetrical in the various paths.
>
>> {
>> struct drm_gem_object *obj = &shmem->base;
>> int ret = 0;
>> @@ -272,7 +275,10 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct
>> }
>>
>> if (obj->import_attach) {
>> - ret = dma_buf_vmap(obj->import_attach->dmabuf, map);
>> + if (local)
>> + ret = dma_buf_vmap_local(obj->import_attach->dmabuf, map);
>> + else
>> + ret = dma_buf_vmap(obj->import_attach->dmabuf, map);
>> if (!ret) {
>> if (WARN_ON(map->is_iomem)) {
>> ret = -EIO;
>> @@ -313,7 +319,7 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, struct
>> return ret;
>> }
>>
>> -/*
>> +/**
>> * drm_gem_shmem_vmap - Create a virtual mapping for a shmem GEM object
>> * @shmem: shmem GEM object
>> * @map: Returns the kernel virtual address of the SHMEM GEM object's backing
>> @@ -339,15 +345,53 @@ int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
>> ret = mutex_lock_interruptible(&shmem->vmap_lock);
>> if (ret)
>> return ret;
>> - ret = drm_gem_shmem_vmap_locked(shmem, map);
>> + ret = drm_gem_shmem_vmap_locked(shmem, map, false);
>> mutex_unlock(&shmem->vmap_lock);
>>
>> return ret;
>> }
>> EXPORT_SYMBOL(drm_gem_shmem_vmap);
>>
>> +/**
>> + * drm_gem_shmem_vmap_local - Create a virtual mapping for a shmem GEM object
>> + * @shmem: shmem GEM object
>> + * @map: Returns the kernel virtual address of the SHMEM GEM object's backing
>> + * store.
>> + *
>> + * This function makes sure that a contiguous kernel virtual address mapping
>> + * exists for the buffer backing the shmem GEM object.
>> + *
>> + * The function is called with the BO's reservation object locked. Callers must
>> + * hold the lock until after unmapping the buffer.
>> + *
>> + * This function can be used to implement &drm_gem_object_funcs.vmap_local. But
>> + * it can also be called by drivers directly, in which case it will hide the
>> + * differences between dma-buf imported and natively allocated objects.
>
> So for the other callbacks I tried to make sure we have different entry
> points for this, since it's not really the same thing and because of the
> locking mess we have with dma_resv_lock vs various pre-existing local
> locking scheme, it's easy to get a mess.
>
> I think the super clean version here would be to also export just the
> internal stuff for the ->v(un)map_local hooks, but that's maybe a bit too
> much boilerplate for no real gain.
> -Daniel
>
>> + *
>> + * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap_local().
>> + *
>> + * Returns:
>> + * 0 on success or a negative error code on failure.
>> + */
>> +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map)
>> +{
>> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>> + int ret;
>> +
>> + dma_resv_assert_held(obj->resv);
>> +
>> + ret = mutex_lock_interruptible(&shmem->vmap_lock);
>> + if (ret)
>> + return ret;
>> + ret = drm_gem_shmem_vmap_locked(shmem, map, true);
>> + mutex_unlock(&shmem->vmap_lock);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_vmap_local);
>> +
>> static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
>> - struct dma_buf_map *map)
>> + struct dma_buf_map *map, bool local)
>> {
>> struct drm_gem_object *obj = &shmem->base;
>>
>> @@ -358,7 +402,10 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
>> return;
>>
>> if (obj->import_attach)
>> - dma_buf_vunmap(obj->import_attach->dmabuf, map);
>> + if (local)
>> + dma_buf_vunmap_local(obj->import_attach->dmabuf, map);
>> + else
>> + dma_buf_vunmap(obj->import_attach->dmabuf, map);
>> else
>> vunmap(shmem->vaddr);
>>
>> @@ -366,7 +413,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
>> drm_gem_shmem_put_pages(shmem);
>> }
>>
>> -/*
>> +/**
>> * drm_gem_shmem_vunmap - Unmap a virtual mapping fo a shmem GEM object
>> * @shmem: shmem GEM object
>> * @map: Kernel virtual address where the SHMEM GEM object was mapped
>> @@ -384,11 +431,38 @@ void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map)
>> struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>>
>> mutex_lock(&shmem->vmap_lock);
>> - drm_gem_shmem_vunmap_locked(shmem, map);
>> + drm_gem_shmem_vunmap_locked(shmem, map, false);
>> mutex_unlock(&shmem->vmap_lock);
>> }
>> EXPORT_SYMBOL(drm_gem_shmem_vunmap);
>>
>> +/**
>> + * drm_gem_shmem_vunmap_local - Unmap a virtual mapping fo a shmem GEM object
>> + * @shmem: shmem GEM object
>> + * @map: Kernel virtual address where the SHMEM GEM object was mapped
>> + *
>> + * This function cleans up a kernel virtual address mapping acquired by
>> + * drm_gem_shmem_vmap_local(). The mapping is only removed when the use count
>> + * drops to zero.
>> + *
>> + * The function is called with the BO's reservation object locked.
>> + *
>> + * This function can be used to implement &drm_gem_object_funcs.vmap_local.
>> + * But it can also be called by drivers directly, in which case it will hide
>> + * the differences between dma-buf imported and natively allocated objects.
>> + */
>> +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map)
>> +{
>> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>> +
>> + dma_resv_assert_held(obj->resv);
>> +
>> + mutex_lock(&shmem->vmap_lock);
>> + drm_gem_shmem_vunmap_locked(shmem, map, true);
>> + mutex_unlock(&shmem->vmap_lock);
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_vunmap_local);
>> +
>> struct drm_gem_shmem_object *
>> drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
>> struct drm_device *dev, size_t size,
>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>> index 434328d8a0d9..3f59bdf749aa 100644
>> --- a/include/drm/drm_gem_shmem_helper.h
>> +++ b/include/drm/drm_gem_shmem_helper.h
>> @@ -114,7 +114,9 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem);
>> int drm_gem_shmem_pin(struct drm_gem_object *obj);
>> void drm_gem_shmem_unpin(struct drm_gem_object *obj);
>> int drm_gem_shmem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map);
>> +int drm_gem_shmem_vmap_local(struct drm_gem_object *obj, struct dma_buf_map *map);
>> void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map);
>> +void drm_gem_shmem_vunmap_local(struct drm_gem_object *obj, struct dma_buf_map *map);
>>
>> int drm_gem_shmem_madvise(struct drm_gem_object *obj, int madv);
>>
>> --
>> 2.29.2
>>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-01-12 13:11 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-08 9:43 [PATCH v4 00/13] drm: Support short-term vmap via vmap_local Thomas Zimmermann
2021-01-08 9:43 ` [PATCH v4 01/13] dma-buf: Add vmap_local and vnumap_local operations Thomas Zimmermann
2021-01-08 16:12 ` Ruhl, Michael J
2021-01-12 7:45 ` Thomas Zimmermann
2021-01-14 14:39 ` Ruhl, Michael J
2021-01-08 9:43 ` [PATCH v4 02/13] drm/gem: Create infrastructure for GEM vmap_local Thomas Zimmermann
2021-01-08 9:43 ` [PATCH v4 03/13] drm/cma-helper: Provide a vmap function for short-term mappings Thomas Zimmermann
2021-01-08 9:43 ` [PATCH v4 04/13] drm/shmem-helper: " Thomas Zimmermann
2021-01-11 16:50 ` Daniel Vetter
2021-01-12 13:11 ` Thomas Zimmermann [this message]
2021-01-12 14:16 ` Daniel Vetter
2021-01-27 12:08 ` Thomas Zimmermann
2021-02-02 14:11 ` Daniel Vetter
2021-01-08 9:43 ` [PATCH v4 05/13] drm/mgag200: Use drm_gem_shmem_vmap_local() in damage handling Thomas Zimmermann
2021-01-11 16:53 ` Daniel Vetter
2021-01-11 16:58 ` Daniel Vetter
2021-01-08 9:43 ` [PATCH v4 06/13] drm/cirrus: " Thomas Zimmermann
2021-01-11 17:00 ` Daniel Vetter
2021-01-11 17:03 ` Daniel Vetter
2021-01-08 9:43 ` [PATCH v4 07/13] drm/gm12u320: " Thomas Zimmermann
2021-01-11 17:01 ` Daniel Vetter
2021-01-08 9:43 ` [PATCH v4 08/13] drm/udl: " Thomas Zimmermann
2021-01-08 9:43 ` [PATCH v4 09/13] drm/vram-helper: Provide a vmap function for short-term mappings Thomas Zimmermann
2021-01-08 9:43 ` [PATCH v4 10/13] drm/ast: Use drm_gem_vram_vmap_local() in cursor update Thomas Zimmermann
2021-01-08 9:43 ` [PATCH v4 11/13] drm/vboxvideo: " Thomas Zimmermann
2021-01-11 17:06 ` Daniel Vetter
2021-01-12 7:54 ` Thomas Zimmermann
2021-01-12 9:17 ` Daniel Vetter
2021-01-12 9:53 ` Thomas Zimmermann
2021-01-08 9:43 ` [PATCH v4 12/13] drm/fb-helper: Move BO locking from DRM client to fbdev damage worker Thomas Zimmermann
2021-01-08 9:43 ` [PATCH v4 13/13] drm/vram-helper: Remove unused drm_gem_vram_{vmap, vunmap}() Thomas Zimmermann
2021-01-11 16:52 ` [PATCH v4 13/13] drm/vram-helper: Remove unused drm_gem_vram_{vmap,vunmap}() Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8d81f20b-3dc1-475b-2ee5-cff901db33c5@suse.de \
--to=tzimmermann@suse.de \
--cc=airlied@redhat.com \
--cc=christian.koenig@amd.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=hdegoede@redhat.com \
--cc=kraxel@redhat.com \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-media@vger.kernel.org \
--cc=sam@ravnborg.org \
--cc=sean@poorly.run \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).