Hello,
On Saturday, December 04, 2010 11:47 PM Pawel Osciak wrote:
> 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.
I always try to keep the code from the beginning of the function when I reply
and snip some not important parts of the mail. I must have missed something.
> 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.
Allocator context is needed to set up IOMMU mapping and I don't see any reason
why to limit it only to mmap mode.
> >> > +{
> >> > + 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.
I've redesigned it in a bit different way, please see v6 patch series.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
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
