On Thursday, November 25, 2010 11:26:56 Marek Szyprowski wrote:
> Hello,
>
> On Thursday, November 25, 2010 11:07 AM Hans Verkuil wrote:
>
> > On Friday, November 19, 2010 16:55:40 Marek Szyprowski wrote:
> > > From: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> > >
> > > Add an implementation of contiguous virtual memory allocator and handling
> > > routines for videobuf2, implemented on top of vmalloc()/vfree() calls.
> > >
> > > 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>
> > > ---
> > > drivers/media/video/Kconfig | 5 +
> > > drivers/media/video/Makefile | 1 +
> > > drivers/media/video/videobuf2-vmalloc.c | 177 +++++++++++++++++++++++++++++++
> > > include/media/videobuf2-vmalloc.h | 21 ++++
> > > 4 files changed, 204 insertions(+), 0 deletions(-)
> > > create mode 100644 drivers/media/video/videobuf2-vmalloc.c
> > > create mode 100644 include/media/videobuf2-vmalloc.h
> > >
> > > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> > > index 83ce858..9351423 100644
> > > --- a/drivers/media/video/Kconfig
> > > +++ b/drivers/media/video/Kconfig
> > > @@ -55,6 +55,11 @@ config VIDEOBUF2_CORE
> > > config VIDEOBUF2_MEMOPS
> > > tristate
> > >
> > > +config VIDEOBUF2_VMALLOC
> > > + select VIDEOBUF2_CORE
> > > + select VIDEOBUF2_MEMOPS
> > > + tristate
> > > +
> > > #
> > > # Multimedia Video device configuration
> > > #
> > > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> > > index a97a2a0..538bee9 100644
> > > --- a/drivers/media/video/Makefile
> > > +++ b/drivers/media/video/Makefile
> > > @@ -116,6 +116,7 @@ obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
> > >
> > > obj-$(CONFIG_VIDEOBUF2_CORE) += videobuf2-core.o
> > > obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
> > > +obj-$(CONFIG_VIDEOBUF2_VMALLOC) += videobuf2-vmalloc.o
> > >
> > > obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
> > >
> > > diff --git a/drivers/media/video/videobuf2-vmalloc.c b/drivers/media/video/videobuf2-vmalloc.c
> > > new file mode 100644
> > > index 0000000..3310900
> > > --- /dev/null
> > > +++ b/drivers/media/video/videobuf2-vmalloc.c
> > > @@ -0,0 +1,177 @@
> > > +/*
> > > + * videobuf2-vmalloc.c - vmalloc memory allocator for videobuf2
> > > + *
> > > + * Copyright (C) 2010 Samsung Electronics
> > > + *
> > > + * Author: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation.
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/mm.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/vmalloc.h>
> > > +
> > > +#include <media/videobuf2-core.h>
> > > +#include <media/videobuf2-memops.h>
> > > +
> > > +struct vb2_vmalloc_conf {
> > > + struct vb2_alloc_ctx alloc_ctx;
> > > +};
> > > +
> > > +struct vb2_vmalloc_buf {
> > > + void *vaddr;
> > > + unsigned long size;
> > > + unsigned int refcount;
> > > +};
> > > +
> > > +static void *vb2_vmalloc_alloc(const struct vb2_alloc_ctx *alloc_ctx,
> > > + unsigned long size)
> > > +{
> > > + struct vb2_vmalloc_buf *buf;
> > > +
> > > + buf = kzalloc(sizeof *buf, GFP_KERNEL);
> > > + if (!buf)
> > > + return NULL;
> > > +
> > > + buf->size = size;
> > > + buf->vaddr = vmalloc_user(buf->size);
> > > + if (!buf->vaddr) {
> > > + printk(KERN_ERR "vmalloc of size %ld failed\n", buf->size);
> > > + kfree(buf);
> > > + return NULL;
> > > + }
> > > +
> > > + buf->refcount++;
> > > + printk(KERN_DEBUG "Allocated vmalloc buffer of size %ld at vaddr=%p\n",
> > > + buf->size, buf->vaddr);
> > > +
> > > + return buf;
> > > +}
> > > +
> > > +static void vb2_vmalloc_put(void *buf_priv)
> > > +{
> > > + struct vb2_vmalloc_buf *buf = buf_priv;
> > > +
> > > + buf->refcount--;
> > > +
> > > + if (0 == buf->refcount) {
> >
> > I think I would feel happier if refcount was of type atomic_t and you would use
> > the atomic decrease-and-test operation here. I know that this is almost certainly
> > called with some lock, but still...
>
> Yes, it is called with mm_sem taken in all cases. Maybe a comment in the source will
> be enough to indicate that this variable does not need to be atomic_t type?
> I can change it to atomic_t if this is really required anyway.
It definitely needs to be documented. But I wonder if what you say is true anyway:
vb2_mmap is called without mm_sem taken as far as I can tell, this calls the mmap
callback, vb2_vmalloc_mmap calls vb2_vmalloc_vm_open directly which does buf_refcount++.
Making refcount atomic would make it easier to prove correctness IMHO.
Anyway, this leads me to a second question: the documentation (either separate or in the
header) should document which ops need to be called with some lock set. That way driver
writers have some guidelines regarding locking. videobuf used to lock internally, but
that's now moved to the driver (and rightly so), but that does mean that the driver writer
needs pointers as to what requires locking and what doesn't.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
--
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
