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.
>> > +{
>> > + 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.
>> > + 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
