RE: [PATCH 5/7] v4l: videobuf2: add read() emulator
Hello,
On Monday, October 25, 2010 2:13 AM Pawel Osciak wrote:
> Hi Marek,
> This is a pretty crafty patch, you've managed to make it nice and
> clean, without adding complexity to streaming code. Nice job. A few of
> my comments below.
Thanks!
> On Tue, Oct 19, 2010 at 23:41, Marek Szyprowski
> wrote:
> > Add a generic read() emulator for videobuf2. It uses MMAP memory type
> > buffers and generic vb2 calls: req_bufs, qbuf and dqbuf. Video date is
> > being copied from mmap buffers to userspace with standard copy_to_user()
> > function. To add read() support to the driver, only one additional
> > structure should be provides which defines the default number of buffers
> > used by emulator and detemines the style of read() emulation
> > ('streaming' or 'one shot').
> >
> > Signed-off-by: Marek Szyprowski
> > Signed-off-by: Kyungmin Park
> > CC: Pawel Osciak
> > ---
> > drivers/media/video/videobuf2-core.c | 322
> > --
> > include/media/videobuf2-core.h | 21 +++
> > 2 files changed, 325 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/video/videobuf2-core.c
> > b/drivers/media/video/videobuf2-core.c
> > index 4a29c49..ab00246 100644
> > --- a/drivers/media/video/videobuf2-core.c
> > +++ b/drivers/media/video/videobuf2-core.c
> > @@ -471,7 +471,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
> > * The return values from this function are intended to be directly returned
> > * from vidioc_reqbufs handler in driver.
> > */
> > -int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> > +static int __vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers
> > *req)
> > {
> > unsigned int num_buffers, num_planes;
> > int ret = 0;
> > @@ -482,8 +482,6 @@ int vb2_reqbufs(struct vb2_queue *q, struct
> > v4l2_requestbuffers *req)
> > return -EINVAL;
> > }
> >
> > - mutex_lock(&q->vb_lock);
> > -
> > if (req->type != q->type) {
> > dprintk(1, "reqbufs: queue type invalid\n");
> > ret = -EINVAL;
> > @@ -567,6 +565,15 @@ end:
> > mutex_unlock(&q->vb_lock);
> > return ret;
> > }
> > +
> > +int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> > +{
> > + int ret = 0;
> > + mutex_lock(&q->vb_lock);
> > + ret = (q->read_data) ? -EBUSY : __vb2_reqbufs(q, req);
> > + mutex_unlock(&q->vb_lock);
> > + return ret;
> > +}
> > EXPORT_SYMBOL_GPL(vb2_reqbufs);
> >
> > /**
> > @@ -821,14 +828,11 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> > * The return values from this function are intended to be directly returned
> > * from vidioc_qbuf handler in driver.
> > */
> > -int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> > +static int __vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> > {
> > struct vb2_buffer *vb;
> > - int ret;
> > -
> > - mutex_lock(&q->vb_lock);
> > + int ret = -EINVAL;
> >
> > - ret = -EINVAL;
> > if (b->type != q->type) {
> > dprintk(1, "qbuf: invalid buffer type\n");
> > goto done;
> > @@ -887,6 +891,14 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer
> > *b)
> > dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index);
> > ret = 0;
> > done:
> > + return ret;
> > +}
> > +
> > +int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> > +{
> > + int ret = 0;
> > + mutex_lock(&q->vb_lock);
> > + ret = (q->read_data) ? -EBUSY : __vb2_qbuf(q, b);
> > mutex_unlock(&q->vb_lock);
> > return ret;
> > }
> > @@ -996,13 +1008,11 @@ end:
> > * The return values from this function are intended to be directly returned
> > * from vidioc_dqbuf handler in driver.
> > */
> > -int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
> > +static int __vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool
> > nonblocking)
> > {
> > struct vb2_buffer *vb = NULL;
> > int ret;
> >
> > - mutex_lock(&q->vb_lock);
> > -
> > if (b->type != q->type) {
> > dprintk(1, "dqbuf: invalid buffer type\n");
> > ret = -EINVAL;
> > @@ -1047,6 +1057,14 @@ int vb2_dqbuf(struct vb2_queue *q, struct
> > v4l2_buffer *b, bool nonblocking)
> > vb->state = VB2_BUF_STATE_DEQUEUED;
> >
> > done:
> > + return ret;
> > +}
> > +
> > +int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
> > +{
> > + int ret;
> > + mutex_lock(&q->vb_lock);
> > + ret = (q->read_data) ? -EBUSY : __vb2_dqbuf(q, b, nonblocking);
> > mutex_unlock(&q->vb_lock);
> > return ret;
> > }
> > @@ -1065,13 +1083,11 @@ EXPORT_SYMBOL_GPL(vb2_dqbuf);
> > * The return values from this function are intended to be directly returned
> > * from vidioc_streamon handler in the driver.
> > */
> > -int vb2_streamon(struct vb2_queue *q, enu
Re: [PATCH 5/7] v4l: videobuf2: add read() emulator
Hi Marek,
This is a pretty crafty patch, you've managed to make it nice and
clean, without adding complexity to streaming code. Nice job. A few of
my comments below.
On Tue, Oct 19, 2010 at 23:41, Marek Szyprowski
wrote:
> Add a generic read() emulator for videobuf2. It uses MMAP memory type
> buffers and generic vb2 calls: req_bufs, qbuf and dqbuf. Video date is
> being copied from mmap buffers to userspace with standard copy_to_user()
> function. To add read() support to the driver, only one additional
> structure should be provides which defines the default number of buffers
> used by emulator and detemines the style of read() emulation
> ('streaming' or 'one shot').
>
> Signed-off-by: Marek Szyprowski
> Signed-off-by: Kyungmin Park
> CC: Pawel Osciak
> ---
> drivers/media/video/videobuf2-core.c | 322
> --
> include/media/videobuf2-core.h | 21 +++
> 2 files changed, 325 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c
> index 4a29c49..ab00246 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -471,7 +471,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
> * The return values from this function are intended to be directly returned
> * from vidioc_reqbufs handler in driver.
> */
> -int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> +static int __vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers
> *req)
> {
> unsigned int num_buffers, num_planes;
> int ret = 0;
> @@ -482,8 +482,6 @@ int vb2_reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req)
> return -EINVAL;
> }
>
> - mutex_lock(&q->vb_lock);
> -
> if (req->type != q->type) {
> dprintk(1, "reqbufs: queue type invalid\n");
> ret = -EINVAL;
> @@ -567,6 +565,15 @@ end:
> mutex_unlock(&q->vb_lock);
> return ret;
> }
> +
> +int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> +{
> + int ret = 0;
> + mutex_lock(&q->vb_lock);
> + ret = (q->read_data) ? -EBUSY : __vb2_reqbufs(q, req);
> + mutex_unlock(&q->vb_lock);
> + return ret;
> +}
> EXPORT_SYMBOL_GPL(vb2_reqbufs);
>
> /**
> @@ -821,14 +828,11 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> * The return values from this function are intended to be directly returned
> * from vidioc_qbuf handler in driver.
> */
> -int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> +static int __vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> {
> struct vb2_buffer *vb;
> - int ret;
> -
> - mutex_lock(&q->vb_lock);
> + int ret = -EINVAL;
>
> - ret = -EINVAL;
> if (b->type != q->type) {
> dprintk(1, "qbuf: invalid buffer type\n");
> goto done;
> @@ -887,6 +891,14 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index);
> ret = 0;
> done:
> + return ret;
> +}
> +
> +int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> +{
> + int ret = 0;
> + mutex_lock(&q->vb_lock);
> + ret = (q->read_data) ? -EBUSY : __vb2_qbuf(q, b);
> mutex_unlock(&q->vb_lock);
> return ret;
> }
> @@ -996,13 +1008,11 @@ end:
> * The return values from this function are intended to be directly returned
> * from vidioc_dqbuf handler in driver.
> */
> -int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
> +static int __vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool
> nonblocking)
> {
> struct vb2_buffer *vb = NULL;
> int ret;
>
> - mutex_lock(&q->vb_lock);
> -
> if (b->type != q->type) {
> dprintk(1, "dqbuf: invalid buffer type\n");
> ret = -EINVAL;
> @@ -1047,6 +1057,14 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer
> *b, bool nonblocking)
> vb->state = VB2_BUF_STATE_DEQUEUED;
>
> done:
> + return ret;
> +}
> +
> +int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
> +{
> + int ret;
> + mutex_lock(&q->vb_lock);
> + ret = (q->read_data) ? -EBUSY : __vb2_dqbuf(q, b, nonblocking);
> mutex_unlock(&q->vb_lock);
> return ret;
> }
> @@ -1065,13 +1083,11 @@ EXPORT_SYMBOL_GPL(vb2_dqbuf);
> * The return values from this function are intended to be directly returned
> * from vidioc_streamon handler in the driver.
> */
> -int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
> +static int __vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
> {
> struct vb2_buffer *vb;
> int ret = 0;
>
> - mutex_lock(&q->vb_lock);
> -
> if (type != q->type) {
> dprintk(1, "streamon: invalid stream type\n");
> ret = -EINVAL;
> @@ -1113,6 +1129,1
