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@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> > 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-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
