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