Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

6 participants
2911 discussions

by Andrew Morton

On Tue, 6 Feb 2024 09:45:18 +0530 Anshuman Khandual <anshuman.khandual(a)arm.com> wrote: > cma_get_name() just returns cma->name without any additional transformation > unlike other helpers such as cma_get_base() and cma_get_size(). This helper > is not worth the additional indirection, and can be dropped after replacing > directly with cma->name in the sole caller __add_cma_heap(). drivers/dma-buf/heaps/cma_heap.c: In function '__add_cma_heap': drivers/dma-buf/heaps/cma_heap.c:379:28: error: invalid use of undefined type 'struct cma' 379 | exp_info.name = cma->name; | ^~ Fixing this would require moving the `struct cma' definition into cma.h. I don't think that's worthwhile.

1 year, 4 months

[PATCH v3 00/11] Add mediate-drm secure flow for SVP

by Jason-JH.Lin

From: Jason-jh Lin <jason-jh.lin(a)mediatek.corp-partner.google.com> Memory Definitions: secure memory - Memory allocated in the TEE (Trusted Execution Environment) which is inaccessible in the REE (Rich Execution Environment, i.e. linux kernel/userspace). secure handle - Integer value which acts as reference to 'secure memory'. Used in communication between TEE and REE to reference 'secure memory'. secure buffer - 'secure memory' that is used to store decrypted, compressed video or for other general purposes in the TEE. secure surface - 'secure memory' that is used to store graphic buffers. Memory Usage in SVP: The overall flow of SVP starts with encrypted video coming in from an outside source into the REE. The REE will then allocate a 'secure buffer' and send the corresponding 'secure handle' along with the encrypted, compressed video data to the TEE. The TEE will then decrypt the video and store the result in the 'secure buffer'. The REE will then allocate a 'secure surface'. The REE will pass the 'secure handles' for both the 'secure buffer' and 'secure surface' into the TEE for video decoding. The video decoder HW will then decode the contents of the 'secure buffer' and place the result in the 'secure surface'. The REE will then attach the 'secure surface' to the overlay plane for rendering of the video. Everything relating to ensuring security of the actual contents of the 'secure buffer' and 'secure surface' is out of scope for the REE and is the responsibility of the TEE. DRM driver handles allocation of gem objects that are backed by a 'secure surface' and for displaying a 'secure surface' on the overlay plane. This introduces a new flag for object creation called DRM_MTK_GEM_CREATE_ENCRYPTED which indicates it should be a 'secure surface'. All changes here are in MediaTek specific code. --- TODO: 1) Remove get sec larb port interface in ddp_comp, ovl and ovl_adaptor. 2) Verify instruction for enabling/disabling dapc and larb port in TEE drop the sec_engine flags in normal world and. 3) Move DISP_REG_OVL_SECURE setting to secure world for mtk_disp_ovl.c. 4) Change the parameter register address in mtk_ddp_sec_write() from "u32 addr" to "struct cmdq_client_reg *cmdq_reg". 5) Implement setting mmsys routing table in the secure world series. --- Based on 5 series and 1 patch: [1] v3 dma-buf: heaps: Add MediaTek secure heap - https://patchwork.kernel.org/project/linux-mediatek/list/?series=809023 [2] v3 add driver to support secure video decoder - https://patchwork.kernel.org/project/linux-mediatek/list/?series=807308 [3] v4 soc: mediatek: Add register definitions for GCE - https://patchwork.kernel.org/project/linux-mediatek/patch/20231212121957.19… [4] v2 Add CMDQ driver support for mt8188 - https://patchwork.kernel.org/project/linux-mediatek/list/?series=810302 [5] Add mediatek,gce-events definition to mediatek,gce-mailbox bindings - https://patchwork.kernel.org/project/linux-mediatek/list/?series=810938 [6] v3 Add CMDQ secure driver for SVP - https://patchwork.kernel.org/project/linux-mediatek/list/?series=812379 --- Change in v3: 1. fix kerneldoc problems 2. fix typo in title and commit message 3. adjust naming for secure variable 4. add the missing part for is_suecure plane implementation 5. use BIT_ULL macro to replace bit shifting 6. move modification of ovl_adaptor part to the correct patch 7. add TODO list in commit message 8. add commit message for using share memory to store execute count Change in v2: 1. remove the DRIVER_RDNDER flag for mtk_drm_ioctl 2. move cmdq_insert_backup_cookie into client driver 3. move secure gce node define from mt8195-cherry.dtsi to mt8195.dtsi --- CK Hu (1): drm/mediatek: Add interface to allocate MediaTek GEM buffer. Jason-JH.Lin (10): drm/mediatek/uapi: Add DRM_MTK_GEM_CREATE_ENCRYPTED flag drm/mediatek: Add secure buffer control flow to mtk_drm_gem drm/mediatek: Add secure identify flag and funcution to mtk_drm_plane drm/mediatek: Add mtk_ddp_sec_write to config secure buffer info drm/mediatek: Add get_sec_port interface to mtk_ddp_comp drm/mediatek: Add secure layer config support for ovl drm/mediatek: Add secure layer config support for ovl_adaptor drm/mediatek: Add secure flow support to mediatek-drm drm/mediatek: Add cmdq_insert_backup_cookie before secure pkt finalize arm64: dts: mt8195: Add secure mbox settings for vdosys arch/arm64/boot/dts/mediatek/mt8195.dtsi | 6 +- drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 + drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 31 +- .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 15 + drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 274 +++++++++++++++++- drivers/gpu/drm/mediatek/mtk_drm_crtc.h | 1 + drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 30 ++ drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 14 + drivers/gpu/drm/mediatek/mtk_drm_drv.c | 13 + drivers/gpu/drm/mediatek/mtk_drm_gem.c | 122 ++++++++ drivers/gpu/drm/mediatek/mtk_drm_gem.h | 16 + drivers/gpu/drm/mediatek/mtk_drm_plane.c | 26 ++ drivers/gpu/drm/mediatek/mtk_drm_plane.h | 2 + drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 11 +- drivers/gpu/drm/mediatek/mtk_mdp_rdma.h | 2 + include/uapi/drm/mediatek_drm.h | 59 ++++ 16 files changed, 607 insertions(+), 18 deletions(-) create mode 100644 include/uapi/drm/mediatek_drm.h -- 2.18.0

1 year, 4 months

[PATCH] dma-buf: heaps: Don't track CMA dma-buf pages under RssFile

by T.J. Mercier

DMA buffers allocated from the CMA dma-buf heap get counted under RssFile for processes that map them and trigger page faults. In addition to the incorrect accounting reported to userspace, reclaim behavior was influenced by the MM_FILEPAGES counter until linux 6.8, but this memory is not reclaimable. [1] Change the CMA dma-buf heap to set VM_PFNMAP on the VMA so MM does not poke at the memory managed by this dma-buf heap, and use vmf_insert_pfn to correct the RSS accounting. The system dma-buf heap does not suffer from this issue since remap_pfn_range is used during the mmap of the buffer, which also sets VM_PFNMAP on the VMA. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/m… Fixes: b61614ec318a ("dma-buf: heaps: Add CMA heap to dmabuf heaps") Signed-off-by: T.J. Mercier <tjmercier(a)google.com> --- drivers/dma-buf/heaps/cma_heap.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c index ee899f8e6721..4a63567e93ba 100644 --- a/drivers/dma-buf/heaps/cma_heap.c +++ b/drivers/dma-buf/heaps/cma_heap.c @@ -168,10 +168,7 @@ static vm_fault_t cma_heap_vm_fault(struct vm_fault *vmf) if (vmf->pgoff > buffer->pagecount) return VM_FAULT_SIGBUS; - vmf->page = buffer->pages[vmf->pgoff]; - get_page(vmf->page); - - return 0; + return vmf_insert_pfn(vma, vmf->address, page_to_pfn(buffer->pages[vmf->pgoff])); } static const struct vm_operations_struct dma_heap_vm_ops = { @@ -185,6 +182,8 @@ static int cma_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma) if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0) return -EINVAL; + vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP); + vma->vm_ops = &dma_heap_vm_ops; vma->vm_private_data = buffer; -- 2.43.0.381.gb435a96ce8-goog

1 year, 5 months

[PATCH v6 0/6] iio: new DMABUF based API, v6

by Paul Cercueil

Hi Jonathan, This is the v6 of my patchset that introduces a new interface based on DMABUF objects. The code was updated quite a bit, using the feedback on the list for this patchset but also the feedback I received on the FunctionFS patchset that I'm working on upstreaming in parallel [1] where the DMABUF handling code is very similar. See below for the full changelog. I decided to drop the scope-based memory management for dma_buf and I hope you are OK with that. Christian wants the patch(es) to support scope-based memory management in dma-buf as a separate patchset; once it's in, I will gladly send a follow-up patch to use __free() where it makes sense. For performance numbers, I'll point you to the cover letter for my v5 patchset [2]. This patchset was based on next-20240129. Cheers, -Paul [1] https://lore.kernel.org/all/20230322092118.9213-1-paul@crapouillou.net/ [2] https://lore.kernel.org/linux-iio/219abc43b4fdd4a13b307ed2efaa0e6869e68e3f.… --- Changelog: * [2/6]: - Use new prototype for axi_dmac_alloc_desc() as it changed upstream * [3/6]: - Remove dead code in iio_dma_resv_lock() - Fix non-block actually blocking - Cache dma_buf_attachment instead of mapping/unmapping it for every transfer - Return -EINVAL instead of IIO_IOCTL_UNHANDLED for unknown ioctl - Make .block_enqueue() callback take a dma_fence pointer, which will be passed to iio_buffer_signal_dmabuf_done() instead of the dma_buf_attachment; and remove the backpointer from the priv structure to the dma_fence. - Use dma_fence_begin/end_signalling in the dma_fence critical sections - Unref dma_fence and dma_buf_attachment in worker, because they might try to lock the dma_resv, which would deadlock. - Add buffer ops to lock/unlock the queue. This is motivated by the fact that once the dma_fence has been installed, we cannot lock anything anymore - so the queue must be locked before the dma_fence is installed. - Use 'long retl' variable to handle the return value of dma_resv_wait_timeout() - Protect dmabufs list access with a mutex - Rework iio_buffer_find_attachment() to use the internal dmabufs list, instead of messing with dmabufs private data. - Add an atomically-increasing sequence number for fences * [4/6]: - Update iio_dma_buffer_enqueue_dmabuf() to take a dma_fence pointer - Pass that dma_fence pointer along to iio_buffer_signal_dmabuf_done() - Add iio_dma_buffer_lock_queue() / iio_dma_buffer_unlock_queue() - Do not lock the queue in iio_dma_buffer_enqueue_dmabuf(). The caller will ensure that it has been locked already. - Replace "int += bool;" by "if (bool) int++;" - Use dma_fence_begin/end_signalling in the dma_fence critical sections - Use one "num_dmabufs" fields instead of one "num_blocks" and one "num_fileio_blocks". Make it an atomic_t, which makes it possible to decrement it atomically in iio_buffer_block_release() without having to lock the queue mutex; and in turn, it means that we don't need to use iio_buffer_block_put_atomic() everywhere to avoid locking the queue mutex twice. - Use cleanup.h guard(mutex) when possible - Explicitely list all states in the switch in iio_dma_can_enqueue_block() - Rename iio_dma_buffer_fileio_mode() to iio_dma_buffer_can_use_fileio(), and add a comment explaining why it cannot race vs. DMABUF. * [5/6]: - Populate .lock_queue / .unlock_queue callbacks - Switch to atomic memory allocations in .submit_queue, because of the dma_fence critical section - Make sure that the size of the scatterlist is enough --- Paul Cercueil (6): dmaengine: Add API function dmaengine_prep_slave_dma_vec() dmaengine: dma-axi-dmac: Implement device_prep_slave_dma_vec iio: core: Add new DMABUF interface infrastructure iio: buffer-dma: Enable support for DMABUFs iio: buffer-dmaengine: Support new DMABUF based userspace API Documentation: iio: Document high-speed DMABUF based API Documentation/iio/dmabuf_api.rst | 54 ++ Documentation/iio/index.rst | 2 + drivers/dma/dma-axi-dmac.c | 40 ++ drivers/iio/buffer/industrialio-buffer-dma.c | 181 ++++++- .../buffer/industrialio-buffer-dmaengine.c | 58 ++- drivers/iio/industrialio-buffer.c | 462 ++++++++++++++++++ include/linux/dmaengine.h | 25 + include/linux/iio/buffer-dma.h | 31 ++ include/linux/iio/buffer_impl.h | 33 ++ include/uapi/linux/iio/buffer.h | 22 + 10 files changed, 891 insertions(+), 17 deletions(-) create mode 100644 Documentation/iio/dmabuf_api.rst -- 2.43.0

1 year, 5 months

[PATCH v6 0/4] usb: gadget: functionfs: DMABUF import interface

by Paul Cercueil

Hi, This is the v6 of my patchset that adds a new DMABUF import interface to FunctionFS. Given that the cache coherency issue that has been discussed after my v5 is a tangential problem and not directly related to this new interface, I decided to drop the dma_buf_begin/end_access() functions for now - but I'm open to the idea of re-introducing them in a subsequent patchset. The patchset was rebased on next-20240129. Cheers, -Paul --- Changelog: * Drop v5's patches [1/6] and [2/6]. * [3/4]: - Drop use of dma_buf_begin/end_access(). We now make the assumption that the devices attached to the DMABUFs must be coherent between themselves. The cache coherency issue is a tangential problem, and those functions can be re-introduced in a subsequent patchset. - Unqueue pending requests on detach. Otherwise, when closing the data endpoint the DMABUF will never be signaled. - Use list_for_each_entry_safe() in ffs_dmabuf_detach(), because there is a list_del() in there. - use pr_vdebug() instead of pr_debug() - Rename ffs_dmabuf_unmap_work() -> ffs_dmabuf_cleanup() --- Paul Cercueil (4): usb: gadget: Support already-mapped DMA SGs usb: gadget: functionfs: Factorize wait-for-endpoint code usb: gadget: functionfs: Add DMABUF import interface Documentation: usb: Document FunctionFS DMABUF API Documentation/usb/functionfs.rst | 36 ++ drivers/usb/gadget/Kconfig | 1 + drivers/usb/gadget/function/f_fs.c | 513 ++++++++++++++++++++++++++-- drivers/usb/gadget/udc/core.c | 7 +- include/linux/usb/gadget.h | 2 + include/uapi/linux/usb/functionfs.h | 41 +++ 6 files changed, 579 insertions(+), 21 deletions(-) -- 2.43.0

1 year, 5 months

[PATCH v5 0/8] iio: new DMABUF based API, v5

by Paul Cercueil

[V4 was: "iio: Add buffer write() support"][1] Hi Jonathan, This is a respin of the V3 of my patchset that introduced a new interface based on DMABUF objects [2]. The V4 was a split of the patchset, to attempt to upstream buffer write() support first. But since there is no current user upstream, it was not merged. This V5 is about doing the opposite, and contains the new DMABUF interface, without adding the buffer write() support. It can already be used with the upstream adi-axi-adc driver. In user-space, Libiio uses it to transfer back and forth blocks of samples between the hardware and the applications, without having to copy the data. On a ZCU102 with a FMComms3 daughter board, running Libiio from the pcercuei/dev-new-dmabuf-api branch [3], compiled with WITH_LOCAL_DMABUF_API=OFF (so that it uses fileio): sudo utils/iio_rwdev -b 4096 -B cf-ad9361-lpc Throughput: 116 MiB/s Same hardware, with the DMABUF API (WITH_LOCAL_DMABUF_API=ON): sudo utils/iio_rwdev -b 4096 -B cf-ad9361-lpc Throughput: 475 MiB/s This benchmark only measures the speed at which the data can be fetched to iio_rwdev's internal buffers, and does not actually try to read the data (e.g. to pipe it to stdout). It shows that fetching the data is more than 4x faster using the new interface. When actually reading the data, the performance difference isn't that impressive (maybe because in case of DMABUF the data is not in cache): WITH_LOCAL_DMABUF_API=OFF (so that it uses fileio): sudo utils/iio_rwdev -b 4096 cf-ad9361-lpc | dd of=/dev/zero status=progress 2446422528 bytes (2.4 GB, 2.3 GiB) copied, 22 s, 111 MB/s WITH_LOCAL_DMABUF_API=ON: sudo utils/iio_rwdev -b 4096 cf-ad9361-lpc | dd of=/dev/zero status=progress 2334388736 bytes (2.3 GB, 2.2 GiB) copied, 21 s, 114 MB/s One interesting thing to note is that fileio is (currently) actually faster than the DMABUF interface if you increase a lot the buffer size. My explanation is that the cache invalidation routine takes more and more time the bigger the DMABUF gets. This is because the DMABUF is backed by small-size pages, so a (e.g.) 64 MiB DMABUF is backed by up to 16 thousands pages, that have to be invalidated one by one. This can be addressed by using huge pages, but the udmabuf driver does not (yet) support creating DMABUFs backed by huge pages. Anyway, the real benefits happen when the DMABUFs are either shared between IIO devices, or between the IIO subsystem and another filesystem. In that case, the DMABUFs are simply passed around drivers, without the data being copied at any moment. We use that feature to transfer samples from our transceivers to USB, using a DMABUF interface to FunctionFS [4]. This drastically increases the throughput, to about 274 MiB/s over a USB3 link, vs. 127 MiB/s using IIO's fileio interface + write() to the FunctionFS endpoints, for a lower CPU usage (0.85 vs. 0.65 load avg.). Based on linux-next/next-20231219. Cheers, -Paul [1] https://lore.kernel.org/all/20230807112113.47157-1-paul@crapouillou.net/ [2] https://lore.kernel.org/all/20230403154800.215924-1-paul@crapouillou.net/ [3] https://github.com/analogdevicesinc/libiio/tree/pcercuei/dev-new-dmabuf-api [4] https://lore.kernel.org/all/20230322092118.9213-1-paul@crapouillou.net/ --- Changelog: - [3/8]: Replace V3's dmaengine_prep_slave_dma_array() with a new dmaengine_prep_slave_dma_vec(), which uses a new 'dma_vec' struct. Note that at some point we will need to support cyclic transfers using dmaengine_prep_slave_dma_vec(). Maybe with a new "flags" parameter to the function? - [4/8]: Implement .device_prep_slave_dma_vec() instead of V3's .device_prep_slave_dma_array(). @Vinod: this patch will cause a small conflict with my other patchset adding scatter-gather support to the axi-dmac driver. This patch adds a call to axi_dmac_alloc_desc(num_sgs), but the prototype of this function changed in my other patchset - it would have to be passed the "chan" variable. I don't know how you prefer it to be resolved. Worst case scenario (and if @Jonathan is okay with that) this one patch can be re-sent later, but it would make this patchset less "atomic". - [5/8]: - Use dev_err() instead of pr_err() - Inline to_iio_dma_fence() - Add comment to explain why we unref twice when detaching dmabuf - Remove TODO comment. It is actually safe to free the file's private data even when transfers are still pending because it won't be accessed. - Fix documentation of new fields in struct iio_buffer_access_funcs - iio_dma_resv_lock() does not need to be exported, make it static - [7/8]: - Use the new dmaengine_prep_slave_dma_vec(). - Restrict to input buffers, since output buffers are not yet supported by IIO buffers. - [8/8]: Use description lists for the documentation of the three new IOCTLs instead of abusing subsections. --- Alexandru Ardelean (1): iio: buffer-dma: split iio_dma_buffer_fileio_free() function Paul Cercueil (7): iio: buffer-dma: Get rid of outgoing queue dmaengine: Add API function dmaengine_prep_slave_dma_vec() dmaengine: dma-axi-dmac: Implement device_prep_slave_dma_vec iio: core: Add new DMABUF interface infrastructure iio: buffer-dma: Enable support for DMABUFs iio: buffer-dmaengine: Support new DMABUF based userspace API Documentation: iio: Document high-speed DMABUF based API Documentation/iio/dmabuf_api.rst | 54 +++ Documentation/iio/index.rst | 2 + drivers/dma/dma-axi-dmac.c | 40 ++ drivers/iio/buffer/industrialio-buffer-dma.c | 242 ++++++++--- .../buffer/industrialio-buffer-dmaengine.c | 52 ++- drivers/iio/industrialio-buffer.c | 402 ++++++++++++++++++ include/linux/dmaengine.h | 25 ++ include/linux/iio/buffer-dma.h | 33 +- include/linux/iio/buffer_impl.h | 26 ++ include/uapi/linux/iio/buffer.h | 22 + 10 files changed, 836 insertions(+), 62 deletions(-) create mode 100644 Documentation/iio/dmabuf_api.rst -- 2.43.0

1 year, 5 months

[PATCH 1/3] udmabuf: Keep track current device mappings

by Andrew Davis

When a device attaches to and maps our buffer we need to keep track of this mapping/device. This is needed for synchronization with these devices when beginning and ending CPU access for instance. Add a list that tracks device mappings as part of {map,unmap}_udmabuf(). Signed-off-by: Andrew Davis <afd(a)ti.com> --- drivers/dma-buf/udmabuf.c | 43 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index c406459996489..3a23f0a7d112a 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -28,6 +28,14 @@ struct udmabuf { struct page **pages; struct sg_table *sg; struct miscdevice *device; + struct list_head attachments; + struct mutex lock; +}; + +struct udmabuf_attachment { + struct device *dev; + struct sg_table *table; + struct list_head list; }; static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf) @@ -120,14 +128,42 @@ static void put_sg_table(struct device *dev, struct sg_table *sg, static struct sg_table *map_udmabuf(struct dma_buf_attachment *at, enum dma_data_direction direction) { - return get_sg_table(at->dev, at->dmabuf, direction); + struct udmabuf *ubuf = at->dmabuf->priv; + struct udmabuf_attachment *a; + + a = kzalloc(sizeof(*a), GFP_KERNEL); + if (!a) + return ERR_PTR(-ENOMEM); + + a->table = get_sg_table(at->dev, at->dmabuf, direction); + if (IS_ERR(a->table)) { + kfree(a); + return a->table; + } + + a->dev = at->dev; + + mutex_lock(&ubuf->lock); + list_add(&a->list, &ubuf->attachments); + mutex_unlock(&ubuf->lock); + + return a->table; } static void unmap_udmabuf(struct dma_buf_attachment *at, struct sg_table *sg, enum dma_data_direction direction) { - return put_sg_table(at->dev, sg, direction); + struct udmabuf_attachment *a = at->priv; + struct udmabuf *ubuf = at->dmabuf->priv; + + mutex_lock(&ubuf->lock); + list_del(&a->list); + mutex_unlock(&ubuf->lock); + + put_sg_table(at->dev, sg, direction); + + kfree(a); } static void release_udmabuf(struct dma_buf *buf) @@ -263,6 +299,9 @@ static long udmabuf_create(struct miscdevice *device, memfd = NULL; } + INIT_LIST_HEAD(&ubuf->attachments); + mutex_init(&ubuf->lock); + exp_info.ops = &udmabuf_ops; exp_info.size = ubuf->pagecount << PAGE_SHIFT; exp_info.priv = ubuf; -- 2.39.2

1 year, 5 months

Re: [PATCH 2/3] udmabuf: Sync buffer mappings for attached devices

by Andrew Davis

On 1/26/24 1:25 AM, Kasireddy, Vivek wrote: >>>> Currently this driver creates a SGT table using the CPU as the >>>> target device, then performs the dma_sync operations against >>>> that SGT. This is backwards to how DMA-BUFs are supposed to behave. >>>> This may have worked for the case where these buffers were given >>>> only back to the same CPU that produced them as in the QEMU case. >>>> And only then because the original author had the dma_sync >>>> operations also backwards, syncing for the "device" on begin_cpu. >>>> This was noticed and "fixed" in this patch[0]. >>>> >>>> That then meant we were sync'ing from the CPU to the CPU using >>>> a pseudo-device "miscdevice". Which then caused another issue >>>> due to the miscdevice not having a proper DMA mask (and why should >>>> it, the CPU is not a DMA device). The fix for that was an even >>>> more egregious hack[1] that declares the CPU is coherent with >>>> itself and can access its own memory space.. >>>> >>>> Unwind all this and perform the correct action by doing the dma_sync >>>> operations for each device currently attached to the backing buffer. >>> Makes sense. >>> >>>> >>>> [0] commit 1ffe09590121 ("udmabuf: fix dma-buf cpu access") >>>> [1] commit 9e9fa6a9198b ("udmabuf: Set the DMA mask for the udmabuf >>>> device (v2)") >>>> >>>> Signed-off-by: Andrew Davis <afd(a)ti.com> >>>> --- >>>> drivers/dma-buf/udmabuf.c | 41 +++++++++++++++------------------------ >>>> 1 file changed, 16 insertions(+), 25 deletions(-) >>>> >>>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c >>>> index 3a23f0a7d112a..ab6764322523c 100644 >>>> --- a/drivers/dma-buf/udmabuf.c >>>> +++ b/drivers/dma-buf/udmabuf.c >>>> @@ -26,8 +26,6 @@ MODULE_PARM_DESC(size_limit_mb, "Max size of a >>>> dmabuf, in megabytes. Default is >>>> struct udmabuf { >>>> pgoff_t pagecount; >>>> struct page **pages; >>>> - struct sg_table *sg; >>>> - struct miscdevice *device; >>>> struct list_head attachments; >>>> struct mutex lock; >>>> }; >>>> @@ -169,12 +167,8 @@ static void unmap_udmabuf(struct >>>> dma_buf_attachment *at, >>>> static void release_udmabuf(struct dma_buf *buf) >>>> { >>>> struct udmabuf *ubuf = buf->priv; >>>> - struct device *dev = ubuf->device->this_device; >>>> pgoff_t pg; >>>> >>>> - if (ubuf->sg) >>>> - put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL); >>> What happens if the last importer maps the dmabuf but erroneously >>> closes it immediately? Would unmap somehow get called in this case? >>> >> >> Good question, had to scan the framework code a bit here. I thought >> closing a DMABUF handle would automatically unwind any current >> attachments/mappings, but it seems nothing in the framework does that. >> >> Looks like that is up to the importing drivers[0]: >> >>> Once a driver is done with a shared buffer it needs to call >>> dma_buf_detach() (after cleaning up any mappings) and then >>> release the reference acquired with dma_buf_get() by >>> calling dma_buf_put(). >> >> So closing a DMABUF after mapping without first unmapping it would >> be a bug in the importer, it is not the exporters problem to check > It may be a bug in the importer but wouldn't the memory associated > with the sg table and attachment get leaked if unmap doesn't get called > in this scenario? > Yes the attachment data would be leaked if unattach was not called, but that is true for all DMABUF exporters. The .release() callback is meant to be the mirror of the export function and it only cleans up that. Same for attach/unattach, map/unmap, etc.. If these calls are not balanced then yes they can leak memory. Since balance is guaranteed by the API, checking the balance should be done at that level, not in each and every exporter. If your comment is that we should add those checks into the DMABUF framework layer then I would agree. Andrew > Thanks, > Vivek > >> for (although some more warnings in the framework checking for that >> might not be a bad idea..). >> >> Andrew >> >> [0] https://www.kernel.org/doc/html/v6.7/driver-api/dma-buf.html >> >>> Thanks, >>> Vivek >>> >>>> - >>>> for (pg = 0; pg < ubuf->pagecount; pg++) >>>> put_page(ubuf->pages[pg]); >>>> kfree(ubuf->pages); >>>> @@ -185,33 +179,31 @@ static int begin_cpu_udmabuf(struct dma_buf >>>> *buf, >>>> enum dma_data_direction direction) >>>> { >>>> struct udmabuf *ubuf = buf->priv; >>>> - struct device *dev = ubuf->device->this_device; >>>> - int ret = 0; >>>> - >>>> - if (!ubuf->sg) { >>>> - ubuf->sg = get_sg_table(dev, buf, direction); >>>> - if (IS_ERR(ubuf->sg)) { >>>> - ret = PTR_ERR(ubuf->sg); >>>> - ubuf->sg = NULL; >>>> - } >>>> - } else { >>>> - dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents, >>>> - direction); >>>> - } >>>> + struct udmabuf_attachment *a; >>>> >>>> - return ret; >>>> + mutex_lock(&ubuf->lock); >>>> + >>>> + list_for_each_entry(a, &ubuf->attachments, list) >>>> + dma_sync_sgtable_for_cpu(a->dev, a->table, direction); >>>> + >>>> + mutex_unlock(&ubuf->lock); >>>> + >>>> + return 0; >>>> } >>>> >>>> static int end_cpu_udmabuf(struct dma_buf *buf, >>>> enum dma_data_direction direction) >>>> { >>>> struct udmabuf *ubuf = buf->priv; >>>> - struct device *dev = ubuf->device->this_device; >>>> + struct udmabuf_attachment *a; >>>> >>>> - if (!ubuf->sg) >>>> - return -EINVAL; >>>> + mutex_lock(&ubuf->lock); >>>> + >>>> + list_for_each_entry(a, &ubuf->attachments, list) >>>> + dma_sync_sgtable_for_device(a->dev, a->table, direction); >>>> + >>>> + mutex_unlock(&ubuf->lock); >>>> >>>> - dma_sync_sg_for_device(dev, ubuf->sg->sgl, ubuf->sg->nents, >>>> direction); >>>> return 0; >>>> } >>>> >>>> @@ -307,7 +299,6 @@ static long udmabuf_create(struct miscdevice >>>> *device, >>>> exp_info.priv = ubuf; >>>> exp_info.flags = O_RDWR; >>>> >>>> - ubuf->device = device; >>>> buf = dma_buf_export(&exp_info); >>>> if (IS_ERR(buf)) { >>>> ret = PTR_ERR(buf); >>>> -- >>>> 2.39.2 >>>

1 year, 5 months

Re: [PATCH 2/3] udmabuf: Sync buffer mappings for attached devices

by Andrew Davis

On 1/24/24 5:05 PM, Kasireddy, Vivek wrote: > Hi Andrew, > >> Currently this driver creates a SGT table using the CPU as the >> target device, then performs the dma_sync operations against >> that SGT. This is backwards to how DMA-BUFs are supposed to behave. >> This may have worked for the case where these buffers were given >> only back to the same CPU that produced them as in the QEMU case. >> And only then because the original author had the dma_sync >> operations also backwards, syncing for the "device" on begin_cpu. >> This was noticed and "fixed" in this patch[0]. >> >> That then meant we were sync'ing from the CPU to the CPU using >> a pseudo-device "miscdevice". Which then caused another issue >> due to the miscdevice not having a proper DMA mask (and why should >> it, the CPU is not a DMA device). The fix for that was an even >> more egregious hack[1] that declares the CPU is coherent with >> itself and can access its own memory space.. >> >> Unwind all this and perform the correct action by doing the dma_sync >> operations for each device currently attached to the backing buffer. > Makes sense. > >> >> [0] commit 1ffe09590121 ("udmabuf: fix dma-buf cpu access") >> [1] commit 9e9fa6a9198b ("udmabuf: Set the DMA mask for the udmabuf >> device (v2)") >> >> Signed-off-by: Andrew Davis <afd(a)ti.com> >> --- >> drivers/dma-buf/udmabuf.c | 41 +++++++++++++++------------------------ >> 1 file changed, 16 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c >> index 3a23f0a7d112a..ab6764322523c 100644 >> --- a/drivers/dma-buf/udmabuf.c >> +++ b/drivers/dma-buf/udmabuf.c >> @@ -26,8 +26,6 @@ MODULE_PARM_DESC(size_limit_mb, "Max size of a >> dmabuf, in megabytes. Default is >> struct udmabuf { >> pgoff_t pagecount; >> struct page **pages; >> - struct sg_table *sg; >> - struct miscdevice *device; >> struct list_head attachments; >> struct mutex lock; >> }; >> @@ -169,12 +167,8 @@ static void unmap_udmabuf(struct >> dma_buf_attachment *at, >> static void release_udmabuf(struct dma_buf *buf) >> { >> struct udmabuf *ubuf = buf->priv; >> - struct device *dev = ubuf->device->this_device; >> pgoff_t pg; >> >> - if (ubuf->sg) >> - put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL); > What happens if the last importer maps the dmabuf but erroneously > closes it immediately? Would unmap somehow get called in this case? > Good question, had to scan the framework code a bit here. I thought closing a DMABUF handle would automatically unwind any current attachments/mappings, but it seems nothing in the framework does that. Looks like that is up to the importing drivers[0]: > Once a driver is done with a shared buffer it needs to call > dma_buf_detach() (after cleaning up any mappings) and then > release the reference acquired with dma_buf_get() by > calling dma_buf_put(). So closing a DMABUF after mapping without first unmapping it would be a bug in the importer, it is not the exporters problem to check for (although some more warnings in the framework checking for that might not be a bad idea..). Andrew [0] https://www.kernel.org/doc/html/v6.7/driver-api/dma-buf.html > Thanks, > Vivek > >> - >> for (pg = 0; pg < ubuf->pagecount; pg++) >> put_page(ubuf->pages[pg]); >> kfree(ubuf->pages); >> @@ -185,33 +179,31 @@ static int begin_cpu_udmabuf(struct dma_buf >> *buf, >> enum dma_data_direction direction) >> { >> struct udmabuf *ubuf = buf->priv; >> - struct device *dev = ubuf->device->this_device; >> - int ret = 0; >> - >> - if (!ubuf->sg) { >> - ubuf->sg = get_sg_table(dev, buf, direction); >> - if (IS_ERR(ubuf->sg)) { >> - ret = PTR_ERR(ubuf->sg); >> - ubuf->sg = NULL; >> - } >> - } else { >> - dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents, >> - direction); >> - } >> + struct udmabuf_attachment *a; >> >> - return ret; >> + mutex_lock(&ubuf->lock); >> + >> + list_for_each_entry(a, &ubuf->attachments, list) >> + dma_sync_sgtable_for_cpu(a->dev, a->table, direction); >> + >> + mutex_unlock(&ubuf->lock); >> + >> + return 0; >> } >> >> static int end_cpu_udmabuf(struct dma_buf *buf, >> enum dma_data_direction direction) >> { >> struct udmabuf *ubuf = buf->priv; >> - struct device *dev = ubuf->device->this_device; >> + struct udmabuf_attachment *a; >> >> - if (!ubuf->sg) >> - return -EINVAL; >> + mutex_lock(&ubuf->lock); >> + >> + list_for_each_entry(a, &ubuf->attachments, list) >> + dma_sync_sgtable_for_device(a->dev, a->table, direction); >> + >> + mutex_unlock(&ubuf->lock); >> >> - dma_sync_sg_for_device(dev, ubuf->sg->sgl, ubuf->sg->nents, >> direction); >> return 0; >> } >> >> @@ -307,7 +299,6 @@ static long udmabuf_create(struct miscdevice >> *device, >> exp_info.priv = ubuf; >> exp_info.flags = O_RDWR; >> >> - ubuf->device = device; >> buf = dma_buf_export(&exp_info); >> if (IS_ERR(buf)) { >> ret = PTR_ERR(buf); >> -- >> 2.39.2 >

1 year, 5 months

Re: [PATCH 1/3] udmabuf: Keep track current device mappings

by Andrew Davis

On 1/24/24 4:36 PM, Kasireddy, Vivek wrote: > Hi Andrew, > >> When a device attaches to and maps our buffer we need to keep track >> of this mapping/device. This is needed for synchronization with these >> devices when beginning and ending CPU access for instance. Add a list >> that tracks device mappings as part of {map,unmap}_udmabuf(). >> >> Signed-off-by: Andrew Davis <afd(a)ti.com> >> --- >> drivers/dma-buf/udmabuf.c | 43 >> +++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 41 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c >> index c406459996489..3a23f0a7d112a 100644 >> --- a/drivers/dma-buf/udmabuf.c >> +++ b/drivers/dma-buf/udmabuf.c >> @@ -28,6 +28,14 @@ struct udmabuf { >> struct page **pages; >> struct sg_table *sg; >> struct miscdevice *device; >> + struct list_head attachments; >> + struct mutex lock; >> +}; >> + >> +struct udmabuf_attachment { >> + struct device *dev; >> + struct sg_table *table; >> + struct list_head list; >> }; >> >> static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf) >> @@ -120,14 +128,42 @@ static void put_sg_table(struct device *dev, struct >> sg_table *sg, >> static struct sg_table *map_udmabuf(struct dma_buf_attachment *at, >> enum dma_data_direction direction) >> { >> - return get_sg_table(at->dev, at->dmabuf, direction); >> + struct udmabuf *ubuf = at->dmabuf->priv; >> + struct udmabuf_attachment *a; >> + >> + a = kzalloc(sizeof(*a), GFP_KERNEL); >> + if (!a) >> + return ERR_PTR(-ENOMEM); >> + >> + a->table = get_sg_table(at->dev, at->dmabuf, direction); >> + if (IS_ERR(a->table)) { >> + kfree(a); >> + return a->table; > Isn't that a use-after-free bug? Indeed it is, will fix. Seems coccicheck also caught this but I missed it when reviewing its output, my bad :( Andrew > Rest of the patch lgtm. > > Thanks, > Vivek > >> + } >> + >> + a->dev = at->dev; >> + >> + mutex_lock(&ubuf->lock); >> + list_add(&a->list, &ubuf->attachments); >> + mutex_unlock(&ubuf->lock); >> + >> + return a->table; >> } >> >> static void unmap_udmabuf(struct dma_buf_attachment *at, >> struct sg_table *sg, >> enum dma_data_direction direction) >> { >> - return put_sg_table(at->dev, sg, direction); >> + struct udmabuf_attachment *a = at->priv; >> + struct udmabuf *ubuf = at->dmabuf->priv; >> + >> + mutex_lock(&ubuf->lock); >> + list_del(&a->list); >> + mutex_unlock(&ubuf->lock); >> + >> + put_sg_table(at->dev, sg, direction); >> + >> + kfree(a); >> } >> >> static void release_udmabuf(struct dma_buf *buf) >> @@ -263,6 +299,9 @@ static long udmabuf_create(struct miscdevice >> *device, >> memfd = NULL; >> } >> >> + INIT_LIST_HEAD(&ubuf->attachments); >> + mutex_init(&ubuf->lock); >> + >> exp_info.ops = &udmabuf_ops; >> exp_info.size = ubuf->pagecount << PAGE_SHIFT; >> exp_info.priv = ubuf; >> -- >> 2.39.2 >

1 year, 5 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig