Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

15 participants
3030 discussions

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

by Christoph Hellwig

On Thu, Oct 24, 2024 at 05:40:02PM +0100, Pavel Begunkov wrote: > On 10/24/24 17:06, Christoph Hellwig wrote: > > On Thu, Oct 24, 2024 at 03:23:06PM +0100, Pavel Begunkov wrote: > > > > That's not what this series does. It adds the new memory_provider_ops > > > > set of hooks, with once implementation for dmabufs, and one for > > > > io_uring zero copy. > > > > > > First, it's not a _new_ abstraction over a buffer as you called it > > > before, the abstraction (net_iov) is already merged. > > > > Umm, it is a new ops vector. > > I don't understand what you mean. Callback? struct memory_provider_ops. It's a method table or ops vetor, no callbacks involved. > Then please go ahead and take a look at the patchset in question > and see how much of dmabuf handling is there comparing to pure > networking changes. The point that it's a new set of API and lots > of changes not related directly to dmabufs stand. dmabufs is useful > there as an abstraction there, but it's a very long stretch saying > that the series is all about it. I did take a look, that's why I replied. > > > on an existing network specific abstraction, which are not restricted to > > > pages or anything specific in the long run, but the flow of which from > > > net stack to user and back is controlled by io_uring. If you worry about > > > abuse, io_uring can't even sanely initialise those buffers itself and > > > therefore asking the page pool code to do that. > > > > No, I worry about trying to io_uring for not good reason. This > > It sounds that the argument is that you just don't want any > io_uring APIs, I don't think you'd be able to help you with > that. No, that's complete misinterpreting what I'm saying. Of course an io_uring API is fine. But tying low-level implementation details to to is not. > > pre-cludes in-kernel uses which would be extremly useful for > > Uses of what? devmem TCP is merged, I'm not removing it, > and the net_iov abstraction is in there, which can be potentially > be reused by other in-kernel users if that'd even make sense. How when you are hardcoding io uring memory registrations instead of making them a generic dmabuf? Which btw would also really help with pre-registering the memry with the iommu to get good performance in IOMMU-enabled setups.

10 months, 3 weeks

Important Message: Incoming mails have been suspended in your email server !!!

by Server Administrator lists.linaro.org

10 months, 3 weeks

Re: [PATCH 1/3] dma-buf/dma-fence_array: use kvzalloc

by Christian König

Am 25.10.24 um 11:05 schrieb Tvrtko Ursulin: > > On 25/10/2024 09:59, Tvrtko Ursulin wrote: >> >> On 24/10/2024 13:41, Christian König wrote: >>> Reports indicates that some userspace applications try to merge more >>> than >>> 80k of fences into a single dma_fence_array leading to a warning from >>> kzalloc() that the requested size becomes to big. >>> >>> While that is clearly an userspace bug we should probably handle >>> that case >>> gracefully in the kernel. >>> >>> So we can either reject requests to merge more than a reasonable >>> amount of >>> fences (64k maybe?) or we can start to use kvzalloc() instead of >>> kzalloc(). >>> This patch here does the later. >> >> Rejecting would potentially be safer, otherwise there is a path for >> userspace to trigger a warn in kvmalloc_node (see 0829b5bcdd3b >> ("drm/i915: 2 GiB of relocations ought to be enough for anybody*")) >> and spam dmesg at will. > > Actually that is a WARN_ON_*ONCE* there so maybe not so critical to > invent a limit. Up for discussion I suppose. > > Regards, > > Tvrtko > >> >> Question is what limit to set... That's one of the reasons why I opted for kvzalloc() initially. I mean we could use some nice round number like 65536, but that would be totally arbitrary. Any comments on the other two patches? I need to get them upstream. Thanks, Christian. >> >> Regards, >> >> Tvrtko >> >>> Signed-off-by: Christian König <christian.koenig(a)amd.com> >>> CC: stable(a)vger.kernel.org >>> --- >>> drivers/dma-buf/dma-fence-array.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-fence-array.c >>> b/drivers/dma-buf/dma-fence-array.c >>> index 8a08ffde31e7..46ac42bcfac0 100644 >>> --- a/drivers/dma-buf/dma-fence-array.c >>> +++ b/drivers/dma-buf/dma-fence-array.c >>> @@ -119,8 +119,8 @@ static void dma_fence_array_release(struct >>> dma_fence *fence) >>> for (i = 0; i < array->num_fences; ++i) >>> dma_fence_put(array->fences[i]); >>> - kfree(array->fences); >>> - dma_fence_free(fence); >>> + kvfree(array->fences); >>> + kvfree_rcu(fence, rcu); >>> } >>> static void dma_fence_array_set_deadline(struct dma_fence *fence, >>> @@ -153,7 +153,7 @@ struct dma_fence_array >>> *dma_fence_array_alloc(int num_fences) >>> { >>> struct dma_fence_array *array; >>> - return kzalloc(struct_size(array, callbacks, num_fences), >>> GFP_KERNEL); >>> + return kvzalloc(struct_size(array, callbacks, num_fences), >>> GFP_KERNEL); >>> } >>> EXPORT_SYMBOL(dma_fence_array_alloc);

10 months, 3 weeks

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

by Christoph Hellwig

On Thu, Oct 24, 2024 at 03:23:06PM +0100, Pavel Begunkov wrote: > > That's not what this series does. It adds the new memory_provider_ops > > set of hooks, with once implementation for dmabufs, and one for > > io_uring zero copy. > > First, it's not a _new_ abstraction over a buffer as you called it > before, the abstraction (net_iov) is already merged. Umm, it is a new ops vector. > Second, you mention devmem TCP, and it's not just a page pool with > "dmabufs", it's a user API to use it and other memory agnostic > allocation logic. And yes, dmabufs there is the least technically > important part. Just having a dmabuf handle solves absolutely nothing. It solves a lot, becaue it provides a proper abstraction. > > So you are precluding zero copy RX into anything but your magic > > io_uring buffers, and using an odd abstraction for that. > > Right io_uring zero copy RX API expects transfer to happen into io_uring > controlled buffers, and that's the entire idea. Buffers that are based > on an existing network specific abstraction, which are not restricted to > pages or anything specific in the long run, but the flow of which from > net stack to user and back is controlled by io_uring. If you worry about > abuse, io_uring can't even sanely initialise those buffers itself and > therefore asking the page pool code to do that. No, I worry about trying to io_uring for not good reason. This pre-cludes in-kernel uses which would be extremly useful for network storage drivers, and it precludes device memory of all kinds. > I'm even more confused how that would help. The user API has to > be implemented and adding a new dmabuf gives nothing, not even > mentioning it's not clear what semantics of that beast is > supposed to be. > The dma-buf maintainers already explained to you last time that there is absolutely no need to use the dmabuf UAPI, you can use dma-bufs through in-kernel interfaces just fine.

10 months, 3 weeks

Fix Fix fence merge handling

by Christian König

Hi guys, turned out that userspace can also merge dma_fence_chain contains which can result in really huge arrays. Fix those merges to sort the arrays and remove the duplicates. Additional to that start to use kvzalloc() for dma_fence_array containers so that can handle much larger arrays if necessary. Please review and comment, Christian.

10 months, 3 weeks

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

by Christoph Hellwig

On Wed, Oct 23, 2024 at 03:34:53PM +0100, Pavel Begunkov wrote: > It doesn't care much what kind of memory it is, nor it's important > for internals how it's imported, it's user addresses -> pages for > user convenience sake. All the net_iov setup code is in the page pool > core code. What it does, however, is implementing the user API, so That's not what this series does. It adds the new memory_provider_ops set of hooks, with once implementation for dmabufs, and one for io_uring zero copy. So you are precluding zero copy RX into anything but your magic io_uring buffers, and using an odd abstraction for that. The right way would be to support zero copy RX into every designated dmabuf, and make io_uring work with udmabuf or if absolutely needed it's own kind of dmabuf. Instead we create a maze of incompatible abstractions here. The use case of e.g. doing zero copy receive into a NVMe CMB using PCIe P2P transactions is every but made up, so this does create a problem.

10 months, 3 weeks

[PATCH] Documentation: dma-buf: heaps: Add heap name definitions

by Maxime Ripard

Following a recent discussion at last Plumbers, John Stultz, Sumit Sewal, TJ Mercier and I came to an agreement that we should document what the dma-buf heaps names are expected to be, and what the buffers attributes you'll get should be documented. Let's create that doc to make sure those attributes and names are guaranteed going forward. Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- To: Jonathan Corbet <corbet(a)lwn.net> To: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Benjamin Gaignard <benjamin.gaignard(a)collabora.com> Cc: Brian Starkey <Brian.Starkey(a)arm.com> Cc: John Stultz <jstultz(a)google.com> Cc: "T.J. Mercier" <tjmercier(a)google.com> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-media(a)vger.kernel.org Cc: linux-doc(a)vger.kernel.org --- Documentation/userspace-api/dma-buf-heaps.rst | 71 +++++++++++++++++++ Documentation/userspace-api/index.rst | 1 + 2 files changed, 72 insertions(+) create mode 100644 Documentation/userspace-api/dma-buf-heaps.rst diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst new file mode 100644 index 000000000000..00436227b542 --- /dev/null +++ b/Documentation/userspace-api/dma-buf-heaps.rst @@ -0,0 +1,71 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================== +Allocating dma-buf using heaps +============================== + +Dma-buf Heaps are a way for userspace to allocate dma-buf objects. They are +typically used to allocate buffers from a specific allocation pool, or to share +buffers across frameworks. + +Heaps +===== + +A heap represent a specific allocator. The Linux kernel currently supports the +following heaps: + + - The ``system`` heap allocates virtually contiguous, cacheable, buffers + + - The ``reserved`` heap allocates physically contiguous, cacheable, buffers. + Depending on the platform, it might be called differently: + + - Acer Iconia Tab A500: ``linux,cma`` + - Allwinner sun4i, sun5i and sun7i families: ``default-pool`` + - Amlogic A1: ``linux,cma`` + - Amlogic G12A/G12B/SM1: ``linux,cma`` + - Amlogic GXBB/GXL: ``linux,cma`` + - ASUS EeePad Transformer TF101: ``linux,cma`` + - ASUS Google Nexus 7 (Project Bach / ME370TG) E1565: ``linux,cma`` + - ASUS Google Nexus 7 (Project Nakasi / ME370T) E1565: ``linux,cma`` + - ASUS Google Nexus 7 (Project Nakasi / ME370T) PM269: ``linux,cma`` + - Asus Transformer Infinity TF700T: ``linux,cma`` + - Asus Transformer Pad 3G TF300TG: ``linux,cma`` + - Asus Transformer Pad TF300T: ``linux,cma`` + - Asus Transformer Pad TF701T: ``linux,cma`` + - Asus Transformer Prime TF201: ``linux,cma`` + - ASUS Vivobook S 15: ``linux,cma`` + - Cadence KC705: ``linux,cma`` + - Digi International ConnectCore 6UL: ``linux,cma`` + - Freescale i.MX8DXL EVK: ``linux,cma`` + - Freescale TQMa8Xx: ``linux,cma`` + - Hisilicon Hikey: ``linux,cma`` + - Lenovo ThinkPad T14s Gen 6: ``linux,cma`` + - Lenovo ThinkPad X13s: ``linux,cma`` + - Lenovo Yoga Slim 7x: ``linux,cma`` + - LG Optimus 4X HD P880: ``linux,cma`` + - LG Optimus Vu P895: ``linux,cma`` + - Loongson 2k0500, 2k1000 and 2k2000: ``linux,cma`` + - Microsoft Romulus: ``linux,cma`` + - NXP i.MX8ULP EVK: ``linux,cma`` + - NXP i.MX93 9x9 QSB: ``linux,cma`` + - NXP i.MX93 11X11 EVK: ``linux,cma`` + - NXP i.MX93 14X14 EVK: ``linux,cma`` + - NXP i.MX95 19X19 EVK: ``linux,cma`` + - Ouya Game Console: ``linux,cma`` + - Pegatron Chagall: ``linux,cma`` + - PHYTEC phyCORE-AM62A SOM: ``linux,cma`` + - PHYTEC phyCORE-i.MX93 SOM: ``linux,cma`` + - Qualcomm SC8280XP CRD: ``linux,cma`` + - Qualcomm X1E80100 CRD: ``linux,cma`` + - Qualcomm X1E80100 QCP: ``linux,cma`` + - RaspberryPi: ``linux,cma`` + - Texas Instruments AM62x SK board family: ``linux,cma`` + - Texas Instruments AM62A7 SK: ``linux,cma`` + - Toradex Apalis iMX8: ``linux,cma`` + - TQ-Systems i.MX8MM TQMa8MxML: ``linux,cma`` + - TQ-Systems i.MX8MN TQMa8MxNL: ``linux,cma`` + - TQ-Systems i.MX8MPlus TQMa8MPxL: ``linux,cma`` + - TQ-Systems i.MX8MQ TQMa8MQ: ``linux,cma`` + - TQ-Systems i.MX93 TQMa93xxLA/TQMa93xxCA SOM: ``linux,cma`` + - TQ-Systems MBA6ULx Baseboard: ``linux,cma`` + diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst index 274cc7546efc..4901ce7c6cb7 100644 --- a/Documentation/userspace-api/index.rst +++ b/Documentation/userspace-api/index.rst @@ -41,10 +41,11 @@ Devices and I/O .. toctree:: :maxdepth: 1 accelerators/ocxl + dma-buf-heaps dma-buf-alloc-exchange gpio/index iommufd media/index dcdbas -- 2.46.1

10 months, 3 weeks

Re: [PATCH v6 02/15] net: generalise net_iov chunk owners

by Christoph Hellwig

On Wed, Oct 16, 2024 at 11:52:39AM -0700, David Wei wrote: > From: Pavel Begunkov <asml.silence(a)gmail.com> > > Currently net_iov stores a pointer to struct dmabuf_genpool_chunk_owner, > which serves as a useful abstraction to share data and provide a > context. However, it's too devmem specific, and we want to reuse it for > other memory providers, and for that we need to decouple net_iov from > devmem. Make net_iov to point to a new base structure called > net_iov_area, which dmabuf_genpool_chunk_owner extends. We've been there before. Instead of reinventing your own memory provider please enhance dmabufs for your use case. We don't really need to build memory buffer abstraction over memory buffer abstraction.

10 months, 4 weeks

Re: Requirements to merge new heaps in the kernel

by John Stultz

On Tue, Oct 22, 2024 at 1:38 AM Maxime Ripard <mripard(a)redhat.com> wrote: > > I wanted to follow-up on the discussion we had at Plumbers with John and > T.J. about (among other things) adding new heaps to the kernel. > > I'm still interested in merging a carve-out driver[1], since it seems to be > in every vendor BSP and got asked again last week. > > I remember from our discussion that for new heap types to be merged, we > needed a kernel use-case. Looking back, I'm not entirely sure how one > can provide that given that heaps are essentially facilities for > user-space. > > Am I misremembering or missing something? What are the requirements for > you to consider adding a new heap driver? It's basically the same as the DRM subsystem rules. https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements ie: There has to be opensource user for it, and the user has to be more significant than a "toy" implementation (which can be a bit subjective and contentious when trying to get out of a chicken and egg loop). thanks -john

10 months, 4 weeks

Re: [PATCH v1 05/10] media: platform: mediatek: add isp_7x camsys unit

by Krzysztof Kozlowski

On 09/10/2024 13:15, Shu-hsiang Yang wrote: > Introduces the top media device driver for the MediaTek ISP7X CAMSYS. > The driver maintains the camera system, including sub-device management, > DMA operations, and integration with the V4L2 framework. It handles > request stream data, buffer management, and MediaTek-specific features, > and pipeline management, streaming control, error handling mechanism. > Additionally, it aggregates sub-drivers for the camera interface, raw > and yuv pipelines. > > Signed-off-by: Shu-hsiang Yang <Shu-hsiang.Yang(a)mediatek.com> ... > + > +static int mtk_cam_probe(struct platform_device *pdev) > +{ > + struct mtk_cam_device *cam_dev; > + struct device *dev = &pdev->dev; > + struct resource *res; > + int ret; > + unsigned int i; > + > + dev_dbg(dev, "camsys | start %s\n", __func__); NAK. Same issues. > + > + /* initialize structure */ > + cam_dev = devm_kzalloc(dev, sizeof(*cam_dev), GFP_KERNEL); > + if (!cam_dev) > + return -ENOMEM; > + > + if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(34))) { > + dev_err(dev, "%s: No suitable DMA available\n", __func__); > + return -EIO; > + } > + > + if (!dev->dma_parms) { > + dev->dma_parms = > + devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL); > + if (!dev->dma_parms) > + return -ENOMEM; > + } > + > + dma_set_max_seg_size(dev, UINT_MAX); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(dev, "failed to get mem\n"); > + return -ENODEV; > + } > + > + cam_dev->base = devm_ioremap_resource(dev, res); > + if (IS_ERR(cam_dev->base)) { > + dev_err(dev, "failed to map register base\n"); > + return PTR_ERR(cam_dev->base); > + } > + > + cam_dev->dev = dev; > + dev_set_drvdata(dev, cam_dev); > + > + cam_dev->composer_cnt = 0; > + cam_dev->num_seninf_devices = 0; > + > + cam_dev->max_stream_num = MTKCAM_SUBDEV_MAX; > + cam_dev->ctxs = devm_kcalloc(dev, cam_dev->max_stream_num, > + sizeof(*cam_dev->ctxs), GFP_KERNEL); > + if (!cam_dev->ctxs) > + return -ENOMEM; > + > + cam_dev->streaming_ctx = 0; > + for (i = 0; i < cam_dev->max_stream_num; i++) > + mtk_cam_ctx_init(cam_dev->ctxs + i, cam_dev, i); > + > + cam_dev->running_job_count = 0; > + spin_lock_init(&cam_dev->pending_job_lock); > + spin_lock_init(&cam_dev->running_job_lock); > + INIT_LIST_HEAD(&cam_dev->pending_job_list); > + INIT_LIST_HEAD(&cam_dev->running_job_list); > + > + cam_dev->dma_processing_count = 0; > + spin_lock_init(&cam_dev->dma_pending_lock); > + spin_lock_init(&cam_dev->dma_processing_lock); > + INIT_LIST_HEAD(&cam_dev->dma_pending); > + INIT_LIST_HEAD(&cam_dev->dma_processing); > + > + mutex_init(&cam_dev->queue_lock); > + > + pm_runtime_enable(dev); > + > + ret = mtk_cam_of_rproc(cam_dev, pdev); > + if (ret) > + goto fail_destroy_mutex; > + > + ret = register_sub_drivers(dev); > + if (ret) { > + dev_err(dev, "fail to register_sub_drivers\n"); > + goto fail_destroy_mutex; > + } > + > + /* register mtk_cam as all isp subdev async parent */ > + cam_dev->notifier.ops = &mtk_cam_async_nf_ops; > + v4l2_async_nf_init(&cam_dev->notifier, &cam_dev->v4l2_dev); > + ret = mtk_cam_async_subdev_add(dev); /* wait all isp sub drivers */ > + if (ret) { > + dev_err(dev, "%s failed mtk_cam_async_subdev_add\n", __func__); > + goto fail_unregister_sub_drivers; > + } > + > + ret = v4l2_async_nf_register(&cam_dev->notifier); > + if (ret) { > + dev_err(dev, "%s async_nf_register ret:%d\n", __func__, ret); > + v4l2_async_nf_cleanup(&cam_dev->notifier); > + goto fail_unregister_sub_drivers; > + } > + > + ret = mtk_cam_debug_fs_init(cam_dev); > + if (ret < 0) > + goto fail_unregister_async_nf; > + > + dev_info(dev, "camsys | [%s] success\n", __func__); NAK. Same issues. > + > + return 0; > + > +fail_unregister_async_nf: > + v4l2_async_nf_unregister(&cam_dev->notifier); > + > +fail_unregister_sub_drivers: > + unregister_sub_drivers(dev); > + > +fail_destroy_mutex: > + mutex_destroy(&cam_dev->queue_lock); > + > + return ret; > +} > + > +static void mtk_cam_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct mtk_cam_device *cam_dev = dev_get_drvdata(dev); > + > + pm_runtime_disable(dev); > + > + mtk_cam_debug_fs_deinit(cam_dev); > + > + v4l2_async_nf_unregister(&cam_dev->notifier); > + > + unregister_sub_drivers(dev); > + > + mutex_destroy(&cam_dev->queue_lock); > +} > + > +static const struct dev_pm_ops mtk_cam_pm_ops = { > + SET_RUNTIME_PM_OPS(mtk_cam_runtime_suspend, mtk_cam_runtime_resume, > + NULL) > +}; > + > +static struct platform_driver mtk_cam_driver = { > + .probe = mtk_cam_probe, > + .remove = mtk_cam_remove, > + .driver = { > + .name = "mtk-cam", > + .of_match_table = of_match_ptr(mtk_cam_of_ids), Same issues as in previous patch. All my comments apply to all your patches in this thread. > + .pm = &mtk_cam_pm_ops, > + } > +}; Best regards, Krzysztof

10 months, 4 weeks

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig