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@xxxxxxxxxxx> > >>--- > >>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@xxxxxxxxxxxxxxx/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@xxxxxxxxxxxxxxx> -- Kind regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
