Hello,
On Tuesday, December 14, 2010 8:58 AM Hans Verkuil wrote:
> On Tuesday, December 14, 2010 08:14:32 Marek Szyprowski wrote:
> > Hello,
> >
> > On Saturday, December 11, 2010 5:55 PM Hans Verkuil wrote:
> >
> > Big thanks for the review! I will fix all these minor issues and resend
> > the patches soon. I hope we will manage to get videobuf2 merged soon! :)
> >
> > > Hi Marek,
> > >
> > > Here is my review. I wish I could ack it, but I found a few bugs that need
> > > fixing first. Also a bunch of small stuff that's trivial to fix.
> > >
> > > On Monday, December 06, 2010 11:52: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
> > > >
> > > > Signed-off-by: Pawel Osciak <p.osc...@samsung.com>
> > > > Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> > > > Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com>
> > > > CC: Pawel Osciak <pa...@osciak.com>
> > > > ---
> >
> > <snip>
> >
> > > > +/**
> > > > + * __vb2_wait_for_done_vb() - wait for a buffer to become available
> > > > + * for dequeuing
> > > > + *
> > > > + * Will sleep if required for nonblocking == false.
> > > > + */
> > > > +static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
> > > > +{
> > > > + /*
> > > > + * All operation on vb_done_list is performed under vb_done_lock
> > > > + * spinlock protection. However buffers may must be removed from
> > > > + * it and returned to userspace only while holding both driver's
> > > > + * lock and the vb_done_lock spinlock. Thus we can be sure that
> > > > as
> > > > + * long as we hold lock, the list will remain not empty if this
> > > > + * check succeeds.
> > > > + */
> > > > +
> > > > + for (;;) {
> > > > + int ret;
> > > > +
> > > > + if (!q->streaming) {
> > > > + dprintk(1, "Streaming off, will not wait for
> > > > buffers\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (!list_empty(&q->done_list)) {
> > > > + /*
> > > > + * Found a buffer that we were waiting for.
> > > > + */
> > > > + break;
> > > > + } else if (nonblocking) {
> > >
> > > The 'else' keyword can be removed since the 'if' above always breaks.
> > >
> > > > + dprintk(1, "Nonblocking and no buffers to
> > > > dequeue, "
> > > > + "will
> > > > not wait\n");
> > > > + return -EAGAIN;
> > > > + }
> > > > +
> > > > + /*
> > > > + * We are streaming and blocking, wait for another
> > > > buffer to
> > > > + * become ready or for streamoff. Driver's lock is
> > > > released to
> > > > + * allow streamoff or qbuf to be called while waiting.
> > > > + */
> > > > + call_qop(q, wait_prepare, q);
> > > > +
> > > > + /*
> > > > + * All locks has been released, it is safe to sleep now.
> > > > + */
> > > > + dprintk(3, "Will sleep waiting for buffers\n");
> > > > + ret = wait_event_interruptible(q->done_wq,
> > > > + !list_empty(&q->done_list) ||
> > > > !q->streaming);
> > > > +
> > > > + /*
> > > > + * We need to reevaluate both conditions again after
> > > > reacquiring
> > > > + * the locks or return an error if it occured. In case
> > > > of error
> > > > + * we return -EINTR, because -ERESTARTSYS should not be
> > > > returned
> > > > + * to userspace.
> > > > + */
> > > > + call_qop(q, wait_finish, q);
> > > > + if (ret)
> > > > + return -EINTR;
> > >
> > > No, this should be -ERESTARTSYS. This won't be returned to userspace,
> > > instead
> > > the kernel will handle the signal and restart the system call
> > > automatically.
> >
> > I thought that -ERESTARTSYS should not be returned to userspace, that's why
> > I
> > use -EINTR here. What does the comment in linux/errno.h refer to?
>
> Actually, I looked at the wait_event_interruptible code and it already returns
> -ERESTARTSYS in case of a signal. So you can just do 'return ret' here.
>
> Anyway, the idea is that if a signal arrived then the driver can return
> -ERESTARTSYS
> to tell the kernel that it should handle the signal and afterwards call the
> system
> call again (restart it). The error code is never returned to userspace.
>
> So userspace should never see this error, instead the kernel will silently
> handle
> this.
Ok, I understand, thanks for clarification! I will fix this issue as well.
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
