Hi Pawel,
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.
> >> > + * @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 :-)
> >> > + * @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.
--
Regards,
Laurent Pinchart
--
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
