Re: [PATCH 1/4] gspca: convert to vb2

[Hans de Goede]

Hi,

On 05/13/2018 11:32 AM, Hans Verkuil wrote:

On 05/12/2018 08:00 PM, Hans de Goede wrote:

Hi Hans,

Overall looks good, 1 comment inline.


-   if (ret == 0 && gspca_dev->sd_desc->dq_callback) {
-   mutex_lock(_dev->usb_lock);
-   gspca_dev->usb_err = 0;
-   if (gspca_dev->present)
-   gspca_dev->sd_desc->dq_callback(gspca_dev);
-   mutex_unlock(_dev->usb_lock);
-   }
+   if (!gspca_dev->sd_desc->dq_callback)
+   return;
   
-	return ret;

+   gspca_dev->usb_err = 0;
+   gspca_dev->sd_desc->dq_callback(gspca_dev);
   }



You are loosing the "if (gspca_dev->present)" check around
the dq_callback here, this may causes issues if the
buffer_finish method gets called after the device has
been unplugged.


Good catch, I've added the 'if' here.


Ok, with that change you may add my rev-by to the entire
series.

Regards,

Hans






If the vb2 code takes care that the buffer_finish method
doesn't get called then you may add my:

Reviewed-by: Hans de Goede 

To this patch.

Patch 2-4 look good to and you may add my:

Reviewed-by: Hans de Goede 

To those too.


Thanks!



Regards,

Hans


p.s.

If the v4l2-ctl + vb2 frameworks take care of not having
any driver callbacks called after disconnect, perhaps
the present flag can be removed?


I actually tried that (using the video_is_registered() function
instead), but it is used all over in gspca subdrivers, and I didn't
want to change them all. It's easier to just keep the field.

The same is true for the streaming field, for that matter. It could
be replaced by a vb2 function, but it would require lots of changes
in gspca subdrivers as well.

Regards,

Hans






-/*
- * queue a video buffer
- *
- * Attempting to queue a buffer that has already been
- * queued will return -EINVAL.
- */
-static int vidioc_qbuf(struct file *file, void *priv,
-   struct v4l2_buffer *v4l2_buf)
+static void gspca_buffer_queue(struct vb2_buffer *vb)
   {
-   struct gspca_dev *gspca_dev = video_drvdata(file);
-   struct gspca_frame *frame;
-   int i, index, ret;
-
-   gspca_dbg(gspca_dev, D_FRAM, "qbuf %d\n", v4l2_buf->index);
-
-   if (mutex_lock_interruptible(_dev->queue_lock))
-   return -ERESTARTSYS;
-
-   index = v4l2_buf->index;
-   if ((unsigned) index >= gspca_dev->nframes) {
-   gspca_dbg(gspca_dev, D_FRAM,
- "qbuf idx %d >= %d\n", index, gspca_dev->nframes);
-   ret = -EINVAL;
-   goto out;
-   }
-   if (v4l2_buf->memory != gspca_dev->memory) {
-   gspca_dbg(gspca_dev, D_FRAM, "qbuf bad memory type\n");
-   ret = -EINVAL;
-   goto out;
-   }
-
-   frame = _dev->frame[index];
-   if (frame->v4l2_buf.flags & BUF_ALL_FLAGS) {
-   gspca_dbg(gspca_dev, D_FRAM, "qbuf bad state\n");
-   ret = -EINVAL;
-   goto out;
-   }
-
-   frame->v4l2_buf.flags |= V4L2_BUF_FLAG_QUEUED;
-
-   if (frame->v4l2_buf.memory == V4L2_MEMORY_USERPTR) {
-   frame->v4l2_buf.m.userptr = v4l2_buf->m.userptr;
-   frame->v4l2_buf.length = v4l2_buf->length;
-   }
-
-   /* put the buffer in the 'queued' queue */
-   i = atomic_read(_dev->fr_q);
-   gspca_dev->fr_queue[i] = index;
-   atomic_set(_dev->fr_q, (i + 1) % GSPCA_MAX_FRAMES);
+   struct gspca_dev *gspca_dev = vb2_get_drv_priv(vb->vb2_queue);
+   struct gspca_buffer *buf = to_gspca_buffer(vb);
+   unsigned long flags;
   
-	v4l2_buf->flags |= V4L2_BUF_FLAG_QUEUED;

-   v4l2_buf->flags &= ~V4L2_BUF_FLAG_DONE;
-   ret = 0;
-out:
-   mutex_unlock(_dev->queue_lock);
-   return ret;
+   spin_lock_irqsave(_dev->qlock, flags);
+   list_add_tail(>list, _dev->buf_list);
+   spin_unlock_irqrestore(_dev->qlock, flags);
   }
   
-/*

- * allocate the resources for read()
- */
-static int read_alloc(struct gspca_dev *gspca_dev,
-   struct file *file)
+static void gspca_return_all_buffers(struct gspca_dev *gspca_dev,
+enum vb2_buffer_state state)
   {
-   struct v4l2_buffer v4l2_buf;
-   int i, ret;
-
-   gspca_dbg(gspca_dev, D_STREAM, "read alloc\n");
+   struct gspca_buffer *buf, *node;
+   unsigned long flags;
   
-	if (mutex_lock_interruptible(_dev->usb_lock))

-   return -ERESTARTSYS;
-
-   if (gspca_dev->nframes == 0) {
-   struct v4l2_requestbuffers rb;
-
-   memset(, 0, sizeof rb);
-   rb.count = gspca_dev->nbufread;
-   rb.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-   rb.memory = GSPCA_MEMORY_READ;
-   ret = vidioc_reqbufs(file, gspca_dev, );
-   if (ret != 0) {
-   gspca_dbg(gspca_dev, D_STREAM, "read reqbuf err %d\n",

Re: [PATCH 1/4] gspca: convert to vb2

[Hans Verkuil]

On 05/12/2018 08:00 PM, Hans de Goede wrote:
> Hi Hans,
> 
> Overall looks good, 1 comment inline.
> 
>> -if (ret == 0 && gspca_dev->sd_desc->dq_callback) {
>> -mutex_lock(_dev->usb_lock);
>> -gspca_dev->usb_err = 0;
>> -if (gspca_dev->present)
>> -gspca_dev->sd_desc->dq_callback(gspca_dev);
>> -mutex_unlock(_dev->usb_lock);
>> -}
>> +if (!gspca_dev->sd_desc->dq_callback)
>> +return;
>>   
>> -return ret;
>> +gspca_dev->usb_err = 0;
>> +gspca_dev->sd_desc->dq_callback(gspca_dev);
>>   }
> 
> 
> You are loosing the "if (gspca_dev->present)" check around
> the dq_callback here, this may causes issues if the
> buffer_finish method gets called after the device has
> been unplugged.

Good catch, I've added the 'if' here.

> 
> If the vb2 code takes care that the buffer_finish method
> doesn't get called then you may add my:
> 
> Reviewed-by: Hans de Goede 
> 
> To this patch.
> 
> Patch 2-4 look good to and you may add my:
> 
> Reviewed-by: Hans de Goede 
> 
> To those too.

Thanks!

> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> If the v4l2-ctl + vb2 frameworks take care of not having
> any driver callbacks called after disconnect, perhaps
> the present flag can be removed?

I actually tried that (using the video_is_registered() function
instead), but it is used all over in gspca subdrivers, and I didn't
want to change them all. It's easier to just keep the field.

The same is true for the streaming field, for that matter. It could
be replaced by a vb2 function, but it would require lots of changes
in gspca subdrivers as well.

Regards,

Hans

> 
> 
> 
>> -/*
>> - * queue a video buffer
>> - *
>> - * Attempting to queue a buffer that has already been
>> - * queued will return -EINVAL.
>> - */
>> -static int vidioc_qbuf(struct file *file, void *priv,
>> -struct v4l2_buffer *v4l2_buf)
>> +static void gspca_buffer_queue(struct vb2_buffer *vb)
>>   {
>> -struct gspca_dev *gspca_dev = video_drvdata(file);
>> -struct gspca_frame *frame;
>> -int i, index, ret;
>> -
>> -gspca_dbg(gspca_dev, D_FRAM, "qbuf %d\n", v4l2_buf->index);
>> -
>> -if (mutex_lock_interruptible(_dev->queue_lock))
>> -return -ERESTARTSYS;
>> -
>> -index = v4l2_buf->index;
>> -if ((unsigned) index >= gspca_dev->nframes) {
>> -gspca_dbg(gspca_dev, D_FRAM,
>> -  "qbuf idx %d >= %d\n", index, gspca_dev->nframes);
>> -ret = -EINVAL;
>> -goto out;
>> -}
>> -if (v4l2_buf->memory != gspca_dev->memory) {
>> -gspca_dbg(gspca_dev, D_FRAM, "qbuf bad memory type\n");
>> -ret = -EINVAL;
>> -goto out;
>> -}
>> -
>> -frame = _dev->frame[index];
>> -if (frame->v4l2_buf.flags & BUF_ALL_FLAGS) {
>> -gspca_dbg(gspca_dev, D_FRAM, "qbuf bad state\n");
>> -ret = -EINVAL;
>> -goto out;
>> -}
>> -
>> -frame->v4l2_buf.flags |= V4L2_BUF_FLAG_QUEUED;
>> -
>> -if (frame->v4l2_buf.memory == V4L2_MEMORY_USERPTR) {
>> -frame->v4l2_buf.m.userptr = v4l2_buf->m.userptr;
>> -frame->v4l2_buf.length = v4l2_buf->length;
>> -}
>> -
>> -/* put the buffer in the 'queued' queue */
>> -i = atomic_read(_dev->fr_q);
>> -gspca_dev->fr_queue[i] = index;
>> -atomic_set(_dev->fr_q, (i + 1) % GSPCA_MAX_FRAMES);
>> +struct gspca_dev *gspca_dev = vb2_get_drv_priv(vb->vb2_queue);
>> +struct gspca_buffer *buf = to_gspca_buffer(vb);
>> +unsigned long flags;
>>   
>> -v4l2_buf->flags |= V4L2_BUF_FLAG_QUEUED;
>> -v4l2_buf->flags &= ~V4L2_BUF_FLAG_DONE;
>> -ret = 0;
>> -out:
>> -mutex_unlock(_dev->queue_lock);
>> -return ret;
>> +spin_lock_irqsave(_dev->qlock, flags);
>> +list_add_tail(>list, _dev->buf_list);
>> +spin_unlock_irqrestore(_dev->qlock, flags);
>>   }
>>   
>> -/*
>> - * allocate the resources for read()
>> - */
>> -static int read_alloc(struct gspca_dev *gspca_dev,
>> -struct file *file)
>> +static void gspca_return_all_buffers(struct gspca_dev *gspca_dev,
>> + enum vb2_buffer_state state)
>>   {
>> -struct v4l2_buffer v4l2_buf;
>> -int i, ret;
>> -
>> -gspca_dbg(gspca_dev, D_STREAM, "read alloc\n");
>> +struct gspca_buffer *buf, *node;
>> +unsigned long flags;
>>   
>> -if (mutex_lock_interruptible(_dev->usb_lock))
>> -return -ERESTARTSYS;
>> -
>> -if (gspca_dev->nframes == 0) {
>> -struct v4l2_requestbuffers rb;
>> -
>> -memset(, 0, sizeof rb);
>> -rb.count = gspca_dev->nbufread;
>> -rb.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> -rb.memory = GSPCA_MEMORY_READ;
>> -ret = vidioc_reqbufs(file, gspca_dev, );
>> -if (ret != 0) {
>> -gspca_dbg(gspca_dev, 

Re: [PATCH 1/4] gspca: convert to vb2

[Hans de Goede]

Hi Hans,

Overall looks good, 1 comment inline.

On 12-05-18 16:44, Hans Verkuil wrote:

From: Hans Verkuil 

Signed-off-by: Hans Verkuil 
---
  drivers/media/usb/gspca/Kconfig|   1 +
  drivers/media/usb/gspca/gspca.c| 899 -
  drivers/media/usb/gspca/gspca.h|  38 +-
  drivers/media/usb/gspca/m5602/m5602_core.c |   4 +-
  drivers/media/usb/gspca/vc032x.c   |   2 +-
  5 files changed, 179 insertions(+), 765 deletions(-)

diff --git a/drivers/media/usb/gspca/Kconfig b/drivers/media/usb/gspca/Kconfig
index bc9a439745aa..d3b6665c342d 100644
--- a/drivers/media/usb/gspca/Kconfig
+++ b/drivers/media/usb/gspca/Kconfig
@@ -2,6 +2,7 @@ menuconfig USB_GSPCA
tristate "GSPCA based webcams"
depends on VIDEO_V4L2
depends on INPUT || INPUT=n
+   select VIDEOBUF2_VMALLOC
default m
---help---
  Say Y here if you want to enable selecting webcams based
diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index d29773b8f696..ca87c58359e0 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -82,32 +82,6 @@ static void PDEBUG_MODE(struct gspca_dev *gspca_dev, int 
debug, char *txt,
  #define GSPCA_MEMORY_NO 0 /* V4L2_MEMORY_xxx starts from 1 */
  #define GSPCA_MEMORY_READ 7
  
-#define BUF_ALL_FLAGS (V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE)

-
-/*
- * VMA operations.
- */
-static void gspca_vm_open(struct vm_area_struct *vma)
-{
-   struct gspca_frame *frame = vma->vm_private_data;
-
-   frame->vma_use_count++;
-   frame->v4l2_buf.flags |= V4L2_BUF_FLAG_MAPPED;
-}
-
-static void gspca_vm_close(struct vm_area_struct *vma)
-{
-   struct gspca_frame *frame = vma->vm_private_data;
-
-   if (--frame->vma_use_count <= 0)
-   frame->v4l2_buf.flags &= ~V4L2_BUF_FLAG_MAPPED;
-}
-
-static const struct vm_operations_struct gspca_vm_ops = {
-   .open   = gspca_vm_open,
-   .close  = gspca_vm_close,
-};
-
  /*
   * Input and interrupt endpoint handling functions
   */
@@ -356,7 +330,7 @@ static void isoc_irq(struct urb *urb)
struct gspca_dev *gspca_dev = (struct gspca_dev *) urb->context;
  
  	gspca_dbg(gspca_dev, D_PACK, "isoc irq\n");

-   if (!gspca_dev->streaming)
+   if (!vb2_start_streaming_called(_dev->queue))
return;
fill_frame(gspca_dev, urb);
  }
@@ -370,7 +344,7 @@ static void bulk_irq(struct urb *urb)
int st;
  
  	gspca_dbg(gspca_dev, D_PACK, "bulk irq\n");

-   if (!gspca_dev->streaming)
+   if (!vb2_start_streaming_called(_dev->queue))
return;
switch (urb->status) {
case 0:
@@ -417,25 +391,23 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
const u8 *data,
int len)
  {
-   struct gspca_frame *frame;
-   int i, j;
+   struct gspca_buffer *buf;
  
  	gspca_dbg(gspca_dev, D_PACK, "add t:%d l:%d\n",	packet_type, len);
  
-	if (packet_type == FIRST_PACKET) {

-   i = atomic_read(_dev->fr_i);
+   spin_lock(_dev->qlock);
+   buf = list_first_entry_or_null(_dev->buf_list,
+  typeof(*buf), list);
+   spin_unlock(_dev->qlock);
  
-		/* if there are no queued buffer, discard the whole frame */

-   if (i == atomic_read(_dev->fr_q)) {
+   if (packet_type == FIRST_PACKET) {
+   /* if there is no queued buffer, discard the whole frame */
+   if (!buf) {
gspca_dev->last_packet_type = DISCARD_PACKET;
gspca_dev->sequence++;
return;
}
-   j = gspca_dev->fr_queue[i];
-   frame = _dev->frame[j];
-   v4l2_get_timestamp(>v4l2_buf.timestamp);
-   frame->v4l2_buf.sequence = gspca_dev->sequence++;
-   gspca_dev->image = frame->data;
+   gspca_dev->image = vb2_plane_vaddr(>vb.vb2_buf, 0);
gspca_dev->image_len = 0;
} else {
switch (gspca_dev->last_packet_type) {
@@ -453,10 +425,10 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
  
  	/* append the packet to the frame buffer */

if (len > 0) {
-   if (gspca_dev->image_len + len > gspca_dev->frsz) {
+   if (gspca_dev->image_len + len > gspca_dev->pixfmt.sizeimage) {
gspca_err(gspca_dev, "frame overflow %d > %d\n",
  gspca_dev->image_len + len,
- gspca_dev->frsz);
+ gspca_dev->pixfmt.sizeimage);
packet_type = DISCARD_PACKET;
} else {
  /* !! image is NULL only when last pkt is LAST or DISCARD
@@ -476,80 +448,23 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
 * next first 

[PATCH 1/4] gspca: convert to vb2

[Hans Verkuil]

From: Hans Verkuil 

Signed-off-by: Hans Verkuil 
---
 drivers/media/usb/gspca/Kconfig|   1 +
 drivers/media/usb/gspca/gspca.c| 899 -
 drivers/media/usb/gspca/gspca.h|  38 +-
 drivers/media/usb/gspca/m5602/m5602_core.c |   4 +-
 drivers/media/usb/gspca/vc032x.c   |   2 +-
 5 files changed, 179 insertions(+), 765 deletions(-)

diff --git a/drivers/media/usb/gspca/Kconfig b/drivers/media/usb/gspca/Kconfig
index bc9a439745aa..d3b6665c342d 100644
--- a/drivers/media/usb/gspca/Kconfig
+++ b/drivers/media/usb/gspca/Kconfig
@@ -2,6 +2,7 @@ menuconfig USB_GSPCA
tristate "GSPCA based webcams"
depends on VIDEO_V4L2
depends on INPUT || INPUT=n
+   select VIDEOBUF2_VMALLOC
default m
---help---
  Say Y here if you want to enable selecting webcams based
diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index d29773b8f696..ca87c58359e0 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -82,32 +82,6 @@ static void PDEBUG_MODE(struct gspca_dev *gspca_dev, int 
debug, char *txt,
 #define GSPCA_MEMORY_NO 0  /* V4L2_MEMORY_xxx starts from 1 */
 #define GSPCA_MEMORY_READ 7
 
-#define BUF_ALL_FLAGS (V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE)
-
-/*
- * VMA operations.
- */
-static void gspca_vm_open(struct vm_area_struct *vma)
-{
-   struct gspca_frame *frame = vma->vm_private_data;
-
-   frame->vma_use_count++;
-   frame->v4l2_buf.flags |= V4L2_BUF_FLAG_MAPPED;
-}
-
-static void gspca_vm_close(struct vm_area_struct *vma)
-{
-   struct gspca_frame *frame = vma->vm_private_data;
-
-   if (--frame->vma_use_count <= 0)
-   frame->v4l2_buf.flags &= ~V4L2_BUF_FLAG_MAPPED;
-}
-
-static const struct vm_operations_struct gspca_vm_ops = {
-   .open   = gspca_vm_open,
-   .close  = gspca_vm_close,
-};
-
 /*
  * Input and interrupt endpoint handling functions
  */
@@ -356,7 +330,7 @@ static void isoc_irq(struct urb *urb)
struct gspca_dev *gspca_dev = (struct gspca_dev *) urb->context;
 
gspca_dbg(gspca_dev, D_PACK, "isoc irq\n");
-   if (!gspca_dev->streaming)
+   if (!vb2_start_streaming_called(_dev->queue))
return;
fill_frame(gspca_dev, urb);
 }
@@ -370,7 +344,7 @@ static void bulk_irq(struct urb *urb)
int st;
 
gspca_dbg(gspca_dev, D_PACK, "bulk irq\n");
-   if (!gspca_dev->streaming)
+   if (!vb2_start_streaming_called(_dev->queue))
return;
switch (urb->status) {
case 0:
@@ -417,25 +391,23 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
const u8 *data,
int len)
 {
-   struct gspca_frame *frame;
-   int i, j;
+   struct gspca_buffer *buf;
 
gspca_dbg(gspca_dev, D_PACK, "add t:%d l:%d\n", packet_type, len);
 
-   if (packet_type == FIRST_PACKET) {
-   i = atomic_read(_dev->fr_i);
+   spin_lock(_dev->qlock);
+   buf = list_first_entry_or_null(_dev->buf_list,
+  typeof(*buf), list);
+   spin_unlock(_dev->qlock);
 
-   /* if there are no queued buffer, discard the whole frame */
-   if (i == atomic_read(_dev->fr_q)) {
+   if (packet_type == FIRST_PACKET) {
+   /* if there is no queued buffer, discard the whole frame */
+   if (!buf) {
gspca_dev->last_packet_type = DISCARD_PACKET;
gspca_dev->sequence++;
return;
}
-   j = gspca_dev->fr_queue[i];
-   frame = _dev->frame[j];
-   v4l2_get_timestamp(>v4l2_buf.timestamp);
-   frame->v4l2_buf.sequence = gspca_dev->sequence++;
-   gspca_dev->image = frame->data;
+   gspca_dev->image = vb2_plane_vaddr(>vb.vb2_buf, 0);
gspca_dev->image_len = 0;
} else {
switch (gspca_dev->last_packet_type) {
@@ -453,10 +425,10 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
 
/* append the packet to the frame buffer */
if (len > 0) {
-   if (gspca_dev->image_len + len > gspca_dev->frsz) {
+   if (gspca_dev->image_len + len > gspca_dev->pixfmt.sizeimage) {
gspca_err(gspca_dev, "frame overflow %d > %d\n",
  gspca_dev->image_len + len,
- gspca_dev->frsz);
+ gspca_dev->pixfmt.sizeimage);
packet_type = DISCARD_PACKET;
} else {
 /* !! image is NULL only when last pkt is LAST or DISCARD
@@ -476,80 +448,23 @@ void gspca_frame_add(struct gspca_dev *gspca_dev,
 * next first packet, wake up the application and advance
 * in the queue */