| Message ID | 1462186753-4177-1-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
Hi Hans On 2016-05-04 10:32, Hans Verkuil wrote: > Hi Marek, > > On 05/04/2016 10:28 AM, Marek Szyprowski wrote: >> Hi Hans, >> >> >> On 2016-05-04 10:22, Hans Verkuil wrote: >>> Hi Marek, >>> >>> On 05/02/2016 12:59 PM, Marek Szyprowski wrote: >>>> This patch lets vb2-dma-contig memory allocator to configure DMA max >>>> segment size properly for the client device. Setting it is needed to let >>>> DMA-mapping subsystem to create a single, contiguous mapping in DMA >>>> address space. This is essential for all devices, which use dma-contig >>>> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes >>>> of operations). >>>> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> --- >>>> Hello, >>>> >>>> This patch is a follow-up of my previous attempts to let Exynos >>>> multimedia devices to work properly with shared buffers when IOMMU is >>>> enabled: >>>> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html >>>> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316 >>>> 3. https://patchwork.linuxtv.org/patch/30870/ >>>> >>>> As sugested by Hans, configuring DMA max segment size should be done by >>>> videobuf2-dma-contig module instead of requiring all device drivers to >>>> do it on their own. >>>> >>>> Here is some backgroud why this is done in videobuf2-dc not in the >>>> respective generic bus code: >>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html >>>> >>>> Best regards, >>>> Marek Szyprowski >>>> >>>> changelog: >>>> v3: >>>> - added FIXME note about possible memory leak >>>> >>>> v2: >>>> - fixes typos and other language issues in the comments >>>> >>>> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690 >>>> --- >>>> drivers/media/v4l2-core/videobuf2-dma-contig.c | 45 ++++++++++++++++++++++++++ >>>> 1 file changed, 45 insertions(+) >>>> >>>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c >>>> index 461ae55eaa98..2ca7e798f394 100644 >>>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c >>>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c >>>> @@ -443,6 +443,42 @@ static void vb2_dc_put_userptr(void *buf_priv) >>>> } >>>> >>>> /* >>>> + * To allow mapping the scatter-list into a single chunk in the DMA >>>> + * address space, the device is required to have the DMA max segment >>>> + * size parameter set to a value larger than the buffer size. Otherwise, >>>> + * the DMA-mapping subsystem will split the mapping into max segment >>>> + * size chunks. This function increases the DMA max segment size >>>> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA >>>> + * address space. >>>> + * This code assumes that the DMA-mapping subsystem will merge all >>>> + * scatterlist segments if this is really possible (for example when >>>> + * an IOMMU is available and enabled). >>>> + * Ideally, this parameter should be set by the generic bus code, but it >>>> + * is left with the default 64KiB value due to historical litmiations in >>>> + * other subsystems (like limited USB host drivers) and there no good >>>> + * place to set it to the proper value. It is done here to avoid fixing >>>> + * all the vb2-dc client drivers. >>>> + * >>>> + * FIXME: the allocated dma_params structure is leaked because there >>>> + * is completely no way to determine when to free it (dma_params might have >>>> + * been also already allocated by the bus code). However in typical >>>> + * use cases this function will be called for platform devices, which are >>>> + * not how-plugged and exist all the time in the target system. >>>> + */ >>>> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size) >>>> +{ >>>> + if (!dev->dma_parms) { >>>> + dev->dma_parms = kzalloc(sizeof(dev->dma_parms), GFP_KERNEL); >>>> + if (!dev->dma_parms) >>>> + return -ENOMEM; >>>> + } >>>> + if (dma_get_max_seg_size(dev) < size) >>>> + return dma_set_max_seg_size(dev, size); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +/* >>>> * For some kind of reserved memory there might be no struct page available, >>>> * so all that can be done to support such 'pages' is to try to convert >>>> * pfn to dma address or at the last resort just assume that >>>> @@ -499,6 +535,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr, >>>> return ERR_PTR(-EINVAL); >>>> } >>>> >>>> + ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE)); >>> Huh? Against which kernel do you compile? The get_userptr prototype is different >>> from the latest mainline kernel. Specifically, dev is now conf->dev. >> I prepared it on top of your 'context3' branch, as you requested not to >> use the >> allocator context related functions, which best suit for this purpose. > That's not quite what I meant, sorry for the confusion. My reference to the > context3 branch was just that: to show upcoming changes and why it was not a > good idea to call this function from the context allocate/free functions. Since > those will disappear. > > The context3 branch isn't for 4.7 (too late for that), but I want to get it > in early in the 4.8 cycle. > > So just base this patch on the latest media_tree master. I will send a version based on media tree in a few minutes. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 461ae55eaa98..2ca7e798f394 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c @@ -443,6 +443,42 @@ static void vb2_dc_put_userptr(void *buf_priv) } /* + * To allow mapping the scatter-list into a single chunk in the DMA + * address space, the device is required to have the DMA max segment + * size parameter set to a value larger than the buffer size. Otherwise, + * the DMA-mapping subsystem will split the mapping into max segment + * size chunks. This function increases the DMA max segment size + * parameter to let DMA-mapping map a buffer as a single chunk in DMA + * address space. + * This code assumes that the DMA-mapping subsystem will merge all + * scatterlist segments if this is really possible (for example when + * an IOMMU is available and enabled). + * Ideally, this parameter should be set by the generic bus code, but it + * is left with the default 64KiB value due to historical litmiations in + * other subsystems (like limited USB host drivers) and there no good + * place to set it to the proper value. It is done here to avoid fixing + * all the vb2-dc client drivers. + * + * FIXME: the allocated dma_params structure is leaked because there + * is completely no way to determine when to free it (dma_params might have + * been also already allocated by the bus code). However in typical + * use cases this function will be called for platform devices, which are + * not how-plugged and exist all the time in the target system. + */ +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size) +{ + if (!dev->dma_parms) { + dev->dma_parms = kzalloc(sizeof(dev->dma_parms), GFP_KERNEL); + if (!dev->dma_parms) + return -ENOMEM; + } + if (dma_get_max_seg_size(dev) < size) + return dma_set_max_seg_size(dev, size); + + return 0; +} + +/* * For some kind of reserved memory there might be no struct page available, * so all that can be done to support such 'pages' is to try to convert * pfn to dma address or at the last resort just assume that @@ -499,6 +535,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr, return ERR_PTR(-EINVAL); } + ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE)); + if (!ret) + return ERR_PTR(ret); + buf = kzalloc(sizeof *buf, GFP_KERNEL); if (!buf) return ERR_PTR(-ENOMEM); @@ -675,10 +715,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf, { struct vb2_dc_buf *buf; struct dma_buf_attachment *dba; + int ret; if (dbuf->size < size) return ERR_PTR(-EFAULT); + ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size)); + if (!ret) + return ERR_PTR(ret); + buf = kzalloc(sizeof(*buf), GFP_KERNEL); if (!buf) return ERR_PTR(-ENOMEM);
This patch lets vb2-dma-contig memory allocator to configure DMA max segment size properly for the client device. Setting it is needed to let DMA-mapping subsystem to create a single, contiguous mapping in DMA address space. This is essential for all devices, which use dma-contig videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes of operations). Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- Hello, This patch is a follow-up of my previous attempts to let Exynos multimedia devices to work properly with shared buffers when IOMMU is enabled: 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316 3. https://patchwork.linuxtv.org/patch/30870/ As sugested by Hans, configuring DMA max segment size should be done by videobuf2-dma-contig module instead of requiring all device drivers to do it on their own. Here is some backgroud why this is done in videobuf2-dc not in the respective generic bus code: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html Best regards, Marek Szyprowski changelog: v3: - added FIXME note about possible memory leak v2: - fixes typos and other language issues in the comments v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690 --- drivers/media/v4l2-core/videobuf2-dma-contig.c | 45 ++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) -- 1.9.2 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html