Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

13 participants
3239 discussions

Re: [Linaro-mm-sig] [PATCH v4 04/13] drm/shmem-helper: Provide a vmap function for short-term mappings

by 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(a)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). 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 11 months

Re: [Linaro-mm-sig] [PATCH v4 01/13] dma-buf: Add vmap_local and vnumap_local operations

by Ruhl, Michael J

>-----Original Message----- >From: dri-devel <dri-devel-bounces(a)lists.freedesktop.org> On Behalf Of >Thomas Zimmermann >Sent: Friday, January 8, 2021 4:43 AM >To: sumit.semwal(a)linaro.org; christian.koenig(a)amd.com; >airlied(a)redhat.com; daniel(a)ffwll.ch; maarten.lankhorst(a)linux.intel.com; >mripard(a)kernel.org; kraxel(a)redhat.com; hdegoede(a)redhat.com; >sean(a)poorly.run; eric(a)anholt.net; sam(a)ravnborg.org >Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch>; dri-devel(a)lists.freedesktop.org; >virtualization(a)lists.linux-foundation.org; linaro-mm-sig(a)lists.linaro.org; >Thomas Zimmermann <tzimmermann(a)suse.de>; linux- >media(a)vger.kernel.org >Subject: [PATCH v4 01/13] dma-buf: Add vmap_local and vnumap_local >operations > >The existing dma-buf calls dma_buf_vmap() and dma_buf_vunmap() are >allowed to pin the buffer or acquire the buffer's reservation object >lock. > >This is a problem for callers that only require a short-term mapping >of the buffer without the pinning, or callers that have special locking >requirements. These may suffer from unnecessary overhead or interfere >with regular pin operations. > >The new interfaces dma_buf_vmap_local(), dma_buf_vunmapo_local(), and >their rsp callbacks in struct dma_buf_ops provide an alternative without >pinning or reservation locking. Callers are responsible for these >operations. > >v4: > * update documentation (Daniel) > >Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> >Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> >Suggested-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> >--- > drivers/dma-buf/dma-buf.c | 81 >+++++++++++++++++++++++++++++++++++++++ > include/linux/dma-buf.h | 34 ++++++++++++++++ > 2 files changed, 115 insertions(+) > >diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >index b8465243eca2..01f9c74d97fa 100644 >--- a/drivers/dma-buf/dma-buf.c >+++ b/drivers/dma-buf/dma-buf.c >@@ -1295,6 +1295,87 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, >struct dma_buf_map *map) > } > EXPORT_SYMBOL_GPL(dma_buf_vunmap); > >+/** >+ * dma_buf_vmap_local - Create virtual mapping for the buffer object into >kernel >+ * address space. >+ * @dmabuf: [in] buffer to vmap >+ * @map: [out] returns the vmap pointer >+ * >+ * Unlike dma_buf_vmap() this is a short term mapping and will not pin >+ * the buffer. The struct dma_resv for the @dmabuf must be locked until >+ * dma_buf_vunmap_local() is called. >+ * >+ * Returns: >+ * 0 on success, or a negative errno code otherwise. >+ */ >+int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map >*map) >+{ >+ struct dma_buf_map ptr; >+ int ret = 0; >+ >+ dma_buf_map_clear(map); >+ >+ if (WARN_ON(!dmabuf)) >+ return -EINVAL; >+ >+ dma_resv_assert_held(dmabuf->resv); >+ >+ if (!dmabuf->ops->vmap_local) >+ return -EINVAL; You are clearing the map, and then doing the above checks. Is it ok to change the map info and then exit on error? Mike >+ mutex_lock(&dmabuf->lock); >+ if (dmabuf->vmapping_counter) { >+ dmabuf->vmapping_counter++; >+ BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); >+ *map = dmabuf->vmap_ptr; >+ goto out_unlock; >+ } >+ >+ BUG_ON(dma_buf_map_is_set(&dmabuf->vmap_ptr)); >+ >+ ret = dmabuf->ops->vmap_local(dmabuf, &ptr); >+ if (WARN_ON_ONCE(ret)) >+ goto out_unlock; >+ >+ dmabuf->vmap_ptr = ptr; >+ dmabuf->vmapping_counter = 1; >+ >+ *map = dmabuf->vmap_ptr; >+ >+out_unlock: >+ mutex_unlock(&dmabuf->lock); >+ return ret; >+} >+EXPORT_SYMBOL_GPL(dma_buf_vmap_local); >+ >+/** >+ * dma_buf_vunmap_local - Unmap a vmap obtained by >dma_buf_vmap_local. >+ * @dmabuf: [in] buffer to vunmap >+ * @map: [in] vmap pointer to vunmap >+ * >+ * Release a mapping established with dma_buf_vmap_local(). >+ */ >+void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct >dma_buf_map *map) >+{ >+ if (WARN_ON(!dmabuf)) >+ return; >+ >+ dma_resv_assert_held(dmabuf->resv); >+ >+ BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); >+ BUG_ON(dmabuf->vmapping_counter == 0); >+ BUG_ON(!dma_buf_map_is_equal(&dmabuf->vmap_ptr, map)); >+ >+ mutex_lock(&dmabuf->lock); >+ if (--dmabuf->vmapping_counter == 0) { >+ if (dmabuf->ops->vunmap_local) >+ dmabuf->ops->vunmap_local(dmabuf, map); >+ dma_buf_map_clear(&dmabuf->vmap_ptr); >+ } >+ mutex_unlock(&dmabuf->lock); >+} >+EXPORT_SYMBOL_GPL(dma_buf_vunmap_local); >+ > #ifdef CONFIG_DEBUG_FS > static int dma_buf_debug_show(struct seq_file *s, void *unused) > { >diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >index 628681bf6c99..aeed754b5467 100644 >--- a/include/linux/dma-buf.h >+++ b/include/linux/dma-buf.h >@@ -264,6 +264,38 @@ struct dma_buf_ops { > > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map >*map); >+ >+ /** >+ * @vmap_local: >+ * >+ * Creates a virtual mapping for the buffer into kernel address space. >+ * >+ * This callback establishes short-term mappings for situations where >+ * callers only use the buffer for a bounded amount of time; such as >+ * updates to the framebuffer or reading back contained information. >+ * In contrast to the regular @vmap callback, vmap_local does never >pin >+ * the buffer to a specific domain or acquire the buffer's reservation >+ * lock. >+ * >+ * This is called with the &dma_buf.resv object locked. Callers must >hold >+ * the lock until after removing the mapping with @vunmap_local. >+ * >+ * This callback is optional. >+ * >+ * Returns: >+ * >+ * 0 on success or a negative error code on failure. >+ */ >+ int (*vmap_local)(struct dma_buf *dmabuf, struct dma_buf_map >*map); >+ >+ /** >+ * @vunmap_local: >+ * >+ * Removes a virtual mapping that was established by @vmap_local. >+ * >+ * This callback is optional. >+ */ >+ void (*vunmap_local)(struct dma_buf *dmabuf, struct dma_buf_map >*map); > }; > > /** >@@ -501,4 +533,6 @@ int dma_buf_mmap(struct dma_buf *, struct >vm_area_struct *, > unsigned long); > int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map >*map); >+int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map >*map); >+void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct >dma_buf_map *map); > #endif /* __DMA_BUF_H__ */ >-- >2.29.2 > >_______________________________________________ >dri-devel mailing list >dri-devel(a)lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel

4 years, 11 months

Re: [Linaro-mm-sig] [PATCH v3 6/8] drm/shmem-helper: Provide a vmap function for short-term mappings

by Daniel Vetter

On Thu, Jan 7, 2021 at 11:28 AM Thomas Zimmermann <tzimmermann(a)suse.de> wrote: > > Hi Daniel > > Am 11.12.20 um 10:50 schrieb Daniel Vetter: > [...] > >> +/** > >> + * 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. > >> + * > >> + * 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); > > > > This bites. You need to check for shmem->import_attach and call > > dma_buf_vmap_local directly here before you take any shmem helper locks. > > Real fix would be to replace both vmap_lock and pages_lock with dma_resv > > lock, but that's more work. Same for vunmap_local > > This comment confuses me. AFAICT vmap_lock protects vmap_use_count. Why > does this exact code work in drm_gem_shmem_vmap() but not in _local() ? You'd hold the dma_resv lock both inside and outside of the ->vmap_lock mutex. Which deadlocks. For the _vmap version we more or less mandate that the caller doens't hold the dma_resv lock already, hence why we can get away with that. -Daniel > Best regards > Thomas > > > > > With that fixed on the helper part of this patch: > > > > Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > > > > > >> + if (ret) > >> + return ret; > >> + ret = drm_gem_shmem_vmap_locked(shmem, map); > >> + 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) > >> { > >> @@ -366,7 +406,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 > >> @@ -389,6 +429,33 @@ void drm_gem_shmem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map) > >> } > >> 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); > >> + 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/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > >> index 1dfc42170059..a33e28d4c5e9 100644 > >> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > >> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > >> @@ -1552,22 +1552,32 @@ mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb, > >> struct drm_rect *clip) > >> { > >> struct drm_device *dev = &mdev->base; > >> + struct drm_gem_object *obj = fb->obj[0]; > >> struct dma_buf_map map; > >> void *vmap; > >> int ret; > >> > >> - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > >> + ret = dma_resv_lock(obj->resv, NULL); > >> if (drm_WARN_ON(dev, ret)) > >> - return; /* BUG: SHMEM BO should always be vmapped */ > >> + return; > >> + ret = drm_gem_shmem_vmap_local(obj, &map); > >> + if (drm_WARN_ON(dev, ret)) > >> + goto err_dma_resv_unlock; /* BUG: SHMEM BO should always be vmapped */ > >> vmap = map.vaddr; /* TODO: Use mapping abstraction properly */ > >> > >> drm_fb_memcpy_dstclip(mdev->vram, vmap, fb, clip); > >> > >> - drm_gem_shmem_vunmap(fb->obj[0], &map); > >> + drm_gem_shmem_vunmap_local(obj, &map); > >> + dma_resv_unlock(obj->resv); > >> > >> /* Always scanout image at VRAM offset 0 */ > >> mgag200_set_startadd(mdev, (u32)0); > >> mgag200_set_offset(mdev, fb); > >> + > >> + return; > >> + > >> +err_dma_resv_unlock: > >> + dma_resv_unlock(obj->resv); > >> } > >> > >> static void > >> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c > >> index 561c49d8657a..58c694964148 100644 > >> --- a/drivers/gpu/drm/tiny/cirrus.c > >> +++ b/drivers/gpu/drm/tiny/cirrus.c > >> @@ -315,6 +315,7 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > >> struct drm_rect *rect) > >> { > >> struct cirrus_device *cirrus = to_cirrus(fb->dev); > >> + struct drm_gem_object *obj = fb->obj[0]; > >> struct dma_buf_map map; > >> void *vmap; > >> int idx, ret; > >> @@ -323,9 +324,12 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > >> if (!drm_dev_enter(&cirrus->dev, &idx)) > >> goto out; > >> > >> - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > >> + ret = dma_resv_lock(obj->resv, NULL); > >> if (ret) > >> goto out_dev_exit; > >> + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > >> + if (ret) > >> + goto out_dma_resv_unlock; > >> vmap = map.vaddr; /* TODO: Use mapping abstraction properly */ > >> > >> if (cirrus->cpp == fb->format->cpp[0]) > >> @@ -345,9 +349,11 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, > >> else > >> WARN_ON_ONCE("cpp mismatch"); > >> > >> - drm_gem_shmem_vunmap(fb->obj[0], &map); > >> ret = 0; > >> > >> + drm_gem_shmem_vunmap_local(obj, &map); > >> +out_dma_resv_unlock: > >> + dma_resv_unlock(obj->resv); > >> out_dev_exit: > >> drm_dev_exit(idx); > >> out: > >> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c > >> index 33f65f4626e5..b0c6e350f2b3 100644 > >> --- a/drivers/gpu/drm/tiny/gm12u320.c > >> +++ b/drivers/gpu/drm/tiny/gm12u320.c > >> @@ -265,11 +265,16 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320) > >> y1 = gm12u320->fb_update.rect.y1; > >> y2 = gm12u320->fb_update.rect.y2; > >> > >> - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > >> + ret = dma_resv_lock(fb->obj[0]->resv, NULL); > >> if (ret) { > >> - GM12U320_ERR("failed to vmap fb: %d\n", ret); > >> + GM12U320_ERR("failed to reserve fb: %d\n", ret); > >> goto put_fb; > >> } > >> + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > >> + if (ret) { > >> + GM12U320_ERR("failed to vmap fb: %d\n", ret); > >> + goto unlock_resv; > >> + } > >> vaddr = map.vaddr; /* TODO: Use mapping abstraction properly */ > >> > >> if (fb->obj[0]->import_attach) { > >> @@ -321,8 +326,11 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320) > >> if (ret) > >> GM12U320_ERR("dma_buf_end_cpu_access err: %d\n", ret); > >> } > >> + > >> +unlock_resv: > >> + dma_resv_unlock(fb->obj[0]->resv); > >> vunmap: > >> - drm_gem_shmem_vunmap(fb->obj[0], &map); > >> + drm_gem_shmem_vunmap_local(fb->obj[0], &map); > >> put_fb: > >> drm_framebuffer_put(fb); > >> gm12u320->fb_update.fb = NULL; > >> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c > >> index 9d34ec9d03f6..46b55b4d03c2 100644 > >> --- a/drivers/gpu/drm/udl/udl_modeset.c > >> +++ b/drivers/gpu/drm/udl/udl_modeset.c > >> @@ -290,14 +290,18 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > >> else if ((clip.x2 > fb->width) || (clip.y2 > fb->height)) > >> return -EINVAL; > >> > >> + ret = dma_resv_lock(fb->obj[0]->resv, NULL); > >> + if (ret) > >> + return ret; > >> + > >> if (import_attach) { > >> ret = dma_buf_begin_cpu_access(import_attach->dmabuf, > >> DMA_FROM_DEVICE); > >> if (ret) > >> - return ret; > >> + goto out_dma_resv_unlock; > >> } > >> > >> - ret = drm_gem_shmem_vmap(fb->obj[0], &map); > >> + ret = drm_gem_shmem_vmap_local(fb->obj[0], &map); > >> if (ret) { > >> DRM_ERROR("failed to vmap fb\n"); > >> goto out_dma_buf_end_cpu_access; > >> @@ -307,7 +311,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > >> urb = udl_get_urb(dev); > >> if (!urb) { > >> ret = -ENOMEM; > >> - goto out_drm_gem_shmem_vunmap; > >> + goto out_drm_gem_shmem_vunmap_local; > >> } > >> cmd = urb->transfer_buffer; > >> > >> @@ -320,7 +324,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > >> &cmd, byte_offset, dev_byte_offset, > >> byte_width); > >> if (ret) > >> - goto out_drm_gem_shmem_vunmap; > >> + goto out_drm_gem_shmem_vunmap_local; > >> } > >> > >> if (cmd > (char *)urb->transfer_buffer) { > >> @@ -336,8 +340,8 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > >> > >> ret = 0; > >> > >> -out_drm_gem_shmem_vunmap: > >> - drm_gem_shmem_vunmap(fb->obj[0], &map); > >> +out_drm_gem_shmem_vunmap_local: > >> + drm_gem_shmem_vunmap_local(fb->obj[0], &map); > >> out_dma_buf_end_cpu_access: > >> if (import_attach) { > >> tmp_ret = dma_buf_end_cpu_access(import_attach->dmabuf, > >> @@ -345,6 +349,8 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y, > >> if (tmp_ret && !ret) > >> ret = tmp_ret; /* only update ret if not set yet */ > >> } > >> +out_dma_resv_unlock: > >> + dma_resv_unlock(fb->obj[0]->resv); > >> > >> return ret; > >> } > >> 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 11 months

[PATCH V2] dmabuf: fix use-after-free of dmabuf's file->f_inode

by Charan Teja Reddy

It is observed 'use-after-free' on the dmabuf's file->f_inode with the race between closing the dmabuf file and reading the dmabuf's debug info. Consider the below scenario where P1 is closing the dma_buf file and P2 is reading the dma_buf's debug info in the system: P1 P2 dma_buf_debug_show() dma_buf_put() __fput() file->f_op->release() dput() .... dentry_unlink_inode() iput(dentry->d_inode) (where the inode is freed) mutex_lock(&db_list.lock) read 'dma_buf->file->f_inode' (the same inode is freed by P1) mutex_unlock(&db_list.lock) dentry->d_op->d_release()--> dma_buf_release() ..... mutex_lock(&db_list.lock) removes the dmabuf from the list mutex_unlock(&db_list.lock) In the above scenario, when dma_buf_put() is called on a dma_buf, it first frees the dma_buf's file->f_inode(=dentry->d_inode) and then removes this dma_buf from the system db_list. In between P2 traversing the db_list tries to access this dma_buf's file->f_inode that was freed by P1 which is a use-after-free case. Since, __fput() calls f_op->release first and then later calls the d_op->d_release, move the dma_buf's db_list removal from d_release() to f_op->release(). This ensures that dma_buf's file->f_inode is not accessed after it is released. Cc: <stable(a)vger.kernel.org> # 5.4+ Fixes: 4ab59c3c638c ("dma-buf: Move dma_buf_release() from fops to dentry_ops") Acked-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> --- V2: Resending with stable tags and Acks V1: https://lore.kernel.org/patchwork/patch/1360118/ drivers/dma-buf/dma-buf.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 0eb80c1..a14dcbb 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -76,10 +76,6 @@ static void dma_buf_release(struct dentry *dentry) dmabuf->ops->release(dmabuf); - mutex_lock(&db_list.lock); - list_del(&dmabuf->list_node); - mutex_unlock(&db_list.lock); - if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); @@ -88,6 +84,22 @@ static void dma_buf_release(struct dentry *dentry) kfree(dmabuf); } +static int dma_buf_file_release(struct inode *inode, struct file *file) +{ + struct dma_buf *dmabuf; + + if (!is_dma_buf_file(file)) + return -EINVAL; + + dmabuf = file->private_data; + + mutex_lock(&db_list.lock); + list_del(&dmabuf->list_node); + mutex_unlock(&db_list.lock); + + return 0; +} + static const struct dentry_operations dma_buf_dentry_ops = { .d_dname = dmabuffs_dname, .d_release = dma_buf_release, @@ -413,6 +425,7 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) } static const struct file_operations dma_buf_fops = { + .release = dma_buf_file_release, .mmap = dma_buf_mmap_internal, .llseek = dma_buf_llseek, .poll = dma_buf_poll, -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation

4 years, 11 months

[PATCH] dmabuf: fix use-after-free of dmabuf's file->f_inode

by Charan Teja Reddy

It is observed 'use-after-free' on the dmabuf's file->f_inode with the race between closing the dmabuf file and reading the dmabuf's debug info. Consider the below scenario where P1 is closing the dma_buf file and P2 is reading the dma_buf's debug info in the system: P1 P2 dma_buf_debug_show() dma_buf_put() __fput() file->f_op->release() dput() .... dentry_unlink_inode() iput(dentry->d_inode) (where the inode is freed) mutex_lock(&db_list.lock) read 'dma_buf->file->f_inode' (the same inode is freed by P1) mutex_unlock(&db_list.lock) dentry->d_op->d_release()--> dma_buf_release() ..... mutex_lock(&db_list.lock) removes the dmabuf from the list mutex_unlock(&db_list.lock) In the above scenario, when dma_buf_put() is called on a dma_buf, it first frees the dma_buf's file->f_inode(=dentry->d_inode) and then removes this dma_buf from the system db_list. In between P2 traversing the db_list tries to access this dma_buf's file->f_inode that was freed by P1 which is a use-after-free case. Since, __fput() calls f_op->release first and then later calls the d_op->d_release, move the dma_buf's db_list removal from d_release() to f_op->release(). This ensures that dma_buf's file->f_inode is not accessed after it is released. Fixes: 4ab59c3c638c ("dma-buf: Move dma_buf_release() from fops to dentry_ops") Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> --- drivers/dma-buf/dma-buf.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 0eb80c1..a14dcbb 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -76,10 +76,6 @@ static void dma_buf_release(struct dentry *dentry) dmabuf->ops->release(dmabuf); - mutex_lock(&db_list.lock); - list_del(&dmabuf->list_node); - mutex_unlock(&db_list.lock); - if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); @@ -88,6 +84,22 @@ static void dma_buf_release(struct dentry *dentry) kfree(dmabuf); } +static int dma_buf_file_release(struct inode *inode, struct file *file) +{ + struct dma_buf *dmabuf; + + if (!is_dma_buf_file(file)) + return -EINVAL; + + dmabuf = file->private_data; + + mutex_lock(&db_list.lock); + list_del(&dmabuf->list_node); + mutex_unlock(&db_list.lock); + + return 0; +} + static const struct dentry_operations dma_buf_dentry_ops = { .d_dname = dmabuffs_dname, .d_release = dma_buf_release, @@ -413,6 +425,7 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) } static const struct file_operations dma_buf_fops = { + .release = dma_buf_file_release, .mmap = dma_buf_mmap_internal, .llseek = dma_buf_llseek, .poll = dma_buf_poll, -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation

4 years, 11 months

Re: [Linaro-mm-sig] [PATCH] cma_heap: fix implicit function declaration

by John Stultz

On Thu, Dec 17, 2020 at 4:31 AM <siyanteng01(a)gmail.com> wrote: > > From: siyanteng <siyanteng01(a)gmail.com> > > When building cma_heap the following error shows up: > > drivers/dma-buf/heaps/cma_heap.c:195:10: error: implicit declaration of function 'vmap'; did you mean 'kmap'? [-Werror=implicit-function-declaration] > 195 | vaddr = vmap(buffer->pages, buffer->pagecount, VM_MAP, PAGE_KERNEL); > | ^~~~ > | kmap > > Use this include: linux-next/include/linux/vmalloc.h > > Signed-off-by: siyanteng <siyanteng01(a)gmail.com> Thanks for submitting this! My apologies for the trouble! We already have a similar patch queued here: https://cgit.freedesktop.org/drm/drm-misc/commit/?h=drm-misc-next-fixes&id=… so hopefully that will land upstream soon. thanks again! -john

4 years, 12 months

[PATCH 1/4] dma-buf: Remove kmap kerneldoc vestiges

by Daniel Vetter

Also try to clarify a bit when dma_buf_begin/end_cpu_access should be called. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/dma-buf.c | 20 ++++++++++++++------ include/linux/dma-buf.h | 25 +++++++++---------------- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index e63684d4cd90..a12fdffa130f 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1001,15 +1001,15 @@ EXPORT_SYMBOL_GPL(dma_buf_move_notify); * vmalloc space might be limited and result in vmap calls failing. * * Interfaces:: + * * void \*dma_buf_vmap(struct dma_buf \*dmabuf) * void dma_buf_vunmap(struct dma_buf \*dmabuf, void \*vaddr) * * The vmap call can fail if there is no vmap support in the exporter, or if - * it runs out of vmalloc space. Fallback to kmap should be implemented. Note - * that the dma-buf layer keeps a reference count for all vmap access and - * calls down into the exporter's vmap function only when no vmapping exists, - * and only unmaps it once. Protection against concurrent vmap/vunmap calls is - * provided by taking the dma_buf->lock mutex. + * it runs out of vmalloc space. Note that the dma-buf layer keeps a reference + * count for all vmap access and calls down into the exporter's vmap function + * only when no vmapping exists, and only unmaps it once. Protection against + * concurrent vmap/vunmap calls is provided by taking the &dma_buf.lock mutex. * * - For full compatibility on the importer side with existing userspace * interfaces, which might already support mmap'ing buffers. This is needed in @@ -1098,6 +1098,11 @@ static int __dma_buf_begin_cpu_access(struct dma_buf *dmabuf, * dma_buf_end_cpu_access(). Only when cpu access is braketed by both calls is * it guaranteed to be coherent with other DMA access. * + * This function will also wait for any DMA transactions tracked through + * implicit synchronization in &dma_buf.resv. For DMA transactions with explicit + * synchronization this function will only ensure cache coherency, callers must + * ensure synchronization with such DMA transactions on their own. + * * Can return negative error values, returns 0 on success. */ int dma_buf_begin_cpu_access(struct dma_buf *dmabuf, @@ -1199,7 +1204,10 @@ EXPORT_SYMBOL_GPL(dma_buf_mmap); * This call may fail due to lack of virtual mapping address space. * These calls are optional in drivers. The intended use for them * is for mapping objects linear in kernel space for high use objects. - * Please attempt to use kmap/kunmap before thinking about these interfaces. + * + * To ensure coherency users must call dma_buf_begin_cpu_access() and + * dma_buf_end_cpu_access() around any cpu access performed through this + * mapping. * * Returns 0 on success, or a negative errno code otherwise. */ diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index cf72699cb2bc..7eca37c8b10c 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -183,24 +183,19 @@ struct dma_buf_ops { * @begin_cpu_access: * * This is called from dma_buf_begin_cpu_access() and allows the - * exporter to ensure that the memory is actually available for cpu - * access - the exporter might need to allocate or swap-in and pin the - * backing storage. The exporter also needs to ensure that cpu access is - * coherent for the access direction. The direction can be used by the - * exporter to optimize the cache flushing, i.e. access with a different + * exporter to ensure that the memory is actually coherent for cpu + * access. The exporter also needs to ensure that cpu access is coherent + * for the access direction. The direction can be used by the exporter + * to optimize the cache flushing, i.e. access with a different * direction (read instead of write) might return stale or even bogus * data (e.g. when the exporter needs to copy the data to temporary * storage). * - * This callback is optional. + * Note that this is both called through the DMA_BUF_IOCTL_SYNC IOCTL + * command for userspace mappings established through @mmap, and also + * for kernel mappings established with @vmap. * - * FIXME: This is both called through the DMA_BUF_IOCTL_SYNC command - * from userspace (where storage shouldn't be pinned to avoid handing - * de-factor mlock rights to userspace) and for the kernel-internal - * users of the various kmap interfaces, where the backing storage must - * be pinned to guarantee that the atomic kmap calls can succeed. Since - * there's no in-kernel users of the kmap interfaces yet this isn't a - * real problem. + * This callback is optional. * * Returns: * @@ -216,9 +211,7 @@ struct dma_buf_ops { * * This is called from dma_buf_end_cpu_access() when the importer is * done accessing the CPU. The exporter can use this to flush caches and - * unpin any resources pinned in @begin_cpu_access. - * The result of any dma_buf kmap calls after end_cpu_access is - * undefined. + * undo anything else done in @begin_cpu_access. * * This callback is optional. * -- 2.29.2

5 years

[PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs

by Hridya Valsaraju

This patch allows statistics to be enabled for each DMA-BUF in sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. The following stats will be exposed by the interface: /sys/kernel/dmabuf/<inode_number>/exporter_name /sys/kernel/dmabuf/<inode_number>/size /sys/kernel/dmabuf/<inode_number>/dev_map_info The inode_number is unique for each DMA-BUF and was added earlier [1] in order to allow userspace to track DMA-BUF usage across different processes. Currently, this information is exposed in /sys/kernel/debug/dma_buf/bufinfo. However, since debugfs is considered unsafe to be mounted in production, it is being duplicated in sysfs. This information is intended to help with root-causing low-memory kills and the debugging/analysis of other memory-related issues. It will also be used to derive DMA-BUF per-exporter stats and per-device usage stats for Android Bug reports. [1]: https://lore.kernel.org/patchwork/patch/1088791/ Signed-off-by: Hridya Valsaraju <hridya(a)google.com> --- Documentation/ABI/testing/sysfs-kernel-dmabuf | 32 ++++ drivers/dma-buf/Kconfig | 11 ++ drivers/dma-buf/Makefile | 1 + drivers/dma-buf/dma-buf-sysfs-stats.c | 162 ++++++++++++++++++ drivers/dma-buf/dma-buf-sysfs-stats.h | 37 ++++ drivers/dma-buf/dma-buf.c | 29 ++++ include/linux/dma-buf.h | 13 ++ 7 files changed, 285 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf b/Documentation/ABI/testing/sysfs-kernel-dmabuf new file mode 100644 index 000000000000..02d407d57aaa --- /dev/null +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf @@ -0,0 +1,32 @@ +What: /sys/kernel/dmabuf +Date: November 2020 +KernelVersion: v5.11 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: The /sys/kernel/dmabuf directory contains a + snapshot of the internal state of every DMA-BUF. + /sys/kernel/dmabuf/<inode_number> will contain the + statistics for the DMA-BUF with the unique inode number + <inode_number> +Users: kernel memory tuning/debugging tools + +What: /sys/kernel/dmabuf/<inode_number>/exporter_name +Date: November 2020 +KernelVersion: v5.11 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This file is read-only and contains the name of the exporter of + the DMA-BUF. + +What: /sys/kernel/dmabuf/<inode_number>/size +Dat: November 2020 +KernelVersion: v5.11 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This file is read-only and specifies the size of the DMA-BUF in + bytes. + +What: /sys/kernel/dmabuf/<inode_number>/dev_map_info +Dat: November 2020 +KernelVersion: v5.11 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This file is read-only and lists the name of devices currently + mapping the DMA-BUF in a space-separated format. + diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 4f8224a6ac95..2fed26f14548 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -64,6 +64,17 @@ menuconfig DMABUF_HEAPS allows userspace to allocate dma-bufs that can be shared between drivers. +menuconfig DMABUF_SYSFS_STATS + bool "DMA-BUF sysfs statistics" + select DMA_SHARED_BUFFER + help + Choose this option to enable DMA-BUF sysfs statistics + in location /sys/kernel/dmabuf. + + /sys/kernel/dmabuf/<inode_number> will contain + statistics for the DMA-BUF with the unique inode number + <inode_number>. + source "drivers/dma-buf/heaps/Kconfig" endmenu diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 995e05f609ff..40d81f23cacf 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_DMABUF_HEAPS) += heaps/ obj-$(CONFIG_SYNC_FILE) += sync_file.o obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o obj-$(CONFIG_UDMABUF) += udmabuf.o +obj-$(CONFIG_DMABUF_SYSFS_STATS) += dma-buf-sysfs-stats.o dmabuf_selftests-y := \ selftest.o \ diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c new file mode 100644 index 000000000000..bcbef81e0a5f --- /dev/null +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: GPL-2.0-only + + +#include <linux/dma-buf.h> +#include <linux/dma-resv.h> +#include <linux/kobject.h> +#include <linux/printk.h> +#include <linux/slab.h> +#include <linux/sysfs.h> + +#define to_dma_buf_entry_from_kobj(x) container_of(x, struct dma_buf_sysfs_entry, kobj) + +struct dma_buf_stats_attribute { + struct attribute attr; + ssize_t (*show)(struct dma_buf *dmabuf, + struct dma_buf_stats_attribute *attr, char *buf); +}; +#define to_dma_buf_stats_attr(x) container_of(x, struct dma_buf_stats_attribute, attr) + +static ssize_t dma_buf_stats_attribute_show(struct kobject *kobj, + struct attribute *attr, + char *buf) +{ + struct dma_buf_stats_attribute *attribute; + struct dma_buf_sysfs_entry *sysfs_entry; + struct dma_buf *dmabuf; + + attribute = to_dma_buf_stats_attr(attr); + sysfs_entry = to_dma_buf_entry_from_kobj(kobj); + dmabuf = sysfs_entry->dmabuf; + + if (!dmabuf || !attribute->show) + return -EIO; + + return attribute->show(dmabuf, attribute, buf); +} + +static const struct sysfs_ops dma_buf_stats_sysfs_ops = { + .show = dma_buf_stats_attribute_show, +}; + +static ssize_t exporter_name_show(struct dma_buf *dmabuf, + struct dma_buf_stats_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "%s\n", dmabuf->exp_name); +} + +static ssize_t size_show(struct dma_buf *dmabuf, + struct dma_buf_stats_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "%zu\n", dmabuf->size); +} + +static ssize_t dev_map_info_show(struct dma_buf *dmabuf, + struct dma_buf_stats_attribute *attr, + char *buf) +{ + ssize_t ret; + struct dma_buf_attachment *attachment; + + ret = dma_resv_lock_interruptible(dmabuf->resv, NULL); + if (ret) + return ret; + + list_for_each_entry(attachment, &dmabuf->attachments, node) { + if (attachment->map_counter) { + ret += sysfs_emit_at(buf, ret, "%s ", + dev_name(attachment->dev)); + } + } + dma_resv_unlock(dmabuf->resv); + + ret += sysfs_emit_at(buf, ret, "\n"); + return ret; +} + +static struct dma_buf_stats_attribute exporter_name_attribute = + __ATTR_RO(exporter_name); +static struct dma_buf_stats_attribute size_attribute = __ATTR_RO(size); +static struct dma_buf_stats_attribute dev_map_info_attribute = + __ATTR_RO(dev_map_info); + +static struct attribute *dma_buf_stats_default_attrs[] = { + &exporter_name_attribute.attr, + &size_attribute.attr, + &dev_map_info_attribute.attr, + NULL, +}; +ATTRIBUTE_GROUPS(dma_buf_stats_default); + +static void dma_buf_sysfs_release(struct kobject *kobj) +{ + struct dma_buf_sysfs_entry *sysfs_entry; + + sysfs_entry = to_dma_buf_entry_from_kobj(kobj); + kfree(sysfs_entry); +} + +static struct kobj_type dma_buf_ktype = { + .sysfs_ops = &dma_buf_stats_sysfs_ops, + .release = dma_buf_sysfs_release, + .default_groups = dma_buf_stats_default_groups, +}; + +void dma_buf_sysfs_free(struct dma_buf *dmabuf) +{ + struct dma_buf_sysfs_entry *sysfs_entry; + + sysfs_entry = dmabuf->sysfs_entry; + if (!sysfs_entry) + return; + + kobject_del(&sysfs_entry->kobj); + kobject_put(&sysfs_entry->kobj); +} + +static struct kset *dma_buf_stats_kset; +int dma_buf_init_sysfs_statistics(void) +{ + dma_buf_stats_kset = kset_create_and_add("dmabuf", NULL, kernel_kobj); + if (!dma_buf_stats_kset) + return -ENOMEM; + + return 0; +} + +void dma_buf_uninit_sysfs_statistics(void) +{ + kset_unregister(dma_buf_stats_kset); +} + +int dma_buf_init_stats_kobj(struct dma_buf *dmabuf) +{ + struct dma_buf_sysfs_entry *sysfs_entry; + int ret; + + if (!dmabuf || !dmabuf->file) + return -EINVAL; + + if (!dmabuf->exp_name) { + pr_err("exporter name must not be empty if stats needed\n"); + return -EINVAL; + } + + sysfs_entry = kzalloc(sizeof(struct dma_buf_sysfs_entry), GFP_KERNEL); + if (!sysfs_entry) + return -ENOMEM; + + sysfs_entry->kobj.kset = dma_buf_stats_kset; + sysfs_entry->dmabuf = dmabuf; + + dmabuf->sysfs_entry = sysfs_entry; + + ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL, + "%lu", file_inode(dmabuf->file)->i_ino); + if (ret) + kobject_put(&sysfs_entry->kobj); + + return ret; +} diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-buf-sysfs-stats.h new file mode 100644 index 000000000000..42fae7d1b11f --- /dev/null +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h @@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _DMA_BUF_SYSFS_STATS_H +#define _DMA_BUF_SYSFS_STATS_H + +#ifdef CONFIG_DMABUF_SYSFS_STATS + +int dma_buf_init_sysfs_statistics(void); +void dma_buf_uninit_sysfs_statistics(void); + +int dma_buf_init_stats_kobj(struct dma_buf *dmabuf); +static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, + int delta) +{ + attach->map_counter += delta; +} +void dma_buf_sysfs_free(struct dma_buf *dmabuf); + +#else + +static inline int dma_buf_init_sysfs_statistics(void) +{ + return 0; +} + +static inline void dma_buf_uninit_sysfs_statistics(void) {} + +static inline int dma_buf_init_stats_kobj(struct dma_buf *dmabuf) +{ + return 0; +} +static inline void dma_buf_sysfs_free(struct dma_buf *dmabuf) {} +static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, + int delta) {} + +#endif +#endif // _DMA_BUF_SYSFS_STATS_H diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index e63684d4cd90..e93df0069bf8 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -29,6 +29,8 @@ #include <uapi/linux/dma-buf.h> #include <uapi/linux/magic.h> +#include "dma-buf-sysfs-stats.h" + static inline int is_dma_buf_file(struct file *); struct dma_buf_list { @@ -83,6 +85,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + dma_buf_sysfs_free(dmabuf); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); @@ -566,6 +569,10 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) file->f_mode |= FMODE_LSEEK; dmabuf->file = file; + ret = dma_buf_init_stats_kobj(dmabuf); + if (ret) + goto err_sysfs; + mutex_init(&dmabuf->lock); INIT_LIST_HEAD(&dmabuf->attachments); @@ -575,6 +582,14 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) return dmabuf; +err_sysfs: + /* + * Set file->f_path.dentry->d_fsdata to NULL so that when + * dma_buf_release() gets invoked by dentry_ops, it exits + * early before calling the release() dma_buf op. + */ + file->f_path.dentry->d_fsdata = NULL; + fput(file); err_dmabuf: kfree(dmabuf); err_module: @@ -732,6 +747,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, dma_resv_unlock(attach->dmabuf->resv); attach->sgt = sgt; attach->dir = DMA_BIDIRECTIONAL; + dma_buf_update_attachment_map_count(attach, 1 /* delta */); } return attach; @@ -786,6 +802,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) dma_resv_lock(attach->dmabuf->resv, NULL); dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir); + dma_buf_update_attachment_map_count(attach, -1 /* delta */); if (dma_buf_is_dynamic(attach->dmabuf)) { dma_buf_unpin(attach); @@ -925,6 +942,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, } #endif /* CONFIG_DMA_API_DEBUG */ + if (!IS_ERR(sg_table)) + dma_buf_update_attachment_map_count(attach, 1 /* delta */); + return sg_table; } EXPORT_SYMBOL_GPL(dma_buf_map_attachment); @@ -962,6 +982,8 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (dma_buf_is_dynamic(attach->dmabuf) && !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) dma_buf_unpin(attach); + + dma_buf_update_attachment_map_count(attach, -1 /* delta */); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); @@ -1399,6 +1421,12 @@ static inline void dma_buf_uninit_debugfs(void) static int __init dma_buf_init(void) { + int ret; + + ret = dma_buf_init_sysfs_statistics(); + if (ret) + return ret; + dma_buf_mnt = kern_mount(&dma_buf_fs_type); if (IS_ERR(dma_buf_mnt)) return PTR_ERR(dma_buf_mnt); @@ -1414,5 +1442,6 @@ static void __exit dma_buf_deinit(void) { dma_buf_uninit_debugfs(); kern_unmount(dma_buf_mnt); + dma_buf_uninit_sysfs_statistics(); } __exitcall(dma_buf_deinit); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index cf72699cb2bc..f5cab13afdfc 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -294,6 +294,7 @@ struct dma_buf_ops { * @poll: for userspace poll support * @cb_excl: for userspace poll support * @cb_shared: for userspace poll support + * @sysfs_entry: for exposing information about this buffer in sysfs * * This represents a shared buffer, created by calling dma_buf_export(). The * userspace representation is a normal file descriptor, which can be created by @@ -329,6 +330,13 @@ struct dma_buf { __poll_t active; } cb_excl, cb_shared; +#ifdef CONFIG_DMABUF_SYSFS_STATS + /* for sysfs stats */ + struct dma_buf_sysfs_entry { + struct kobject kobj; + struct dma_buf *dmabuf; + } *sysfs_entry; +#endif }; /** @@ -378,6 +386,8 @@ struct dma_buf_attach_ops { * @importer_ops: importer operations for this attachment, if provided * dma_buf_map/unmap_attachment() must be called with the dma_resv lock held. * @importer_priv: importer specific attachment data. + * @map_counter: Number of times the buffer has been mapped through this + * dma_buf_map_attachment. * * This structure holds the attachment information between the dma_buf buffer * and its user device(s). The list contains one attachment struct per device @@ -398,6 +408,9 @@ struct dma_buf_attachment { const struct dma_buf_attach_ops *importer_ops; void *importer_priv; void *priv; +#ifdef CONFIG_DMABUF_SYSFS_STATS + unsigned int map_counter; +#endif }; /** -- 2.29.2.576.ga3fc446d84-goog

5 years

Re: [Linaro-mm-sig] [PATCH v3 3/8] dma-buf: Add vmap_local and vnumap_local operations

by Daniel Vetter

On Wed, Dec 09, 2020 at 03:25:22PM +0100, Thomas Zimmermann wrote: > The existing dma-buf calls dma_buf_vmap() and dma_buf_vunmap() are > allowed to pin the buffer or acquire the buffer's reservation object > lock. > > This is a problem for callers that only require a short-term mapping > of the buffer without the pinning, or callers that have special locking > requirements. These may suffer from unnecessary overhead or interfere > with regular pin operations. > > The new interfaces dma_buf_vmap_local(), dma_buf_vunmapo_local(), and > their rsp callbacks in struct dma_buf_ops provide an alternative without > pinning or reservation locking. Callers are responsible for these > operations. > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > --- > drivers/dma-buf/dma-buf.c | 80 +++++++++++++++++++++++++++++++++++++++ > include/linux/dma-buf.h | 34 +++++++++++++++++ > 2 files changed, 114 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index e63684d4cd90..be9f80190a66 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1265,6 +1265,86 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map) > } > EXPORT_SYMBOL_GPL(dma_buf_vunmap); > > +/** > + * dma_buf_vmap_local - Create virtual mapping for the buffer object into kernel > + * address space. > + * @dmabuf: [in] buffer to vmap > + * @map: [out] returns the vmap pointer > + * > + * This call may fail due to lack of virtual mapping address space. > + * These calls are optional in drivers. The intended use for them > + * is for mapping objects linear in kernel space for high use objects. > + * Please attempt to use kmap/kunmap before thinking about these interfaces. We also need to specify whether callers need to call dma_buf_begin/end_cpu access around these or not. For current implementations it doesn't matter, but if you want to convert udl/gm12u320, it will. I think requiring an explicit call would be good, for more consistency with how normal vmap works. -Daniel > + * > + * Returns: > + * 0 on success, or a negative errno code otherwise. > + */ > +int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map *map) > +{ > + struct dma_buf_map ptr; > + int ret = 0; > + > + dma_buf_map_clear(map); > + > + if (WARN_ON(!dmabuf)) > + return -EINVAL; > + > + dma_resv_assert_held(dmabuf->resv); > + > + if (!dmabuf->ops->vmap_local) > + return -EINVAL; > + > + mutex_lock(&dmabuf->lock); > + if (dmabuf->vmapping_counter) { > + dmabuf->vmapping_counter++; > + BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); > + *map = dmabuf->vmap_ptr; > + goto out_unlock; > + } > + > + BUG_ON(dma_buf_map_is_set(&dmabuf->vmap_ptr)); > + > + ret = dmabuf->ops->vmap_local(dmabuf, &ptr); > + if (WARN_ON_ONCE(ret)) > + goto out_unlock; > + > + dmabuf->vmap_ptr = ptr; > + dmabuf->vmapping_counter = 1; > + > + *map = dmabuf->vmap_ptr; > + > +out_unlock: > + mutex_unlock(&dmabuf->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(dma_buf_vmap_local); > + > +/** > + * dma_buf_vunmap_local - Unmap a vmap obtained by dma_buf_vmap_local. > + * @dmabuf: [in] buffer to vunmap > + * @map: [in] vmap pointer to vunmap > + */ > +void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct dma_buf_map *map) > +{ > + if (WARN_ON(!dmabuf)) > + return; > + > + dma_resv_assert_held(dmabuf->resv); > + > + BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); > + BUG_ON(dmabuf->vmapping_counter == 0); > + BUG_ON(!dma_buf_map_is_equal(&dmabuf->vmap_ptr, map)); > + > + mutex_lock(&dmabuf->lock); > + if (--dmabuf->vmapping_counter == 0) { > + if (dmabuf->ops->vunmap_local) > + dmabuf->ops->vunmap_local(dmabuf, map); > + dma_buf_map_clear(&dmabuf->vmap_ptr); > + } > + mutex_unlock(&dmabuf->lock); > +} > +EXPORT_SYMBOL_GPL(dma_buf_vunmap_local); > + > #ifdef CONFIG_DEBUG_FS > static int dma_buf_debug_show(struct seq_file *s, void *unused) > { > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index cf72699cb2bc..f66580d23a9b 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -269,6 +269,38 @@ struct dma_buf_ops { > > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > + > + /** > + * @vmap_local: > + * > + * Creates a virtual mapping for the buffer into kernel address space. > + * > + * This callback establishes short-term mappings for situations where > + * callers only use the buffer for a bounded amount of time; such as > + * updates to the framebuffer or reading back contained information. > + * In contrast to the regular @vmap callback, vmap_local does never pin > + * the buffer to a specific domain or acquire the buffer's reservation > + * lock. > + * > + * This is called with the dmabuf->resv object locked. Callers must hold > + * the lock until after removing the mapping with @vunmap_local. > + * > + * This callback is optional. > + * > + * Returns: > + * > + * 0 on success or a negative error code on failure. > + */ > + int (*vmap_local)(struct dma_buf *dmabuf, struct dma_buf_map *map); > + > + /** > + * @vunmap_local: > + * > + * Removes a virtual mapping that wa sestablished by @vmap_local. > + * > + * This callback is optional. > + */ > + void (*vunmap_local)(struct dma_buf *dmabuf, struct dma_buf_map *map); > }; > > /** > @@ -506,4 +538,6 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *, > unsigned long); > int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > +int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map *map); > +void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct dma_buf_map *map); > #endif /* __DMA_BUF_H__ */ > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

Re: [Linaro-mm-sig] [PATCH v3 8/8] drm/fb-helper: Move BO locking from DRM client to fbdev damage worker

by Daniel Vetter

On Fri, Dec 11, 2020 at 11:16:25AM +0100, Thomas Zimmermann wrote: > Hi > > Am 11.12.20 um 11:01 schrieb Daniel Vetter: > > On Wed, Dec 09, 2020 at 03:25:27PM +0100, Thomas Zimmermann wrote: > > > Fbdev emulation has to lock the BO into place while flushing the shadow > > > buffer into the BO's memory. Remove any interference with pinning by > > > using vmap_local functionality (instead of full vmap). This requires > > > BO reservation locking in fbdev's damage worker. > > > > > > The new DRM client functions for locking and vmap_local functionality > > > are added for consistency with the existing style. > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > > > > The old vmap/vunmap in the client library aren't used anymore, please > > delete. That will also make it clearer in the diff what's going on and > > that it makes sense to have the client and fb-helper part in one patch. > > They are still around for perma-mapped BOs where HW supports it (really only > CMA-based drivers). See drm_fb_helper_generic_probe() and > drm_fbdev_cleanup(). Ah right I didn't grep this carefully enough. I guess in that case splitting this into the drm_client patch and fb-helper patch would be good I think. Also some kerneldoc commment addition for normal vmap that vmap_local is preferred for short-term mappings, and for vmap_local that _vmap() gives you the long term mapping. Just for more links and stuff in docs. With that: Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> -Daniel > > Best regards > Thomas > > > > > With that: Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > > > > > --- > > > drivers/gpu/drm/drm_client.c | 91 +++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/drm_fb_helper.c | 41 +++++++-------- > > > include/drm/drm_client.h | 4 ++ > > > 3 files changed, 116 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > > > index ce45e380f4a2..795f5cb052ba 100644 > > > --- a/drivers/gpu/drm/drm_client.c > > > +++ b/drivers/gpu/drm/drm_client.c > > > @@ -288,6 +288,37 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u > > > return ERR_PTR(ret); > > > } > > > +/** > > > + * drm_client_buffer_lock - Locks the DRM client buffer > > > + * @buffer: DRM client buffer > > > + * > > > + * This function locks the client buffer by acquiring the buffer > > > + * object's reservation lock. > > > + * > > > + * Unlock the buffer with drm_client_buffer_unlock(). > > > + * > > > + * Returns: > > > + * 0 on success, or a negative errno code otherwise. > > > + */ > > > +int > > > +drm_client_buffer_lock(struct drm_client_buffer *buffer) > > > +{ > > > + return dma_resv_lock(buffer->gem->resv, NULL); > > > +} > > > +EXPORT_SYMBOL(drm_client_buffer_lock); > > > + > > > +/** > > > + * drm_client_buffer_unlock - Unlock DRM client buffer > > > + * @buffer: DRM client buffer > > > + * > > > + * Unlocks a client buffer. See drm_client_buffer_lock(). > > > + */ > > > +void drm_client_buffer_unlock(struct drm_client_buffer *buffer) > > > +{ > > > + dma_resv_unlock(buffer->gem->resv); > > > +} > > > +EXPORT_SYMBOL(drm_client_buffer_unlock); > > > + > > > /** > > > * drm_client_buffer_vmap - Map DRM client buffer into address space > > > * @buffer: DRM client buffer > > > @@ -348,6 +379,66 @@ void drm_client_buffer_vunmap(struct drm_client_buffer *buffer) > > > } > > > EXPORT_SYMBOL(drm_client_buffer_vunmap); > > > +/** > > > + * drm_client_buffer_vmap_local - Map DRM client buffer into address space > > > + * @buffer: DRM client buffer > > > + * @map_copy: Returns the mapped memory's address > > > + * > > > + * This function maps a client buffer into kernel address space. If the > > > + * buffer is already mapped, it returns the existing mapping's address. > > > + * > > > + * Client buffer mappings are not ref'counted. Each call to > > > + * drm_client_buffer_vmap_local() should be followed by a call to > > > + * drm_client_buffer_vunmap_local(); or the client buffer should be mapped > > > + * throughout its lifetime. > > > + * > > > + * The returned address is a copy of the internal value. In contrast to > > > + * other vmap interfaces, you don't need it for the client's vunmap > > > + * function. So you can modify it at will during blit and draw operations. > > > + * > > > + * Returns: > > > + * 0 on success, or a negative errno code otherwise. > > > + */ > > > +int > > > +drm_client_buffer_vmap_local(struct drm_client_buffer *buffer, struct dma_buf_map *map_copy) > > > +{ > > > + struct dma_buf_map *map = &buffer->map; > > > + int ret; > > > + > > > + /* > > > + * FIXME: The dependency on GEM here isn't required, we could > > > + * convert the driver handle to a dma-buf instead and use the > > > + * backend-agnostic dma-buf vmap_local support instead. This would > > > + * require that the handle2fd prime ioctl is reworked to pull the > > > + * fd_install step out of the driver backend hooks, to make that > > > + * final step optional for internal users. > > > + */ > > > + ret = drm_gem_vmap_local(buffer->gem, map); > > > + if (ret) > > > + return ret; > > > + > > > + *map_copy = *map; > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL(drm_client_buffer_vmap_local); > > > + > > > +/** > > > + * drm_client_buffer_vunmap_local - Unmap DRM client buffer > > > + * @buffer: DRM client buffer > > > + * > > > + * This function removes a client buffer's memory mapping. Calling this > > > + * function is only required by clients that manage their buffer mappings > > > + * by themselves. > > > + */ > > > +void drm_client_buffer_vunmap_local(struct drm_client_buffer *buffer) > > > +{ > > > + struct dma_buf_map *map = &buffer->map; > > > + > > > + drm_gem_vunmap_local(buffer->gem, map); > > > +} > > > +EXPORT_SYMBOL(drm_client_buffer_vunmap_local); > > > + > > > static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer) > > > { > > > int ret; > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > > index e82db0f4e771..a56a7d9f7e35 100644 > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > @@ -399,28 +399,34 @@ static int drm_fb_helper_damage_blit(struct drm_fb_helper *fb_helper, > > > int ret; > > > /* > > > - * We have to pin the client buffer to its current location while > > > - * flushing the shadow buffer. In the general case, concurrent > > > - * modesetting operations could try to move the buffer and would > > > - * fail. The modeset has to be serialized by acquiring the reservation > > > - * object of the underlying BO here. > > > - * > > > * For fbdev emulation, we only have to protect against fbdev modeset > > > * operations. Nothing else will involve the client buffer's BO. So it > > > * is sufficient to acquire struct drm_fb_helper.lock here. > > > */ > > > mutex_lock(&fb_helper->lock); > > > - ret = drm_client_buffer_vmap(buffer, &map); > > > + /* > > > + * We have to keep the client buffer at its current location while > > > + * flushing the shadow buffer. Concurrent operations could otherwise > > > + * try to move the buffer. Therefore acquiring the reservation > > > + * object of the underlying BO here. > > > + */ > > > + ret = drm_client_buffer_lock(buffer); > > > + if (ret) > > > + goto out_mutex_unlock; > > > + > > > + ret = drm_client_buffer_vmap_local(buffer, &map); > > > if (ret) > > > - goto out; > > > + goto out_drm_client_buffer_unlock; > > > dst = map; > > > drm_fb_helper_damage_blit_real(fb_helper, clip, &dst); > > > - drm_client_buffer_vunmap(buffer); > > > + drm_client_buffer_vunmap_local(buffer); > > > -out: > > > +out_drm_client_buffer_unlock: > > > + drm_client_buffer_unlock(buffer); > > > +out_mutex_unlock: > > > mutex_unlock(&fb_helper->lock); > > > return ret; > > > @@ -946,15 +952,11 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) > > > drm_modeset_lock_all(fb_helper->dev); > > > drm_client_for_each_modeset(modeset, &fb_helper->client) { > > > crtc = modeset->crtc; > > > - if (!crtc->funcs->gamma_set || !crtc->gamma_size) { > > > - ret = -EINVAL; > > > - goto out; > > > - } > > > + if (!crtc->funcs->gamma_set || !crtc->gamma_size) > > > + return -EINVAL; > > > - if (cmap->start + cmap->len > crtc->gamma_size) { > > > - ret = -EINVAL; > > > - goto out; > > > - } > > > + if (cmap->start + cmap->len > crtc->gamma_size) > > > + return -EINVAL; > > > r = crtc->gamma_store; > > > g = r + crtc->gamma_size; > > > @@ -967,9 +969,8 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) > > > ret = crtc->funcs->gamma_set(crtc, r, g, b, > > > crtc->gamma_size, NULL); > > > if (ret) > > > - goto out; > > > + return ret; > > > } > > > -out: > > > drm_modeset_unlock_all(fb_helper->dev); > > > return ret; > > > diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h > > > index f07f2fb02e75..df61e339a11c 100644 > > > --- a/include/drm/drm_client.h > > > +++ b/include/drm/drm_client.h > > > @@ -156,8 +156,12 @@ struct drm_client_buffer * > > > drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format); > > > void drm_client_framebuffer_delete(struct drm_client_buffer *buffer); > > > int drm_client_framebuffer_flush(struct drm_client_buffer *buffer, struct drm_rect *rect); > > > +int drm_client_buffer_lock(struct drm_client_buffer *buffer); > > > +void drm_client_buffer_unlock(struct drm_client_buffer *buffer); > > > int drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map *map); > > > void drm_client_buffer_vunmap(struct drm_client_buffer *buffer); > > > +int drm_client_buffer_vmap_local(struct drm_client_buffer *buffer, struct dma_buf_map *map); > > > +void drm_client_buffer_vunmap_local(struct drm_client_buffer *buffer); > > > int drm_client_modeset_create(struct drm_client_dev *client); > > > void drm_client_modeset_free(struct drm_client_dev *client); > > > -- > > > 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig