| Message ID | 1461849603-6313-1-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 04/28/2016 03:20 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> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> Should this go in for 4.7? (might be too late) Should this go into older kernels as well? Regards, Hans > --- > 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: > 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 | 39 ++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c > index 461ae55eaa98..d0382d62954d 100644 > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c > @@ -443,6 +443,36 @@ 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 > + * scatter-list 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. > + */ > +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 +529,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 +709,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); > -- 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
Hello, On 2016-04-28 15:29, Hans Verkuil wrote: > On 04/28/2016 03:20 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> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > Should this go in for 4.7? (might be too late) Should this go into older > kernels as well? Yes, please queue it to v4.7 if possible. It might be a good idea to backport this to older kernels, but at least in case of Exynos there are still other issues, which prevent using all multimedia drivers with IOMMU enabled (caused by Exynos MFC device special needs). > > Regards, > > Hans > >> --- >> 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: >> 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 | 39 ++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c >> index 461ae55eaa98..d0382d62954d 100644 >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c >> @@ -443,6 +443,36 @@ 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 >> + * scatter-list 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. >> + */ >> +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 +529,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 +709,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); >> > Best regards
On 04/28/2016 03:39 PM, Marek Szyprowski wrote: > Hello, > > > On 2016-04-28 15:29, Hans Verkuil wrote: >> On 04/28/2016 03:20 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> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> >> >> Should this go in for 4.7? (might be too late) Should this go into older >> kernels as well? Oops, typo. I meant 4.6, not 4.7. I gather that committing this for 4.6 is not necessary, so I just queue it up for 4.7. Regards, Hans -- 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
Hi Marek, Thanks for the patch! On Thu, Apr 28, 2016 at 03:20:03PM +0200, 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: > 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 | 39 ++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c > index 461ae55eaa98..d0382d62954d 100644 > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c > @@ -443,6 +443,36 @@ 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 > + * scatter-list 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. > + */ > +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); Doesn't this create a memory leak? Do consider that devices may be also removed from the system at runtime. Looks very nice otherwise. > + 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 +529,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 +709,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);
Hi Sakari, On 2016-04-29 13:21, Sakari Ailus wrote: > Hi Marek, > > Thanks for the patch! > > On Thu, Apr 28, 2016 at 03:20:03PM +0200, 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: >> 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 | 39 ++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c >> index 461ae55eaa98..d0382d62954d 100644 >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c >> @@ -443,6 +443,36 @@ 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 >> + * scatter-list 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. >> + */ >> +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); > Doesn't this create a memory leak? Do consider that devices may be also > removed from the system at runtime. > > Looks very nice otherwise. Yes it does, but there is completely no way to determine when to do that (dma_params might have been already allocated by the bus code). The whole dma_params idea and its handling is a bit messy. IMHO we can leave this for now until dma_params gets cleaned up (I remember someone said that he has this on his todo list, but I don't remember now who it was...). > >> + 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 +529,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 +709,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); Best regards
Hi Marek, On Fri, Apr 29, 2016 at 01:39:43PM +0200, Marek Szyprowski wrote: > Hi Sakari, > > > On 2016-04-29 13:21, Sakari Ailus wrote: > >Hi Marek, > > > >Thanks for the patch! > > > >On Thu, Apr 28, 2016 at 03:20:03PM +0200, 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: > >>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 | 39 ++++++++++++++++++++++++++ > >> 1 file changed, 39 insertions(+) > >> > >>diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c > >>index 461ae55eaa98..d0382d62954d 100644 > >>--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c > >>+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c > >>@@ -443,6 +443,36 @@ 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 > >>+ * scatter-list 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. > >>+ */ > >>+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); > >Doesn't this create a memory leak? Do consider that devices may be also > >removed from the system at runtime. > > > >Looks very nice otherwise. > > Yes it does, but there is completely no way to determine when to do that > (dma_params might have been already allocated by the bus code). The whole > dma_params idea and its handling is a bit messy. IMHO we can leave this > for now until dma_params gets cleaned up (I remember someone said that he > has this on his todo list, but I don't remember now who it was...). You could fix this in a V4L2-specific way by storing the information whether you've allocated the dma-params e.g. in struct video_device. That's probably not worth the trouble, especially if someone's about to have a better solution. I'd add a "FIXME: ..." comment so we won't forget about it. Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
On 04/29/16 15:56, Sakari Ailus wrote: > Hi Marek, > > On Fri, Apr 29, 2016 at 01:39:43PM +0200, Marek Szyprowski wrote: >> Hi Sakari, >> >> >> On 2016-04-29 13:21, Sakari Ailus wrote: >>> Hi Marek, >>> >>> Thanks for the patch! >>> >>> On Thu, Apr 28, 2016 at 03:20:03PM +0200, 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: >>>> 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 | 39 ++++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>>> >>>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c >>>> index 461ae55eaa98..d0382d62954d 100644 >>>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c >>>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c >>>> @@ -443,6 +443,36 @@ 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 >>>> + * scatter-list 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. >>>> + */ >>>> +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); >>> Doesn't this create a memory leak? Do consider that devices may be also >>> removed from the system at runtime. >>> >>> Looks very nice otherwise. >> >> Yes it does, but there is completely no way to determine when to do that >> (dma_params might have been already allocated by the bus code). The whole >> dma_params idea and its handling is a bit messy. IMHO we can leave this >> for now until dma_params gets cleaned up (I remember someone said that he >> has this on his todo list, but I don't remember now who it was...). > > You could fix this in a V4L2-specific way by storing the information whether > you've allocated the dma-params e.g. in struct video_device. That's probably > not worth the trouble, especially if someone's about to have a better > solution. > > I'd add a "FIXME: ..." comment so we won't forget about it. > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > I agree with Sakari, please add a FIXME here explaining the issue. I'll pick up the v3 patch for a pull request on Wednesday. Regards, Hans -- 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..d0382d62954d 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c @@ -443,6 +443,36 @@ 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 + * scatter-list 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. + */ +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 +529,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 +709,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: 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 | 39 ++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)