Pawel Osciak wrote: > -----Original Message----- > From: Pawel Osciak [mailto:pawel@xxxxxxxxxx] > Sent: Sunday, December 05, 2010 7:47 AM > To: Marek Szyprowski; Jonghun Han > Cc: linux-media@xxxxxxxxxxxxxxx; kyungmin.park@xxxxxxxxxxx > Subject: Re: [PATCH 7/7] v4l: videobuf2: add CMA allocator > > Hi, > please see my comments below. > Also, if I could suggest something, please cut the code between > functions, not inside them, it'll make it easier for others to find > that location in the original code. > > On Wed, Dec 1, 2010 at 00:56, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> > wrote: > > Hello, > > > > On Wednesday, December 01, 2010 9:36 AM Jonghun Han wrote: > > > >> Marek Szyprowski wrote: > >> 2010/11/20 Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>: > >> > From: Pawel Osciak <p.osciak@xxxxxxxxxxx> > >> > > >> > Add support for the CMA contiguous memory allocator to videobuf2. > >> > > >> > Signed-off-by: Pawel Osciak <p.osciak@xxxxxxxxxxx> > >> > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> > >> > Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> > >> > CC: Pawel Osciak <pawel@xxxxxxxxxx> > >> > --- > >> > >> Hi Marek, > >> > >> <snip> > >> > >> > +static void *vb2_cma_get_userptr(unsigned long vaddr, unsigned long size) > >> > >> static void *vb2_cma_get_userptr(unsigned long vaddr, unsigned long size, > >> const struct vb2_alloc_ctx > >> *alloc_ctx) > >> alloc_ctx can be useful for many cases. > > > > Yes, right, I will add it in the next version. > > > > Actually, I don't think so. Can you give an example of one of those > "many cases"? There is no need to add unused arguments that "can be > useful in future" just for the sake of future extensions, especially > not in the kernel in such sensitive code. This function can > potentially be called on each frame. And even more importantly, the > whole allocator interface has been designed so that functions will not > have to be vb2- or media-specific. Ideally, we should be able to hook > up functions from mm and other modules. And such functions either use > only vaddr and size, or have means of finding their contexts > otherwise. > If I remember correctly, this approach was actually suggested by > Laurent and it makes a lot of sense. We want to be able to use third > party kernel functions in allocators as much as possbile. > Hi, What I mentioned many cases is related in VCMM(Virtual Contiguous Memory Manager). http://www.linuxsymposium.org/2010/view_abstract.php?content_key=64 When IOMMU(SYS.MMU) is used for device, each device uses its own device virtual address not a physical address or virtual address. VCMM device id is needed to get the device virtual address. But in vb2_cma_get_userptr we cannot find the VCMM device id. If vb2_alloc_ctx is added as argument in vb2_cma_get_userptr and VCMM device id is set in vb2_cma_conf on vb2_cma_init, we can find VCMM device id in vb2_cma_get_userptr. > >> > +{ > >> > + struct vb2_cma_buf *buf; > >> > + unsigned long paddr = 0; > >> > + int ret; > >> > + > >> > + buf = kzalloc(sizeof *buf, GFP_KERNEL); > >> > + if (!buf) > >> > + return ERR_PTR(-ENOMEM); > >> > + > >> > + buf->vma = vb2_get_userptr(vaddr); > >> > >> vb2_get_vma looks good instead of vb2_get_userptr. > >> How do you think about this ? > > > > Right, this will make the code easier to understand. Thanks for the hint! > > > > Actually, I don't think it's a good idea. I think you have the whole > concept backwards. > vb2 doesn't care what is returned from this function. All it wants is > a cookie from this allocator. From the point of view where this > function is used, it is "get_userptr" not "get_vma", vb2 wants to get > something describing a user pointer, but that does not have to mean a > vma. Moreover, in any case, the core does not care at all what it is > it is getting, since it *never uses it*. Changing the name of this > function would actually make it confusing. > > In other words: this function is an implementation of > "get_userptr_private_allocator_cookie" allocator interface function > and *not* a "get_vma" function implementation. It does not have to be > a vma and the core does not care what it is anyway. > What I mentioned is vb2_get_userptr not vb2_cma_get_userptr(vb2_mem_ops->get_userptr). vb2_get_userptr is used in vb2_cma_get_userptr to find struct vm_area in videobuf2-memops.c Cookie concept is very nice. I absolutely agree with you. > >> > + if (!buf->vma) { > >> > + printk(KERN_ERR "Failed acquiring VMA for vaddr > >> 0x%08lx\n", > >> > + vaddr); > >> > + ret = -EINVAL; > >> > + goto done; > >> > + } > >> > + > >> > + ret = vb2_contig_verify_userptr(buf->vma, vaddr, size, &paddr); > >> > + if (ret) { > >> > + vb2_put_userptr(buf->vma); > >> > + goto done; > >> > + } > >> > + > >> > >> In vb2_contig_verify_userptr, vma is re-found via find_vma although it has > >> been checked after vb2_get_userptr. > >> Why double checking is needed ? > > > > I'm not sure. I must check this core again, maybe it can be simplified a bit. > > I agree, it seems that it should be enough to remove find_vma from the > gerneric verify contig function. > > -- > Best regards, > Pawel Osciak -- 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
