Re: [PATCH v4] media: vb2-dma-contig: configure DMA max segment size properly
Dear Mauro,
On 2016-05-09 12:09, Mauro Carvalho Chehab wrote:
Em Mon, 09 May 2016 08:13:06 +0200
Marek Szyprowski escreveu:
Hi Mauro
On 2016-05-06 20:52, Mauro Carvalho Chehab wrote:
Em Wed, 04 May 2016 11:00:03 +0200
Marek Szyprowski escreveu:
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
Acked-by: Sakari Ailus
---
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:
v4:
- rebased onto media master tree
- call vb2_dc_set_max_seg_size after allocating vb2 buf object
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 | 53
+-
1 file changed, 51 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 5361197f3e57..6291842a889f 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -448,6 +448,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 hot-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);
Why don't you use devm_kzalloc() here? dma_parms will then be freed
if the device gets hot-unplugged/unbind.
Although the structure will be freed, but the pointer in the struct device
will still point to the freed resource.
Then you'll need some other logic (maybe a kref?) both free it and zero
the pointer when it is safe.
Btw, the only two drivers that seem to dynamically allocate it are
using devm_*:
drivers/gpu/drm/exynos/exynos_drm_iommu.c: subdrv_dev->dma_parms =
devm_kzalloc(subdrv_dev,
drivers/gpu/drm/exynos/exynos_drm_iommu.c:
sizeof(*subdrv_dev->dma_parms),
drivers/gpu/drm/exynos/exynos_drm_iommu.c: if (!subdrv_dev->dma_parms)
drivers/gpu/drm/rockchip/rockchip_drm_drv.c:dev->dma_parms = devm_kzalloc(dev,
sizeof(*dev->dma_parms),
drivers/gpu/drm/rockchip/rockchip_drm_drv.c:if (!dev->dma_parms) {
On the other drivers, the struct is embed on some other struct
that is freed only after the need of dma_parms.
Please note that devm_ allocations
are freed on driver removal/unbind not on device removal.
Yes, I know. Personally, I don't like devm_ allocations due to that.
Yet, it is better to free it later than to keep it leaking.
That's why I
decided
to leak memory instead of allowing to access resource that has been freed.
T
Re: [PATCH v4] media: vb2-dma-contig: configure DMA max segment size properly
Em Mon, 09 May 2016 08:13:06 +0200
Marek Szyprowski escreveu:
> Hi Mauro
>
>
> On 2016-05-06 20:52, Mauro Carvalho Chehab wrote:
> > Em Wed, 04 May 2016 11:00:03 +0200
> > Marek Szyprowski escreveu:
> >
> >> 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
> >> Acked-by: Sakari Ailus
> >> ---
> >> 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:
> >> v4:
> >> - rebased onto media master tree
> >> - call vb2_dc_set_max_seg_size after allocating vb2 buf object
> >>
> >> 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 | 53
> >> +-
> >> 1 file changed, 51 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> index 5361197f3e57..6291842a889f 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> @@ -448,6 +448,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 hot-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);
> > Why don't you use devm_kzalloc() here? dma_parms will then be freed
> > if the device gets hot-unplugged/unbind.
>
> Although the structure will be freed, but the pointer in the struct device
> will still point to the freed resource.
Then you'll need some other logic (maybe a kref?) both free it and zero
the pointer when it is safe.
Btw, the only two drivers that seem to dynamically allocate it are
using devm_*:
drivers/gpu/drm/exynos/exynos_drm_iommu.c: subdrv_dev->dma_parms =
devm_kzalloc(subdrv_dev,
drivers/gpu/drm/exynos/exynos_drm_iommu.c:
sizeof(*subdrv_dev->dma_parms),
drivers/gpu/drm/exynos/exynos_drm_iommu.c: if (!subdrv_dev->dma_parms)
drivers/gpu/drm/rockchip/rockchip_drm_drv.c:dev->dma_parms =
devm_kzalloc(dev, sizeof(*dev->dma_parms),
drivers/gpu/drm/rockchip/rockchip_drm_drv.c:if (!dev->dma_parms) {
On the other drivers, the struct is embed on some other struct
that is f
Re: [PATCH v4] media: vb2-dma-contig: configure DMA max segment size properly
Hi Mauro
On 2016-05-06 20:52, Mauro Carvalho Chehab wrote:
Em Wed, 04 May 2016 11:00:03 +0200
Marek Szyprowski escreveu:
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
Acked-by: Sakari Ailus
---
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:
v4:
- rebased onto media master tree
- call vb2_dc_set_max_seg_size after allocating vb2 buf object
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 | 53 +-
1 file changed, 51 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 5361197f3e57..6291842a889f 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -448,6 +448,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 hot-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);
Why don't you use devm_kzalloc() here? dma_parms will then be freed
if the device gets hot-unplugged/unbind.
Although the structure will be freed, but the pointer in the struct device
will still point to the freed resource. Please note that devm_ allocations
are freed on driver removal/unbind not on device removal. That's why I
decided
to leak memory instead of allowing to access resource that has been freed.
And yes: it is possible to hot-unplug (actually, hot-unbind) a platform
device via sysfs.
Just assuming that only platform drivers will use dma-contig and adding
a memory leak here seems really ugly!
The whole handling of dma_params structure is really ugly and right now
there
is no good way of ensuring proper dma parameters.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] media: vb2-dma-contig: configure DMA max segment size properly
Em Wed, 04 May 2016 11:00:03 +0200
Marek Szyprowski escreveu:
> 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
> Acked-by: Sakari Ailus
> ---
> 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:
> v4:
> - rebased onto media master tree
> - call vb2_dc_set_max_seg_size after allocating vb2 buf object
>
> 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 | 53
> +-
> 1 file changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 5361197f3e57..6291842a889f 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -448,6 +448,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 hot-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);
Why don't you use devm_kzalloc() here? dma_parms will then be freed
if the device gets hot-unplugged/unbind.
And yes: it is possible to hot-unplug (actually, hot-unbind) a platform
device via sysfs.
Just assuming that only platform drivers will use dma-contig and adding
a memory leak here seems really ugly!
Regards,
Mauro
--
Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4] media: vb2-dma-contig: configure DMA max segment size properly
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
Acked-by: Sakari Ailus
---
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:
v4:
- rebased onto media master tree
- call vb2_dc_set_max_seg_size after allocating vb2 buf object
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 | 53 +-
1 file changed, 51 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 5361197f3e57..6291842a889f 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -448,6 +448,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 hot-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
@@ -509,6 +545,10 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned
long vaddr,
if (!buf)
return ERR_PTR(-ENOMEM);
+ ret = vb2_dc_set_max_seg_size(conf->dev, PAGE_ALIGN(size + PAGE_SIZE));
+ if (!ret)
+ goto fail_buf;
+
buf->dev = conf->dev;
buf->dma_dir = dma_dir;
@@ -682,6 +722,7 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct
dma_buf *dbuf,
struct vb2_dc_conf *conf = alloc_ctx;
struct vb2_dc_buf *buf;
struct dma_buf_attachment *dba;
+ int ret;
if (dbuf->size < size)
return ERR_PTR(-EFAULT);
@@ -690,13 +731,17 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct
dma_buf *dbuf,
if (!buf)
return ERR_PTR(-ENOMEM);
+ ret = vb2_dc_set_max_seg_size(conf->dev, PAGE_ALIGN(size));
+ if (!ret)
+ goto fail_buf;
+
buf->dev = conf->dev;
/* create attachment for the dmabuf with the user device */
dba = dma_buf_attach(dbuf, buf->dev);
if (IS_ERR(dba)) {
pr_err("failed to attach dmabuf\n");
-
