On Thursday, November 25, 2010 10:48:39 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.osc...@samsung.com>
> > >
> > > Videobuf2 is a Video for Linux 2 API-compatible driver framework for
> > > multimedia devices. It acts as an intermediate layer between userspace
> > > applications and device drivers. It also provides low-level, modular
> > > memory management functions for drivers.
> > >
> > > Videobuf2 eases driver development, reduces drivers' code size and aids in
> > > proper and consistent implementation of V4L2 API in drivers.
> > >
> > > Videobuf2 memory management backend is fully modular. This allows custom
> > > memory management routines for devices and platforms with non-standard
> > > memory management requirements to be plugged in, without changing the
> > > high-level buffer management functions and API.
> > >
> > > The framework provides:
> > > - implementations of streaming I/O V4L2 ioctls and file operations
> > > - high-level video buffer, video queue and state management functions
> > > - video buffer memory allocation and management
> >
> > Excellent job. I'm really pleased by the outstanding code quality.
> > Nevertheless I got a bunch of comments (I wonder if I'm known as an annoying
> > nit-picking reviewer in the community :-)).
> >
> > I've reviewed the patch from bottom to top (starting at the header file), so
> > comments at the bottom can also apply to functions at the top when I got
> > tired
> > repeating the same information. Sorry about that weird order.
> >
> > Feel free to disagree with my comments, I've probably been overzealous
> > during
> > the review to make sure everything I had a doubt about would be properly
> > addressed.
> >
> > It's now past 2AM and I don't have enough courage to review the other
> > patches.
> > I'm afraid they will have to wait until tomorrow. If they're of the same
> > quality as this one I'm sure they will be good.
>
> Thanks for your hard work! I can imagine how much time you had to spend on
> catching
> all these things.
>
> > BTW, where's the patch for Documentation/video4linux ? :-)
>
> Well, I hope it will be ready one day ;) What kind of documentation do you
> expect
> there? Vb2 structures and functions are already documented directly in the
> source.
>
> > One last comment, why was the decision to remove all locking from vb2 taken
> > ?
>
> The idea came from Hans after posting version 1 and I really like it. It
> simplifies
> a lot of things in the drivers. The original idea of irq_spinlock and vb_lock
> was
> horrible. Having more than one queue in the driver meant that you need to
> make a
> lot of nasty hacks to ensure proper locking, because in some cases you had to
> take
> locks from all queues. The irq_lock usage was even worse. Videobuf used it to
> silently mess around the buffers that have been already queued to the
> hardware.
> Now, after Hans changes to v4l2 core (BKL removal in favor of simple mutex)
> almost
> all this mess can be simply removed. Proper locking can be easily ensured by
> the
> new mutex in the v4l2 core. There is no need to have any locks inside vb2. By
> removing irq_lock the design of the drivers is easier to understand, because
> there
> is no implicit actions on buffers that are processed by the hardware.
The BKL removal stuff has nothing to do with this. The main reason is just to
simplify
locking in drivers. Things get really complex if you have multiple nested locks.
Moving locking out of vb2 and into the driver gives the control back to the
driver.
> > > +/**
> > > + * vb2_has_consumers() - return true if the userspace is waiting for a
> > > buffer + * @q: videobuf2 queue
> > > + *
> > > + * This function returns true if a userspace application is waiting for a
> > > buffer + * to be ready to dequeue (on which a hardware operation has been
> > > finished). + */
> > > +bool vb2_has_consumers(struct vb2_queue *q)
> > > +{
> > > + return waitqueue_active(&q->done_wq);
> > > +}
> > > +EXPORT_SYMBOL_GPL(vb2_has_consumers);
> >
> > What is this for ? Do you have use cases in mind ?
>
> The vivi driver is using it, but frankly it should be redesigned to use some
> other check.
It is a bit of an odd function. I'm also not sure how useful it is.
> > > + * @plane_setup: called before memory allocation num_planes times;
> > > + * driver should return the required size of plane
> > > number
> > > + * plane_no
> > > + * @unlock: release any locks taken while calling vb2
> > > functions;
> > > + * it is called before poll_wait function in
> > > vb2_poll
> > > + * implementation; required to avoid deadlock when
> > > vb2_poll
> > > + * function waits for a buffer
> > > + * @lock: reacquire all locks released in the previous
> > > callback;
> > > + * required to continue operation after sleeping in
> > > + * poll_wait function
> >
> > Those names were not very clear to me at first sight. What about renaming
> > those two operations poll_prepare and poll_finish (or similar) ? Feel free
> > to
> > disagree here, I'm not sure what I would prefer, but I thought I would throw
> > the idea in.
>
> I see your point here but I'm not sure it will make the code easier to
> understand.
> Hans - could you comment on this?
I think I agree with Laurent, although I think I would prefer to have it called
wait_prepare and wait_finish. Better alternatives are welcome :-)
> > > + * @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
Typo: recieve -> receive
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
