Hello,
On Friday, November 26, 2010 7:00 PM Laurent Pinchart wrote:
> On Friday 26 November 2010 17:49:02 Pawel Osciak wrote:
> > On Thu, Nov 25, 2010 at 01:48, Marek Szyprowski wrote:
> > > Hello,
> > >
> > > On Thursday, November 25, 2010 2:17 AM Laurent Pinchart wrote:
> > >> On Friday 19 November 2010 16:55:38 Marek Szyprowski wrote:
> > >> > From: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> > >> >
> > >> > Videobuf2 is a Video for Linux 2 API-compatible driver framework for
>
> [snip]
>
> > >> > +/**
> > >> > + * vb2_mmap() - map video buffers into application address space
> > >> > + * @q: videobuf2 queue
> > >> > + * @vma: vma passed to the mmap file operation handler in the
> > >> > driver + *
> > >> > + * Should be called from mmap file operation handler of a driver.
> > >> > + * This function maps one plane of one of the available video buffers
> > >> > to + * userspace. To map whole video memory allocated on reqbufs,
> > >> > this function + * has to be called once per each plane per each
> > >> > buffer previously allocated. + *
> > >> > + * When the userspace application calls mmap, it passes to it an
> > >> > offset returned + * to it earlier by the means of vidioc_querybuf
> > >> > handler. That offset acts as + * a "cookie", which is then used to
> > >> > identify the plane to be mapped. + * This function finds a plane with
> > >> > a matching offset and a mapping is performed + * by the means of a
> > >> > provided memory operation. + *
> > >> > + * The return values from this function are intended to be directly
> > >> > returned + * from the mmap handler in driver.
> > >> > + */
> > >> > +int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
> > >> > +{
> > >> > + unsigned long off = vma->vm_pgoff << PAGE_SHIFT;
> > >> > + struct vb2_plane *vb_plane;
> > >> > + struct vb2_buffer *vb;
> > >> > + unsigned int buffer, plane;
> > >
> > > <snip>
> > >
> > >> > +
> > >> > + vb = q->bufs[buffer];
> > >> > + vb_plane = &vb->planes[plane];
> > >> > +
> > >> > + if (vb_plane->mapped) {
> > >> > + dprintk(1, "Plane already mapped\n");
> > >>
> > >> What is the reason to disallow buffers from being mapped several times ?
> > >> If there's a valid one, it would be worth adding it in a comment here.
> > >
> > > I see no reason, maybe Pawel will explain it a bit more. IMHO this check
> > > can be removed.
> >
> > First thing, I think we talked about that on IRC with Hans and agreed
> > that we don't really need to map a buffer multiple times from one
> > process. This is also carried over from vb1 as far as I can remember.
> > I think it was there because vb1 used to allocate memory on mmap, to
> > prevent multiple allocations. This check could be removed, but do we
> > really need such a feature?
>
> What if a process calls fork() or pthread_create() with an existing mapping ?
> I seem to remember that the mmap operation handler can be called again.
It was probably handled this way in some older kernels. Currently a vm_area is
just copied and a vm_open() op is called if provided.
I agree however that there is no real reason to limit number of mmap calls.
> > >> > + * @buf_alloc: called to allocate a struct vb2_buffer;
> > >> > driver usually + * embeds it in its own custom buffer
> > >> > structure; returns + * a pointer to vb2_buffer or
> > >> > NULL if failed; if not + * provided
> > >> > kmalloc(sizeof(struct vb2_buffer, GFP_KERNEL) + * is
> > >> > used
> > >> > + * @buf_free: called to free the structure allocated by
> > >> > @buf_alloc; + * if not provided kfree(vb) is used
> > >>
> > >> I'm curious, do we have use cases for buf_alloc != kmalloc and buf_free
> > >> != kfree ?
> > >
> > > Well, the problem is not which function to call, but the size that is
> > > passed as the argument. Drivers usually wants to embed the struct vb2
> > > inside its own 'buffer' structure. See vivi driver ported to vb2 for the
> > > reference. The driver get a pointer to vb2 and the uses containerof() to
> > > get his own buffer. To make it possible the buffer need to be allocated
> > > by the driver not the vb2. Currently I found no other way of solving
> > > this than such callbacks.
> >
> > I really didn't like how the concept of embedding structures
> > complicated both vb1 and is now complicating vb2. Maybe we should
> > think about going back to driver private structures and fixed-size
> > videobuf buffers?
>
> I'm open to discussions on this :-)
2:1, as I will probably get back to the way it was handled in videobuf1 (with the
requirement that struct vb2 must be a first element of driver's custom buffer).
> > >> > + * @buf_init: called once after allocating a buffer (in
> > >> > MMAP case) + * or after acquiring a new USERPTR
> > >> > buffer; drivers may + * perform additional
> > >> > buffer-related initialization; + * initialization
> > >> > failure (return != 0) will prevent + * queue setup
> > >> > from completing successfully; optional + * @buf_prepare: called
> > >> > every time the buffer is queued from userspace; + *
> > >> > drivers may perform any initialization required before + *
> > >> > each hardware operation in this callback; + *
> > >> > if an error is returned, the buffer will not be queued + *
> > >> > in driver; optional
> > >> > + * @buf_finish: called before every dequeue of the buffer
> > >> > back to + * userspace; drivers may perform any
> > >> > operations required + * before userspace accesses the
> > >> > buffer; optional + * @buf_cleanup: called once before the buffer is
> > >> > freed; drivers may + * perform any additional
> > >> > cleanup; optional
> > >> > + * @start_streaming: called once before entering 'streaming'
> > >> > state; enables + * driver to recieve buffers over
> > >> > buf_queue() callback + * @stop_streaming: called when
> > >> > 'streaming' state must be disabled; driver + * should
> > >> > stop any dma transactions or wait until they + *
> > >> > finish and give back all buffers it got from buf_queue() + *
> > >> > callback
> > >>
> > >> start_streaming is only called in response to vb2_streamon, and
> > >> stop_streaming in response to vb2_streamoff or vb2_release (implicit
> > >> streamoff). I think it would be better to require the driver to stop
> > >> streaming before calling vb2_streamoff and vb2_release, and have the
> > >> driver start streaming only when vb2_streamon returns. I don't really
> > >> like vb2 trying to manage the driver's streaming state. Those two
> > >> operations could then be removed.
> > >
> > > Hardware streaming starts not only from STREAMON ioctl, but also from
> > > read/write and poll in case of file io access types. My idea was to make
> > > all vb2 functions to be simple callback for the respective ioctls or
> > > file ops, and move all the logic that need to be implemented in the
> > > driver to the queue callbacks. Such separation make the driver much
> > > easier to understand and avoids some common mistakes like forgetting to
> > > 'start streaming' in the poll implementation or so.
> >
> > I agree with Marek, I remember the problems with starting streaming in
> > OMAP hardware, but believe the simplest case should be the easiest to
> > implement. Unless this prevents OMAP drivers from using vb2, I thnik
> > it should stay as is. Would this cause trouble for OMAP drivers?
>
> I'd have to double-check that, but even if it doesn't, I don't like VB2 trying
> to handle hardware streaming. VB2 should only handle buffers, buffer queues
> and allocators.
One of the reasons for creating vb2 is to have a clean interface. Like I said
hardware streaming starts from 3 different places (and at least one of them is
easily forgotten in the drivers).
If you want to do it manually from a driver - go ahead. I hope most of a drivers
will benefit from a single point of a hardware streaming control.
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
