Hello,
On Thursday, November 25, 2010 12:23 PM Hans Verkuil wrote:
> 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.
After more thoughts I've decided to remove it in the next version.
> > > > + * @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 :-)
These 2 names sounds good. I will use them in the next version.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
