| Message ID | 1462352403-27418-1-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Em Wed, 04 May 2016 11:00:03 +0200 Marek Szyprowski <m.szyprowski@samsung.com> 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 <m.szyprowski@samsung.com> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.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: > 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
Hi Mauro On 2016-05-06 20:52, Mauro Carvalho Chehab wrote: > Em Wed, 04 May 2016 11:00:03 +0200 > Marek Szyprowski <m.szyprowski@samsung.com> 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 <m.szyprowski@samsung.com> >> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.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: >> 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
Em Mon, 09 May 2016 08:13:06 +0200 Marek Szyprowski <m.szyprowski@samsung.com> escreveu: > Hi Mauro > > > On 2016-05-06 20:52, Mauro Carvalho Chehab wrote: > > Em Wed, 04 May 2016 11:00:03 +0200 > > Marek Szyprowski <m.szyprowski@samsung.com> 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 <m.szyprowski@samsung.com> > >> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.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: > >> 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. The problem is that, if a non-platform driver needs to use DMA-CONTIG, the leak can be harmful, as things like qemu can unbind the host driver in order to use the device by the guest. Btw, currently, 3 non-platform drivers already use it: drivers/media/pci/dt3155/Kconfig: select VIDEOBUF2_DMA_CONTIG drivers/media/pci/solo6x10/Kconfig: select VIDEOBUF2_DMA_CONTIG drivers/media/pci/sta2x11/Kconfig: select VIDEOBUF2_DMA_CONTIG (and ideally ivtv and cx18 should be converted to VB2 dma-contig some day) Btw, as the above PCI drivers are not for ARM, I'm wandering if the assumptions you took on your patch are also valid on x86 and other architectures that support the PCI bus. In any case, if you want to do it generic, for all drivers, all bus controllers, you'll need to do lots of tests with non-ARM devices, to be 100% sure that it won't cause regressions. On a quick inspection, though, I suspect it will be doing the wrong thing for the above drivers, as drivers/pci/probe.c already sets dma_parms during pci_device_add(), and enforces a max segment size of 64K, by calling pci_set_dma_max_seg_size(dev, 65536); (with is a wrapper to dma_set_max_seg_size) > > 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. Yeah, agreed. Well, there are some alternatives (ordered from the better to the worse): 1) Fix the dma_parms core support; 2) Add a pair of functions to register/unregister dma_parms at include/media/videobuf2-dma-contig.h. The driver needing to set it should call the unregister function if it gets unbind/removed; 3) Add kref somewhere to do the dma_parms de-allocation in a sane way. 4) do any dma_parms-related code inside the drivers, and not at the core (probably not the best idea, but at least we confine the troubles); I think I would go for (2) for now, and try to do (1) for long run. Regards, Mauro -- 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
Dear Mauro, On 2016-05-09 12:09, Mauro Carvalho Chehab wrote: > Em Mon, 09 May 2016 08:13:06 +0200 > Marek Szyprowski <m.szyprowski@samsung.com> escreveu: > >> Hi Mauro >> >> >> On 2016-05-06 20:52, Mauro Carvalho Chehab wrote: >>> Em Wed, 04 May 2016 11:00:03 +0200 >>> Marek Szyprowski <m.szyprowski@samsung.com> 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 <m.szyprowski@samsung.com> >>>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.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: >>>> 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. > The problem is that, if a non-platform driver needs to use DMA-CONTIG, > the leak can be harmful, as things like qemu can unbind the host > driver in order to use the device by the guest. > > Btw, currently, 3 non-platform drivers already use it: > > drivers/media/pci/dt3155/Kconfig: select VIDEOBUF2_DMA_CONTIG > drivers/media/pci/solo6x10/Kconfig: select VIDEOBUF2_DMA_CONTIG > drivers/media/pci/sta2x11/Kconfig: select VIDEOBUF2_DMA_CONTIG > > (and ideally ivtv and cx18 should be converted to VB2 dma-contig some day) > > Btw, as the above PCI drivers are not for ARM, I'm wandering if the > assumptions you took on your patch are also valid on x86 and other > architectures that support the PCI bus. In any case, if you want to > do it generic, for all drivers, all bus controllers, you'll need to > do lots of tests with non-ARM devices, to be 100% sure that it won't > cause regressions. Well, the only case when vb2-dma-contig really 'works' is a hacked solution where userspace provides USERPTR buffer, which is in fact mmaped contig buffer from the other device. Also DMABUF made of contig buffer should work, assuming that its exporter won't split it into several chunks. Anything else will fail, regardless of the arch/platform and the presence of the IOMMU. Especially the PCI drivers will not support USERPTR/DMABUF modes properly due to limited dma_max_seg_size property. This is a historical mess related to scatterlist code, which we cannot do much to improve. On the other side there are DRM drivers which 'creatively' use scatterlists by ignoring dma_params limits. In case of vb2-dma-contig this approach cannot be applied, because it relies on dma-mapping subsystem, which always obeys the limits. Probably the best would be to leave existing scatterlist API with its limitations for storage/network drivers and develop something more suitable for mapping a whole buffer to DMA address space and it for media and drm (something like dma_map_page_array() or dma_map_pfn_array() will be much more suitable that using scatter list, which is designed for describing a 'buffer' with sub-page resolution, typical in case of storage and network devices). > On a quick inspection, though, I suspect it will be doing the wrong > thing for the above drivers, as drivers/pci/probe.c already sets > dma_parms during pci_device_add(), and enforces a max segment size of > 64K, by calling > pci_set_dma_max_seg_size(dev, 65536); > (with is a wrapper to dma_set_max_seg_size) > >>> 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. > Yeah, agreed. > > Well, there are some alternatives (ordered from the better to the > worse): > > 1) Fix the dma_parms core support; > 2) Add a pair of functions to register/unregister dma_parms at > include/media/videobuf2-dma-contig.h. The driver needing to set > it should call the unregister function if it gets unbind/removed; > 3) Add kref somewhere to do the dma_parms de-allocation in a sane > way. > 4) do any dma_parms-related code inside the drivers, and not at > the core (probably not the best idea, but at least we confine > the troubles); > > I think I would go for (2) for now, and try to do (1) for long > run. I've tried with (2) approach, but I was instructed by Hans to do it in the vb2-core. I can get back to such approach if you want. Best regards
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"); - kfree(buf); - return dba; + ret = PTR_ERR(dba); + goto fail_buf; } buf->dma_dir = dma_dir; @@ -704,6 +749,10 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf, buf->db_attach = dba; return buf; + +fail_buf: + kfree(buf); + return ERR_PTR(ret); } /*********************************************/