- Linaro-mm-sig - lists.linaro.org

Re: [Linaro-mm-sig] [PATCH v4 2/4] mm: failfast mode with __GFP_NORETRY in alloc_contig_range

by Minchan Kim

On Mon, Jan 25, 2021 at 02:12:00PM +0100, Michal Hocko wrote: > On Thu 21-01-21 09:55:00, Minchan Kim wrote: > > Contiguous memory allocation can be stalled due to waiting > > on page writeback and/or page lock which causes unpredictable > > delay. It's a unavoidable cost for the requestor to get *big* > > contiguous memory but it's expensive for *small* contiguous > > memory(e.g., order-4) because caller could retry the request > > in different range where would have easy migratable pages > > without stalling. > > > > This patch introduce __GFP_NORETRY as compaction gfp_mask in > > alloc_contig_range so it will fail fast without blocking > > when it encounters pages needed waiting. > > I am not against controling how hard this allocator tries with gfp mask > but this changelog is rather void on any data and any user. > > It is also rather dubious to have retries when then caller says to not > retry. Since max_tries is 1 with ++tries, it shouldn't retry. > > Also why didn't you consider GFP_NOWAIT semantic for non blocking mode? GFP_NOWAIT seems to be low(specific) flags rather than the one I want to express. Even though I said only page writeback/lock in the description, the goal is to avoid costly operations we might find later so such "failfast", I thought GFP_NORETRY would be good fit. > > > Signed-off-by: Minchan Kim <minchan(a)kernel.org> > > --- > > mm/page_alloc.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index b031a5ae0bd5..1cdc3ee0b22e 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -8491,12 +8491,16 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, > > unsigned int nr_reclaimed; > > unsigned long pfn = start; > > unsigned int tries = 0; > > + unsigned int max_tries = 5; > > int ret = 0; > > struct migration_target_control mtc = { > > .nid = zone_to_nid(cc->zone), > > .gfp_mask = GFP_USER | __GFP_MOVABLE | __GFP_RETRY_MAYFAIL, > > }; > > > > + if (cc->alloc_contig && cc->mode == MIGRATE_ASYNC) > > + max_tries = 1; > > + > > migrate_prep(); > > > > while (pfn < end || !list_empty(&cc->migratepages)) { > > @@ -8513,7 +8517,7 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, > > break; > > } > > tries = 0; > > - } else if (++tries == 5) { > > + } else if (++tries == max_tries) { > > ret = ret < 0 ? ret : -EBUSY; > > break; > > } > > @@ -8564,7 +8568,7 @@ int alloc_contig_range(unsigned long start, unsigned long end, > > .nr_migratepages = 0, > > .order = -1, > > .zone = page_zone(pfn_to_page(start)), > > - .mode = MIGRATE_SYNC, > > + .mode = gfp_mask & __GFP_NORETRY ? MIGRATE_ASYNC : MIGRATE_SYNC, > > .ignore_skip_hint = true, > > .no_set_skip_hint = true, > > .gfp_mask = current_gfp_context(gfp_mask), > > -- > > 2.30.0.296.g2bfb1c46d8-goog > > -- > Michal Hocko > SUSE Labs

4 years, 4 months

1
0
0 0

[PATCH] drm/todo: Add entry for moving to dma_resv_lock

by Daniel Vetter

Requested by Thomas. I think it justifies a new level, since I tried to make some forward progress on this last summer, and gave up (for now). This is very tricky. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: David Airlie <airlied(a)linux.ie> Cc: Daniel Vetter <daniel(a)ffwll.ch> 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 --- Documentation/gpu/todo.rst | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index dea9082c0e5f..f872d3d33218 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -23,6 +23,9 @@ Advanced: Tricky tasks that need fairly good understanding of the DRM subsystem and graphics topics. Generally need the relevant hardware for development and testing. +Expert: Only attempt these if you've successfully completed some tricky +refactorings already and are an expert in the specific area + Subsystem-wide refactorings =========================== @@ -168,6 +171,22 @@ Contact: Daniel Vetter, respective driver maintainers Level: Advanced +Move Buffer Object Locking to dma_resv_lock() +--------------------------------------------- + +Many drivers have their own per-object locking scheme, usually using +mutex_lock(). This causes all kinds of trouble for buffer sharing, since +depending which driver is the exporter and importer, the locking hierarchy is +reversed. + +To solve this we need one standard per-object locking mechanism, which is +dma_resv_lock(). This lock needs to be called as the outermost lock, with all +other driver specific per-object locks removed. The problem is tha rolling out +the actual change to the locking contract is a flag day, due to struct dma_buf +buffer sharing. + +Level: Expert + Convert logging to drm_* functions with drm_device paramater ------------------------------------------------------------ -- 2.30.0

4 years, 4 months

2
2
0 0

[PATCH 03/11] drm/vblank: Annotate with dma-fence signalling section

by Daniel Vetter

This is rather overkill since currently all drivers call this from hardirq (or at least timers). But maybe in the future we're going to have thread irq handlers and what not, doesn't hurt to be prepared. Plus this is an easy start for sprinkling these fence annotations into shared code. Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/drm_vblank.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 30912d8f82a5..f2aeb9bf325f 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -24,6 +24,7 @@ * OTHER DEALINGS IN THE SOFTWARE. */ +#include <linux/dma-fence.h> #include <linux/export.h> #include <linux/kthread.h> #include <linux/moduleparam.h> @@ -1922,7 +1923,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; unsigned long irqflags; - bool disable_irq; + bool disable_irq, fence_cookie; if (drm_WARN_ON_ONCE(dev, !drm_dev_has_vblank(dev))) return false; @@ -1930,6 +1931,8 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) if (drm_WARN_ON(dev, pipe >= dev->num_crtcs)) return false; + fence_cookie = dma_fence_begin_signalling(); + spin_lock_irqsave(&dev->event_lock, irqflags); /* Need timestamp lock to prevent concurrent execution with @@ -1942,6 +1945,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) if (!vblank->enabled) { spin_unlock(&dev->vblank_time_lock); spin_unlock_irqrestore(&dev->event_lock, irqflags); + dma_fence_end_signalling(fence_cookie); return false; } @@ -1968,6 +1972,8 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) if (disable_irq) vblank_disable_fn(&vblank->disable_timer); + dma_fence_end_signalling(fence_cookie); + return true; } EXPORT_SYMBOL(drm_handle_vblank); -- 2.30.0

4 years, 4 months

1
0
0 0

[PATCH 02/11] drm/vkms: Annotate vblank timer

by Daniel Vetter

This is needed to signal the fences from page flips, annotate it accordingly. We need to annotate entire timer callback since if we get stuck anywhere in there, then the timer stops, and hence fences stop. Just annotating the top part that does the vblank handling isn't enough. Tested-by: Melissa Wen <melissa.srw(a)gmail.com> Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo(a)gmail.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Rodrigo Siqueira <rodrigosiqueiramelo(a)gmail.com> Cc: Haneen Mohammed <hamohammed.sa(a)gmail.com> Cc: Daniel Vetter <daniel(a)ffwll.ch> --- drivers/gpu/drm/vkms/vkms_crtc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index 0443b7deeaef..6164349cdf11 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -1,5 +1,7 @@ // SPDX-License-Identifier: GPL-2.0+ +#include <linux/dma-fence.h> + #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_probe_helper.h> @@ -14,7 +16,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) struct drm_crtc *crtc = &output->crtc; struct vkms_crtc_state *state; u64 ret_overrun; - bool ret; + bool ret, fence_cookie; + + fence_cookie = dma_fence_begin_signalling(); ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer, output->period_ns); @@ -49,6 +53,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) DRM_DEBUG_DRIVER("Composer worker already queued\n"); } + dma_fence_end_signalling(fence_cookie); + return HRTIMER_RESTART; } -- 2.30.0

4 years, 4 months

1
0
0 0

[PATCH 01/11] drm/atomic-helper: Add dma-fence annotations

by Daniel Vetter

This is a bit disappointing since we need to split the annotations over all the different parts. I was considering just leaking the critical section into the ->atomic_commit_tail callback of each driver. But that would mean we need to pass the fence_cookie into each driver (there's a total of 13 implementations of this hook right now), so bad flag day. And also a bit leaky abstraction. Hence just do it function-by-function. Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 4a66768b6057..69121d2925bd 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1567,6 +1567,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_flip_done); void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) { struct drm_device *dev = old_state->dev; + bool fence_cookie = dma_fence_begin_signalling(); drm_atomic_helper_commit_modeset_disables(dev, old_state); @@ -1578,6 +1579,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) drm_atomic_helper_commit_hw_done(old_state); + dma_fence_end_signalling(fence_cookie); + drm_atomic_helper_wait_for_vblanks(dev, old_state); drm_atomic_helper_cleanup_planes(dev, old_state); @@ -1597,6 +1600,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_tail); void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) { struct drm_device *dev = old_state->dev; + bool fence_cookie = dma_fence_begin_signalling(); drm_atomic_helper_commit_modeset_disables(dev, old_state); @@ -1609,6 +1613,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) drm_atomic_helper_commit_hw_done(old_state); + dma_fence_end_signalling(fence_cookie); + drm_atomic_helper_wait_for_vblanks(dev, old_state); drm_atomic_helper_cleanup_planes(dev, old_state); @@ -1624,6 +1630,9 @@ static void commit_tail(struct drm_atomic_state *old_state) ktime_t start; s64 commit_time_ms; unsigned int i, new_self_refresh_mask = 0; + bool fence_cookie; + + fence_cookie = dma_fence_begin_signalling(); funcs = dev->mode_config.helper_private; @@ -1652,6 +1661,8 @@ static void commit_tail(struct drm_atomic_state *old_state) if (new_crtc_state->self_refresh_active) new_self_refresh_mask |= BIT(i); + dma_fence_end_signalling(fence_cookie); + if (funcs && funcs->atomic_commit_tail) funcs->atomic_commit_tail(old_state); else @@ -1810,6 +1821,7 @@ int drm_atomic_helper_commit(struct drm_device *dev, bool nonblock) { int ret; + bool fence_cookie; if (state->async_update) { ret = drm_atomic_helper_prepare_planes(dev, state); @@ -1832,6 +1844,8 @@ int drm_atomic_helper_commit(struct drm_device *dev, if (ret) return ret; + fence_cookie = dma_fence_begin_signalling(); + if (!nonblock) { ret = drm_atomic_helper_wait_for_fences(dev, state, true); if (ret) @@ -1869,6 +1883,7 @@ int drm_atomic_helper_commit(struct drm_device *dev, */ drm_atomic_state_get(state); + dma_fence_end_signalling(fence_cookie); if (nonblock) queue_work(system_unbound_wq, &state->commit_work); else @@ -1877,6 +1892,7 @@ int drm_atomic_helper_commit(struct drm_device *dev, return 0; err: + dma_fence_end_signalling(fence_cookie); drm_atomic_helper_cleanup_planes(dev, state); return ret; } -- 2.30.0

4 years, 4 months

1
0
0 0

[PATCH v3 0/4] Chunk Heap Support on DMA-HEAP

by Minchan Kim

This patchset introduces a new dma heap, "chunk-heap" that makes it easy to perform the bulk allocation of high order pages. It has been created to help optimize the 4K/8K HDR video playback with secure DRM HW to protect contents on memory. The HW needs physically contiguous memory chunks(e.g, 64K) up to several hundred MB memory. To make such high-order big bulk allocations work, chunk-heap uses CMA area. To avoid CMA allocation long stall on blocking pages(e.g., page writeback and/or page locking), it uses failfast mode of the CMA API(i.e., __GFP_NORETRY) so it will continue to find easy migratable pages in different pageblocks without stalling. At last resort, it will allow the blocking only if it couldn't find the available memory in the end. First two patches introduces the failfast mode as __GFP_NORETRY in alloc_contig_range and the allow to use it from the CMA API. Third patch introduces device tree syntax for chunk-heap to bind the specific CMA area with chunk-heap. Finally, last patch implements chunk-heap as dma-buf heap. * since v2 - * introduce gfp_mask with __GFP_NORETRY on cma_alloc - mhocko * do not expoert CMA APIs - Christoph * use compatible string for DT instead of dma-heap specific property - Hridya * Since v1 - https://lore.kernel.org/linux-mm/20201117181935.3613581-1-minchan@kernel.or… * introduce alloc_contig_mode - David * use default CMA instead of device tree - John Hyesoo Yu (2): dt-bindings: reserved-memory: Make DMA-BUF CMA heap DT-configurable dma-buf: heaps: add chunk heap to dmabuf heaps Minchan Kim (2): mm: cma: introduce gfp flag in cma_alloc instead of no_warn mm: failfast mode with __GFP_NORETRY in alloc_contig_range .../reserved-memory/dma_heap_chunk.yaml | 58 +++ drivers/dma-buf/heaps/Kconfig | 8 + drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/chunk_heap.c | 477 ++++++++++++++++++ drivers/dma-buf/heaps/cma_heap.c | 2 +- drivers/s390/char/vmcp.c | 2 +- include/linux/cma.h | 2 +- kernel/dma/contiguous.c | 3 +- mm/cma.c | 12 +- mm/cma_debug.c | 2 +- mm/hugetlb.c | 6 +- mm/page_alloc.c | 8 +- mm/secretmem.c | 3 +- 13 files changed, 568 insertions(+), 16 deletions(-) create mode 100644 Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml create mode 100644 drivers/dma-buf/heaps/chunk_heap.c -- 2.30.0.284.gd98b1dd5eaa7-goog

4 years, 4 months

8
18
0 0

[PATCH v2] 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/buffers/<inode_number>/exporter_name /sys/kernel/dmabuf/buffers/<inode_number>/size /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/device /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/map_counter 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 will be used to derive DMA-BUF per-exporter stats and per-device usage stats for Android Bug reports. The corresponding userspace changes can be found at [2]. Telemetry tools will also capture this information(along with other memory metrics) periodically as well as on important events like a foreground app kill (which might have been triggered by Low Memory Killer). It will also contribute to provide a snapshot of the system memory usage on other events such as OOM kills and Application Not Responding events. A shell script that can be run on a classic Linux environment to read out the DMA-BUF statistics can be found at [3](suggested by John Stultz). The patch contains the following improvements over the previous version: 1) Each attachment is represented by its own directory to allow creating a symlink to the importing device and to also provide room for future expansion. 2) The number of distinct mappings of each attachment is exposed in a separate file. 3) The per-buffer statistics are now in /sys/kernel/dmabuf/buffers inorder to make the interface expandable in future. All of the improvements above are based on suggestions/feedback from Daniel Vetter and Christian König. [1]: https://lore.kernel.org/patchwork/patch/1088791/ [2]: https://android-review.googlesource.com/q/topic:%22dmabuf-sysfs%22+(status:… [3]: https://android-review.googlesource.com/c/platform/system/memory/libmeminfo… Signed-off-by: Hridya Valsaraju <hridya(a)google.com> --- Changes in v2: -Move statistics to /sys/kernel/dmabuf/buffers in oder to allow addition of other DMA-BUF-related sysfs stats in future. Based on feedback from Daniel Vetter. -Each attachment has its own directory to represent attaching devices as symlinks and to introduce map_count as a separate file. Based on feedback from Daniel Vetter and Christian König. Thank you both! -Commit messages updated to point to userspace code in AOSP that will read the DMA-BUF sysfs stats. .../ABI/testing/sysfs-kernel-dmabuf-buffers | 52 ++++ drivers/dma-buf/Kconfig | 11 + drivers/dma-buf/Makefile | 1 + drivers/dma-buf/dma-buf-sysfs-stats.c | 283 ++++++++++++++++++ drivers/dma-buf/dma-buf-sysfs-stats.h | 62 ++++ drivers/dma-buf/dma-buf.c | 37 +++ include/linux/dma-buf.h | 20 ++ 7 files changed, 466 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers 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-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers new file mode 100644 index 000000000000..6f7c65209f07 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers @@ -0,0 +1,52 @@ +What: /sys/kernel/dmabuf/buffers +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: The /sys/kernel/dmabuf/buffers directory contains a + snapshot of the internal state of every DMA-BUF. + /sys/kernel/dmabuf/buffers/<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/buffers/<inode_number>/exporter_name +Date: January 2021 +KernelVersion: v5.12 +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/buffers/<inode_number>/size +Date: January 2021 +KernelVersion: v5.12 +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/buffers/<inode_number>/attachments +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This directory will contain subdirectories representing every + attachment of the DMA-BUF. + +What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid> +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This directory will contain information on the attaching device + and the number of current distinct device mappings. + +What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid>/device +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This file is read-only and is a symlink to the attaching devices's + sysfs entry. + +What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid>/map_counter +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This file is read-only and contains a map_counter indicating the + number of distinct device mappings of the attachment. diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 4f8224a6ac95..27e6a2dafeaa 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/buffers. + + /sys/kernel/dmabuf/buffers/<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..61f85c3d16a5 --- /dev/null +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c @@ -0,0 +1,283 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * DMA-BUF sysfs statistics. + * + * Copyright (C) 2021 Google LLC. + */ + +#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 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 attribute *dma_buf_stats_default_attrs[] = { + &exporter_name_attribute.attr, + &size_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, +}; + +#define to_dma_buf_attach_entry_from_kobj(x) container_of(x, struct dma_buf_attach_sysfs_entry, kobj) + +struct dma_buf_attach_stats_attribute { + struct attribute attr; + ssize_t (*show)(struct dma_buf_attach_sysfs_entry *sysfs_entry, + struct dma_buf_attach_stats_attribute *attr, char *buf); +}; +#define to_dma_buf_attach_stats_attr(x) container_of(x, struct dma_buf_attach_stats_attribute, attr) + +static ssize_t dma_buf_attach_stats_attribute_show(struct kobject *kobj, + struct attribute *attr, + char *buf) +{ + struct dma_buf_attach_stats_attribute *attribute; + struct dma_buf_attach_sysfs_entry *sysfs_entry; + + attribute = to_dma_buf_attach_stats_attr(attr); + sysfs_entry = to_dma_buf_attach_entry_from_kobj(kobj); + + if (!attribute->show) + return -EIO; + + return attribute->show(sysfs_entry, attribute, buf); +} + +static const struct sysfs_ops dma_buf_attach_stats_sysfs_ops = { + .show = dma_buf_attach_stats_attribute_show, +}; + +static ssize_t map_counter_show(struct dma_buf_attach_sysfs_entry *sysfs_entry, + struct dma_buf_attach_stats_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "%u\n", sysfs_entry->map_counter); +} + +static struct dma_buf_attach_stats_attribute map_counter_attribute = + __ATTR_RO(map_counter); + +static struct attribute *dma_buf_attach_stats_default_attrs[] = { + &map_counter_attribute.attr, + NULL, +}; +ATTRIBUTE_GROUPS(dma_buf_attach_stats_default); + +static void dma_buf_attach_sysfs_release(struct kobject *kobj) +{ + struct dma_buf_attach_sysfs_entry *sysfs_entry; + + sysfs_entry = to_dma_buf_attach_entry_from_kobj(kobj); + kfree(sysfs_entry); +} + +static struct kobj_type dma_buf_attach_ktype = { + .sysfs_ops = &dma_buf_attach_stats_sysfs_ops, + .release = dma_buf_attach_sysfs_release, + .default_groups = dma_buf_attach_stats_default_groups, +}; + +void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach) +{ + struct dma_buf_attach_sysfs_entry *sysfs_entry; + + sysfs_entry = attach->sysfs_entry; + if (!sysfs_entry) + return; + + sysfs_delete_link(&sysfs_entry->kobj, &attach->dev->kobj, "device"); + + kobject_del(&sysfs_entry->kobj); + kobject_put(&sysfs_entry->kobj); +} + +int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, + unsigned int uid) +{ + struct dma_buf_attach_sysfs_entry *sysfs_entry; + int ret; + struct dma_buf *dmabuf; + + if (!attach) + return -EINVAL; + + dmabuf = attach->dmabuf; + + sysfs_entry = kzalloc(sizeof(struct dma_buf_attach_sysfs_entry), + GFP_KERNEL); + if (!sysfs_entry) + return -ENOMEM; + + sysfs_entry->kobj.kset = dmabuf->sysfs_entry->attach_stats_kset; + + attach->sysfs_entry = sysfs_entry; + + ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_attach_ktype, + NULL, "%u", uid); + if (ret) + goto kobj_err; + + ret = sysfs_create_link(&sysfs_entry->kobj, &attach->dev->kobj, + "device"); + if (ret) + goto link_err; + + return 0; + +link_err: + kobject_del(&sysfs_entry->kobj); +kobj_err: + kobject_put(&sysfs_entry->kobj); + attach->sysfs_entry = NULL; + + return ret; +} +void dma_buf_stats_teardown(struct dma_buf *dmabuf) +{ + struct dma_buf_sysfs_entry *sysfs_entry; + + sysfs_entry = dmabuf->sysfs_entry; + if (!sysfs_entry) + return; + + kset_unregister(sysfs_entry->attach_stats_kset); + kobject_del(&sysfs_entry->kobj); + kobject_put(&sysfs_entry->kobj); +} + +static struct kset *dma_buf_stats_kset; +static struct kset *dma_buf_per_buffer_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; + + dma_buf_per_buffer_stats_kset = kset_create_and_add("buffers", NULL, + &dma_buf_stats_kset->kobj); + if (!dma_buf_per_buffer_stats_kset) { + kset_unregister(dma_buf_stats_kset); + return -ENOMEM; + } + + return 0; +} + +void dma_buf_uninit_sysfs_statistics(void) +{ + kset_unregister(dma_buf_per_buffer_stats_kset); + kset_unregister(dma_buf_stats_kset); +} + +int dma_buf_stats_setup(struct dma_buf *dmabuf) +{ + struct dma_buf_sysfs_entry *sysfs_entry; + int ret; + struct kset *attach_stats_kset; + + 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_per_buffer_stats_kset; + sysfs_entry->dmabuf = dmabuf; + + dmabuf->sysfs_entry = sysfs_entry; + + /* create the directory for buffer stats */ + ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL, + "%lu", file_inode(dmabuf->file)->i_ino); + if (ret) + goto err_sysfs_dmabuf; + + /* create the directory for attachment stats */ + attach_stats_kset = kset_create_and_add("attachments", NULL, + &sysfs_entry->kobj); + if (!attach_stats_kset) { + ret = -ENOMEM; + goto err_sysfs_attach; + } + + sysfs_entry->attach_stats_kset = attach_stats_kset; + + return 0; + +err_sysfs_attach: + kobject_del(&sysfs_entry->kobj); +err_sysfs_dmabuf: + kobject_put(&sysfs_entry->kobj); + dmabuf->sysfs_entry = NULL; + 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..5f4703249117 --- /dev/null +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h @@ -0,0 +1,62 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * DMA-BUF sysfs statistics. + * + * Copyright (C) 2021 Google LLC. + */ + +#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_stats_setup(struct dma_buf *dmabuf); +int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, + unsigned int uid); +static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, + int delta) +{ + struct dma_buf_attach_sysfs_entry *entry = attach->sysfs_entry; + + entry->map_counter += delta; +} +void dma_buf_stats_teardown(struct dma_buf *dmabuf); +void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach); +static inline unsigned int dma_buf_update_attach_uid(struct dma_buf *dmabuf) +{ + struct dma_buf_sysfs_entry *entry = dmabuf->sysfs_entry; + + return entry->attachment_uid++; +} +#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_stats_setup(struct dma_buf *dmabuf) +{ + return 0; +} +static inline int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, + unsigned int uid) +{ + return 0; +} + +static inline void dma_buf_stats_teardown(struct dma_buf *dmabuf) {} +static inline void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach) {} +static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, + int delta) {} +static inline unsigned int dma_buf_update_attach_uid(struct dma_buf *dmabuf) +{ + return 0; +} +#endif +#endif // _DMA_BUF_SYSFS_STATS_H diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 9ad6397aaa97..29f9ea18eb47 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 { @@ -79,6 +81,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + dma_buf_stats_teardown(dmabuf); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); @@ -579,6 +582,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_stats_setup(dmabuf); + if (ret) + goto err_sysfs; + mutex_init(&dmabuf->lock); INIT_LIST_HEAD(&dmabuf->attachments); @@ -588,6 +595,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: @@ -692,6 +707,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, { struct dma_buf_attachment *attach; int ret; + unsigned int attach_uid; if (WARN_ON(!dmabuf || !dev)) return ERR_PTR(-EINVAL); @@ -717,8 +733,13 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, } dma_resv_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments); + attach_uid = dma_buf_update_attach_uid(dmabuf); dma_resv_unlock(dmabuf->resv); + ret = dma_buf_attach_stats_setup(attach, attach_uid); + if (ret) + goto err_sysfs; + /* When either the importer or the exporter can't handle dynamic * mappings we cache the mapping here to avoid issues with the * reservation object lock. @@ -745,6 +766,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; @@ -761,6 +783,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_unlock(attach->dmabuf->resv); +err_sysfs: dma_buf_detach(dmabuf, attach); return ERR_PTR(ret); } @@ -799,6 +822,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); @@ -812,6 +836,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach); + dma_buf_attach_stats_teardown(attach); kfree(attach); } EXPORT_SYMBOL_GPL(dma_buf_detach); @@ -938,6 +963,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); @@ -975,6 +1003,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); @@ -1412,6 +1442,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); @@ -1427,5 +1463,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..4ae5cc38a4a7 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -294,6 +294,9 @@ 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. + * The attachment_uid member of @sysfs_entry is protected by dma_resv lock + * and is incremented on each attach. * * 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 +332,15 @@ 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; + unsigned int attachment_uid; + struct kset *attach_stats_kset; + } *sysfs_entry; +#endif }; /** @@ -378,6 +390,7 @@ 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. + * @sysfs_entry: For exposing information about this attachment in sysfs. * * 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 +411,13 @@ struct dma_buf_attachment { const struct dma_buf_attach_ops *importer_ops; void *importer_priv; void *priv; +#ifdef CONFIG_DMABUF_SYSFS_STATS + /* for sysfs stats */ + struct dma_buf_attach_sysfs_entry { + struct kobject kobj; + unsigned int map_counter; + } *sysfs_entry; +#endif }; /** -- 2.30.0.284.gd98b1dd5eaa7-goog

4 years, 4 months

3
4
0 0

Re: [Linaro-mm-sig] [PATCH 00/40] [Set 14] Rid W=1 warnings from GPU

by Daniel Vetter

On Fri, Jan 15, 2021 at 06:22:23PM +0000, Zack Rusin wrote: > > > On Jan 15, 2021, at 13:12, Lee Jones <lee.jones(a)linaro.org> wrote: > > > > This set is part of a larger effort attempting to clean-up W=1 > > kernel builds, which are currently overwhelmingly riddled with > > niggly little warnings. > > > > Penultimate set, promise. :) > > > Thank you for all that work. For all the vmwgfx bits: > Reviewed-by: Zack Rusin <zackr(a)vmware.com> I pulled all the non-vmxgfx patches to drm-misc-next, I'll leave the vmw stuff to Zack. Thanks for your work here! -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 4 months

1
0
0 0

[PATCH] drm-buf: Add debug option

by Daniel Vetter

We have too many people abusing the struct page they can get at but really shouldn't in importers. Aside from that the backing page might simply not exist (for dynamic p2p mappings) looking at it and using it e.g. for mmap can also wreak the page handling of the exporter completely. Importers really must go through the proper interface like dma_buf_mmap for everything. I'm semi-tempted to enforce this for dynamic importers since those really have no excuse at all to break the rules. Unfortuantely we can't store the right pointers somewhere safe to make sure we oops on something recognizable, so best is to just wrangle them a bit by flipping all the bits. At least on x86 kernel addresses have all their high bits sets and the struct page array is fairly low in the kernel mapping, so flipping all the bits gives us a very high pointer in userspace and hence excellent chances for an invalid dereference. v2: Add a note to the @map_dma_buf hook that exporters shouldn't do fancy caching tricks, which would blow up with this address scrambling trick here (Chris) Enable by default when CONFIG_DMA_API_DEBUG is enabled. Reviewed-by: Chris Wilson <chris(a)chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: David Stevens <stevensd(a)chromium.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/Kconfig | 8 +++++++ drivers/dma-buf/dma-buf.c | 49 +++++++++++++++++++++++++++++++++++---- include/linux/dma-buf.h | 6 +++++ 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 4f8224a6ac95..4e16c71c24b7 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -50,6 +50,14 @@ config DMABUF_MOVE_NOTIFY This is marked experimental because we don't yet have a consistent execution context and memory management between drivers. +config DMABUF_DEBUG + bool "DMA-BUF debug checks" + default y if DMA_API_DEBUG + help + This option enables additional checks for DMA-BUF importers and + exporters. Specifically it validates that importers do not peek at the + underlying struct page when they import a buffer. + config DMABUF_SELFTESTS tristate "Selftests for the dma-buf interfaces" default n diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 1c9bd51db110..6e4725f7dfde 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -666,6 +666,30 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_GPL(dma_buf_put); +static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, + enum dma_data_direction direction) +{ + struct sg_table *sg_table; + + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); + +#if CONFIG_DMABUF_DEBUG + if (sg_table) { + int i; + struct scatterlist *sg; + + /* To catch abuse of the underlying struct page by importers mix + * up the bits, but take care to preserve the low SG_ bits to + * not corrupt the sgt. The mixing is undone in __unmap_dma_buf + * before passing the sgt back to the exporter. */ + for_each_sgtable_sg(sg_table, sg, i) + sg->page_link ^= ~0xffUL; + } +#endif + + return sg_table; +} + /** * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list * @dmabuf: [in] buffer to attach device to. @@ -737,7 +761,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, goto err_unlock; } - sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL); + sgt = __map_dma_buf(attach, DMA_BIDIRECTIONAL); if (!sgt) sgt = ERR_PTR(-ENOMEM); if (IS_ERR(sgt)) { @@ -784,6 +808,23 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, } EXPORT_SYMBOL_GPL(dma_buf_attach); +static void __unmap_dma_buf(struct dma_buf_attachment *attach, + struct sg_table *sg_table, + enum dma_data_direction direction) +{ + +#if CONFIG_DMABUF_DEBUG + if (sg_table) { + int i; + struct scatterlist *sg; + + for_each_sgtable_sg(sg_table, sg, i) + sg->page_link ^= ~0xffUL; + } +#endif + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); +} + /** * dma_buf_detach - Remove the given attachment from dmabuf's attachments list * @dmabuf: [in] buffer to detach from. @@ -802,7 +843,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_lock(attach->dmabuf->resv, NULL); - dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir); + __unmap_dma_buf(attach, attach->sgt, attach->dir); if (dma_buf_is_dynamic(attach->dmabuf)) { dma_buf_unpin(attach); @@ -924,7 +965,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, } } - sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); + sg_table = __map_dma_buf(attach, direction); if (!sg_table) sg_table = ERR_PTR(-ENOMEM); @@ -987,7 +1028,7 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_assert_held(attach->dmabuf->resv); - attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); + __unmap_dma_buf(attach, sg_table, direction); if (dma_buf_is_dynamic(attach->dmabuf) && !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 628681bf6c99..efdc56b9d95f 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -154,6 +154,12 @@ struct dma_buf_ops { * On failure, returns a negative error value wrapped into a pointer. * May also return -EINTR when a signal was received while being * blocked. + * + * Note that exporters should not try to cache the scatter list, or + * return the same one for multiple calls. Caching is done either by the + * DMA-BUF code (for non-dynamic importers) or the importer. Ownership + * of the scatter list is transferred to the caller, and returned by + * @unmap_dma_buf. */ struct sg_table * (*map_dma_buf)(struct dma_buf_attachment *, enum dma_data_direction); -- 2.29.2

4 years, 4 months

5
10
0 0

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: Tuesday, January 12, 2021 2:45 AM >To: Ruhl, Michael J <michael.j.ruhl(a)intel.com>; 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: linaro-mm-sig(a)lists.linaro.org; Daniel Vetter <daniel.vetter(a)ffwll.ch>; >virtualization(a)lists.linux-foundation.org; dri-devel(a)lists.freedesktop.org; >linux-media(a)vger.kernel.org >Subject: Re: [PATCH v4 01/13] dma-buf: Add vmap_local and vnumap_local >operations > >Hi > >Am 08.01.21 um 17:12 schrieb 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? > >In vmap_local map argument returns the mapping's address. Callers are >expected to check the return code. But I would expect a careless caller >to not check, or check for map being cleared. Clearing it here first is >the save thing to do. Ahh, a pre-emptive attempt to make sure the caller doesn't cause more problems on error. 😊 Thanks, M >Best regards >Thomas > >> >> 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 >> _______________________________________________ >> dri-devel mailing list >> dri-devel(a)lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > >-- >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

4 years, 4 months

1
0
0 0

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

by Daniel Vetter

On Tue, Jan 12, 2021 at 02:11:24PM +0100, Thomas Zimmermann wrote: > 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(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). > > 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). We'd need to disable all that for pass-through vmap of imported objects. > Given all that, would it be possible to remove vmap_lock in favor of taking > the resv lock in vmap/vunmap? All possible (and imo long-term desirable), the trouble is in rolling it out. I've looked at rolling out dma_resv as the one and only lock for shmem helpers before, and gave up. Exynos is the worst (but not the only) offender: - it has it's own per-object lock - that per-object lock is taken most often before calling into various vfuncs, which means for a gradual transition the dma_resv lock would nest within that existing per-object lock (until we've completely replaced it) - but exynos also uses dma_resv already as an outermost lock in its command submission path iow as soon as you add dma_resv_lock anywhere in shmem helpers, we've angered lockdep with a deadlock. That means the only path I think is feasible is adding dma_resv lock to all drivers paths first, _outside_ of any existing driver specific per-object locks. Then remove the driver-specific object locks, and only then can we sprinkle dma_resv_assert_locked all over shmem helpers. Ofc any driver without per-driver locks of their own could directly switch over to dma_resv lock, but until we've converted over all the drivers with their own locking shmem helpers would be stuck where they are right now. I gave up :-/ But maybe if you only try to tackle vmap it might be feasible, since a lot fewer callers. Cheers, Daniel > > 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 4 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v4 11/13] drm/vboxvideo: Use drm_gem_vram_vmap_local() in cursor update

by Daniel Vetter

On Tue, Jan 12, 2021 at 08:54:02AM +0100, Thomas Zimmermann wrote: > Hi > > Am 11.01.21 um 18:06 schrieb Daniel Vetter: > > On Fri, Jan 08, 2021 at 10:43:38AM +0100, Thomas Zimmermann wrote: > > > Cursor updates in vboxvideo require a short-term mapping of the > > > source BO. Use drm_gem_vram_vmap_local() and avoid the pinning > > > operations. > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > > > > All these drivers patches break the dma_resv_lock vs > > dma_fence_begin/end_signalling nesting rules, so this doesn't work. > > > > Generally this is what the prepare/cleanup_fb hooks are for, that's where > > mappings (including vmaps) are meant to be set up, permanently. > > > > I'm kinda not clear on why we need all these changes, I thought the > > locking problem is just in the fb helper paths, because it's outside of > > the atomic path and could conflict with an atomic update at the same time? > > So only that one should get the vmap_local treatment, everything else > > should keep the normal vmap treatment. > > Kind of responding to all your comment on the driver changes: > > These drivers only require short-term mappings, so using vmap_local would be > the natural choice. For SHMEM helpers, it's mostly a cosmetic thing. For > VRAM helpers, I was hoping to remove the vmap/vunmap helpers entirely. One > cannot really map the BOs for the long-term, so not having the helpers at > all would make sense. > > But reading all your comments on the driver patches, I'd rather not update > the drivers here but later convert them to use prepare_fb/cleanup_fb in the > correct way. Ack from me on this plan. I think I got all the other patches with an r-b or ack? -Daniel > > Best regards > Thomas > > > -Daniel > > > --- > > > drivers/gpu/drm/vboxvideo/vbox_mode.c | 15 +++++++++------ > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c > > > index dbc0dd53c69e..215b37c78c10 100644 > > > --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c > > > +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c > > > @@ -381,7 +381,8 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane, > > > container_of(plane->dev, struct vbox_private, ddev); > > > struct vbox_crtc *vbox_crtc = to_vbox_crtc(plane->state->crtc); > > > struct drm_framebuffer *fb = plane->state->fb; > > > - struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(fb->obj[0]); > > > + struct drm_gem_object *obj = fb->obj[0]; > > > + struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(obj); > > > u32 width = plane->state->crtc_w; > > > u32 height = plane->state->crtc_h; > > > size_t data_size, mask_size; > > > @@ -401,11 +402,12 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane, > > > vbox_crtc->cursor_enabled = true; > > > - ret = drm_gem_vram_vmap(gbo, &map); > > > + ret = dma_resv_lock(obj->resv, NULL); > > > + if (ret) > > > + return; > > > + ret = drm_gem_vram_vmap_local(gbo, &map); > > > if (ret) { > > > - /* > > > - * BUG: we should have pinned the BO in prepare_fb(). > > > - */ > > > + dma_resv_unlock(obj->resv); > > > mutex_unlock(&vbox->hw_mutex); > > > DRM_WARN("Could not map cursor bo, skipping update\n"); > > > return; > > > @@ -421,7 +423,8 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane, > > > data_size = width * height * 4 + mask_size; > > > copy_cursor_image(src, vbox->cursor_data, width, height, mask_size); > > > - drm_gem_vram_vunmap(gbo, &map); > > > + drm_gem_vram_vunmap_local(gbo, &map); > > > + dma_resv_unlock(obj->resv); > > > flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE | > > > VBOX_MOUSE_POINTER_ALPHA; > > > -- > > > 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, 4 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v4 11/13] drm/vboxvideo: Use drm_gem_vram_vmap_local() in cursor update

by Daniel Vetter

On Fri, Jan 08, 2021 at 10:43:38AM +0100, Thomas Zimmermann wrote: > Cursor updates in vboxvideo require a short-term mapping of the > source BO. Use drm_gem_vram_vmap_local() and avoid the pinning > operations. > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> All these drivers patches break the dma_resv_lock vs dma_fence_begin/end_signalling nesting rules, so this doesn't work. Generally this is what the prepare/cleanup_fb hooks are for, that's where mappings (including vmaps) are meant to be set up, permanently. I'm kinda not clear on why we need all these changes, I thought the locking problem is just in the fb helper paths, because it's outside of the atomic path and could conflict with an atomic update at the same time? So only that one should get the vmap_local treatment, everything else should keep the normal vmap treatment. -Daniel > --- > drivers/gpu/drm/vboxvideo/vbox_mode.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c > index dbc0dd53c69e..215b37c78c10 100644 > --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c > +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c > @@ -381,7 +381,8 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane, > container_of(plane->dev, struct vbox_private, ddev); > struct vbox_crtc *vbox_crtc = to_vbox_crtc(plane->state->crtc); > struct drm_framebuffer *fb = plane->state->fb; > - struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(fb->obj[0]); > + struct drm_gem_object *obj = fb->obj[0]; > + struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(obj); > u32 width = plane->state->crtc_w; > u32 height = plane->state->crtc_h; > size_t data_size, mask_size; > @@ -401,11 +402,12 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane, > > vbox_crtc->cursor_enabled = true; > > - ret = drm_gem_vram_vmap(gbo, &map); > + ret = dma_resv_lock(obj->resv, NULL); > + if (ret) > + return; > + ret = drm_gem_vram_vmap_local(gbo, &map); > if (ret) { > - /* > - * BUG: we should have pinned the BO in prepare_fb(). > - */ > + dma_resv_unlock(obj->resv); > mutex_unlock(&vbox->hw_mutex); > DRM_WARN("Could not map cursor bo, skipping update\n"); > return; > @@ -421,7 +423,8 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane, > data_size = width * height * 4 + mask_size; > > copy_cursor_image(src, vbox->cursor_data, width, height, mask_size); > - drm_gem_vram_vunmap(gbo, &map); > + drm_gem_vram_vunmap_local(gbo, &map); > + dma_resv_unlock(obj->resv); > > flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE | > VBOX_MOUSE_POINTER_ALPHA; > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 4 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v4 06/13] drm/cirrus: Use drm_gem_shmem_vmap_local() in damage handling

by Daniel Vetter

On Fri, Jan 08, 2021 at 10:43:33AM +0100, Thomas Zimmermann wrote: > Damage handling in cirrus requires a short-term mapping of the source > BO. Use drm_gem_shmem_vmap_local(). > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Hm more possible errors that we don't report to userspace ... Why don't we vmap/vunmap these in prepare/cleanup_fb? Generally we'd want a long-term vmap here to make sure this all works nicely. Since it's nothing new, on this patch: Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > --- > drivers/gpu/drm/tiny/cirrus.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c > index a043e602199e..21cd7056d45f 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: > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 4 months

1
1
0 0

Re: [Linaro-mm-sig] [PATCH v4 07/13] drm/gm12u320: Use drm_gem_shmem_vmap_local() in damage handling

by Daniel Vetter

On Fri, Jan 08, 2021 at 10:43:34AM +0100, Thomas Zimmermann wrote: > Damage handling in gm12u320 requires a short-term mapping of the source > BO. Use drm_gem_shmem_vmap_local(). > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> > --- > drivers/gpu/drm/tiny/gm12u320.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > 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); Unlock before vunmap. -Daniel > 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; > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 4 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v4 05/13] drm/mgag200: Use drm_gem_shmem_vmap_local() in damage handling

by Daniel Vetter

On Fri, Jan 08, 2021 at 10:43:32AM +0100, Thomas Zimmermann wrote: > Damage handling in mgag200 requires a short-term mapping of the source > BO. Use drm_gem_shmem_vmap_local(). > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > --- > drivers/gpu/drm/mgag200/mgag200_mode.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > 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 > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 4 months

1
1
0 0

Re: [Linaro-mm-sig] [PATCH v4 13/13] drm/vram-helper: Remove unused drm_gem_vram_{vmap, vunmap}()

by Daniel Vetter

On Fri, Jan 08, 2021 at 10:43:40AM +0100, Thomas Zimmermann wrote: > VRAM-helper BO's cannot be exported, so calls for vmap and vunmap > can only come from the BO's drivers or a kernel client. These are > supposed use vmap_local functionality. ^to > > The vmap and vunmap operations in VRAM helpers are therefore unused > and can be removed. > > Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > --- > drivers/gpu/drm/drm_gem_vram_helper.c | 98 --------------------------- > include/drm/drm_gem_vram_helper.h | 2 - > 2 files changed, 100 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > index c7fba3a0758e..c7d166cd16df 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -379,72 +379,6 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo) > } > EXPORT_SYMBOL(drm_gem_vram_unpin); > > -/** > - * drm_gem_vram_vmap() - Pins and maps a GEM VRAM object into kernel address > - * space > - * @gbo: The GEM VRAM object to map > - * @map: Returns the kernel virtual address of the VRAM GEM object's backing > - * store. > - * > - * The vmap function pins a GEM VRAM object to its current location, either > - * system or video memory, and maps its buffer into kernel address space. > - * As pinned object cannot be relocated, you should avoid pinning objects > - * permanently. Call drm_gem_vram_vunmap() with the returned address to > - * unmap and unpin the GEM VRAM object. > - * > - * Returns: > - * 0 on success, or a negative error code otherwise. > - */ > -int drm_gem_vram_vmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map) > -{ > - int ret; > - > - ret = ttm_bo_reserve(&gbo->bo, true, false, NULL); > - if (ret) > - return ret; > - > - ret = drm_gem_vram_pin_locked(gbo, 0); > - if (ret) > - goto err_ttm_bo_unreserve; > - ret = drm_gem_vram_vmap_local(gbo, map); > - if (ret) > - goto err_drm_gem_vram_unpin_locked; > - > - ttm_bo_unreserve(&gbo->bo); > - > - return 0; > - > -err_drm_gem_vram_unpin_locked: > - drm_gem_vram_unpin_locked(gbo); > -err_ttm_bo_unreserve: > - ttm_bo_unreserve(&gbo->bo); > - return ret; > -} > -EXPORT_SYMBOL(drm_gem_vram_vmap); > - > -/** > - * drm_gem_vram_vunmap() - Unmaps and unpins a GEM VRAM object > - * @gbo: The GEM VRAM object to unmap > - * @map: Kernel virtual address where the VRAM GEM object was mapped > - * > - * A call to drm_gem_vram_vunmap() unmaps and unpins a GEM VRAM buffer. See > - * the documentation for drm_gem_vram_vmap() for more information. > - */ > -void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map) > -{ > - int ret; > - > - ret = ttm_bo_reserve(&gbo->bo, false, false, NULL); > - if (WARN_ONCE(ret, "ttm_bo_reserve_failed(): ret=%d\n", ret)) > - return; > - > - drm_gem_vram_vunmap_local(gbo, map); > - drm_gem_vram_unpin_locked(gbo); > - > - ttm_bo_unreserve(&gbo->bo); > -} > -EXPORT_SYMBOL(drm_gem_vram_vunmap); > - > /** > * drm_gem_vram_vmap_local() - Maps a GEM VRAM object into kernel address space > * @gbo: The GEM VRAM object to map > @@ -870,36 +804,6 @@ static void drm_gem_vram_object_unpin(struct drm_gem_object *gem) > drm_gem_vram_unpin(gbo); > } > > -/** > - * drm_gem_vram_object_vmap() - > - * Implements &struct drm_gem_object_funcs.vmap > - * @gem: The GEM object to map > - * @map: Returns the kernel virtual address of the VRAM GEM object's backing > - * store. > - * > - * Returns: > - * 0 on success, or a negative error code otherwise. > - */ > -static int drm_gem_vram_object_vmap(struct drm_gem_object *gem, struct dma_buf_map *map) > -{ > - struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem); > - > - return drm_gem_vram_vmap(gbo, map); > -} > - > -/** > - * drm_gem_vram_object_vunmap() - > - * Implements &struct drm_gem_object_funcs.vunmap > - * @gem: The GEM object to unmap > - * @map: Kernel virtual address where the VRAM GEM object was mapped > - */ > -static void drm_gem_vram_object_vunmap(struct drm_gem_object *gem, struct dma_buf_map *map) > -{ > - struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem); > - > - drm_gem_vram_vunmap(gbo, map); > -} > - > static int drm_gem_vram_object_vmap_local(struct drm_gem_object *gem, struct dma_buf_map *map) > { > struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem); > @@ -922,8 +826,6 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = { > .free = drm_gem_vram_object_free, > .pin = drm_gem_vram_object_pin, > .unpin = drm_gem_vram_object_unpin, > - .vmap = drm_gem_vram_object_vmap, > - .vunmap = drm_gem_vram_object_vunmap, > .vmap_local = drm_gem_vram_object_vmap_local, > .vunmap_local = drm_gem_vram_object_vunmap_local, > .mmap = drm_gem_ttm_mmap, > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h > index bd6a60e7c22b..e571867f4069 100644 > --- a/include/drm/drm_gem_vram_helper.h > +++ b/include/drm/drm_gem_vram_helper.h > @@ -97,8 +97,6 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo); > s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo); > int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag); > int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo); > -int drm_gem_vram_vmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map); > -void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, struct dma_buf_map *map); > int drm_gem_vram_vmap_local(struct drm_gem_vram_object *gbo, struct dma_buf_map *map); > void drm_gem_vram_vunmap_local(struct drm_gem_vram_object *gbo, struct dma_buf_map *map); > > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 4 months

1
0
0 0

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, 4 months

1
0
0 0

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, 5 months

1
0
0 0

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, 5 months

1
0
0 0

[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, 5 months

1
0
0 0

[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, 5 months

3
2
0 0

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, 5 months

1
0
0 0

[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

4 years, 5 months

3
10
0 0

[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

4 years, 5 months

5
14
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig