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.osciak@xxxxxxxxxxx>
> > > >
> > > > 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
