Re: [RFCv11 PATCH 26/29] vim2m: use workqueue

[Hans Verkuil]

On 12/04/18 11:02, Tomasz Figa wrote:
> Hi Hans,
> 
> On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil  wrote:
> 
>> From: Hans Verkuil 
> 
>> v4l2_ctrl uses mutexes, so we can't setup a ctrl_handler in
>> interrupt context. Switch to a workqueue instead.
> 
> Could it make more sense to just replace the old (non-hr) timer used in
> this driver with delayed work?

I agree. I'll change that once v12 is posted to the mailing list.

Regards,

Hans

> 
> Best regards,
> Tomasz
> 

Re: [RFCv11 PATCH 26/29] vim2m: use workqueue

[Hans Verkuil]

On 11/04/18 16:06, Paul Kocialkowski wrote:
> Hi,
> 
> On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote:
>> From: Hans Verkuil 
>>
>> v4l2_ctrl uses mutexes, so we can't setup a ctrl_handler in
>> interrupt context. Switch to a workqueue instead.
> 
> See one comment below.
> 
>> Signed-off-by: Hans Verkuil 
>> ---
>>  drivers/media/platform/vim2m.c | 15 +--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/vim2m.c
>> b/drivers/media/platform/vim2m.c
>> index ef970434af13..9b18b32c255d 100644
>> --- a/drivers/media/platform/vim2m.c
>> +++ b/drivers/media/platform/vim2m.c
>> @@ -150,6 +150,7 @@ struct vim2m_dev {
>>  spinlock_t  irqlock;
>>  
>>  struct timer_list   timer;
>> +struct work_struct  work_run;
> 
> Wouldn't it make more sense to move this to vim2m_ctx instead (since
> this is heavily m2m-specific)?

The work is triggered by a timer which is m2m_dev specific. So it makes
no sense to move this to the per-filehandle vim2m_ctx IMHO.

Regards,

Hans

> 
>>  struct v4l2_m2m_dev *m2m_dev;
>>  };
>> @@ -392,9 +393,10 @@ static void device_run(void *priv)
>>  schedule_irq(dev, ctx->transtime);
>>  }
>>  
>> -static void device_isr(struct timer_list *t)
>> +static void device_work(struct work_struct *w)
>>  {
>> -struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t,
>> timer);
>> +struct vim2m_dev *vim2m_dev =
>> +container_of(w, struct vim2m_dev, work_run);
>>  struct vim2m_ctx *curr_ctx;
>>  struct vb2_v4l2_buffer *src_vb, *dst_vb;
>>  unsigned long flags;
>> @@ -426,6 +428,13 @@ static void device_isr(struct timer_list *t)
>>  }
>>  }
>>  
>> +static void device_isr(struct timer_list *t)
>> +{
>> +struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t,
>> timer);
>> +
>> +schedule_work(_dev->work_run);
>> +}
>> +
>>  /*
>>   * video ioctls
>>   */
>> @@ -806,6 +815,7 @@ static void vim2m_stop_streaming(struct vb2_queue
>> *q)
>>  struct vb2_v4l2_buffer *vbuf;
>>  unsigned long flags;
>>  
>> +flush_scheduled_work();
>>  for (;;) {
>>  if (V4L2_TYPE_IS_OUTPUT(q->type))
>>  vbuf = v4l2_m2m_src_buf_remove(ctx-
>>> fh.m2m_ctx);
>> @@ -1011,6 +1021,7 @@ static int vim2m_probe(struct platform_device
>> *pdev)
>>  vfd = >vfd;
>>  vfd->lock = >dev_mutex;
>>  vfd->v4l2_dev = >v4l2_dev;
>> +INIT_WORK(>work_run, device_work);
>>  
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>>  dev->mdev.dev = >dev;

Re: [RFCv11 PATCH 26/29] vim2m: use workqueue

[Tomasz Figa]

Hi Hans,

On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil  wrote:

> From: Hans Verkuil 

> v4l2_ctrl uses mutexes, so we can't setup a ctrl_handler in
> interrupt context. Switch to a workqueue instead.

Could it make more sense to just replace the old (non-hr) timer used in
this driver with delayed work?

Best regards,
Tomasz

Re: [RFCv11 PATCH 26/29] vim2m: use workqueue

[Paul Kocialkowski]

Hi,

On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> v4l2_ctrl uses mutexes, so we can't setup a ctrl_handler in
> interrupt context. Switch to a workqueue instead.

See one comment below.

> Signed-off-by: Hans Verkuil 
> ---
>  drivers/media/platform/vim2m.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vim2m.c
> b/drivers/media/platform/vim2m.c
> index ef970434af13..9b18b32c255d 100644
> --- a/drivers/media/platform/vim2m.c
> +++ b/drivers/media/platform/vim2m.c
> @@ -150,6 +150,7 @@ struct vim2m_dev {
>   spinlock_t  irqlock;
>  
>   struct timer_list   timer;
> + struct work_struct  work_run;

Wouldn't it make more sense to move this to vim2m_ctx instead (since
this is heavily m2m-specific)?

>   struct v4l2_m2m_dev *m2m_dev;
>  };
> @@ -392,9 +393,10 @@ static void device_run(void *priv)
>   schedule_irq(dev, ctx->transtime);
>  }
>  
> -static void device_isr(struct timer_list *t)
> +static void device_work(struct work_struct *w)
>  {
> - struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t,
> timer);
> + struct vim2m_dev *vim2m_dev =
> + container_of(w, struct vim2m_dev, work_run);
>   struct vim2m_ctx *curr_ctx;
>   struct vb2_v4l2_buffer *src_vb, *dst_vb;
>   unsigned long flags;
> @@ -426,6 +428,13 @@ static void device_isr(struct timer_list *t)
>   }
>  }
>  
> +static void device_isr(struct timer_list *t)
> +{
> + struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t,
> timer);
> +
> + schedule_work(_dev->work_run);
> +}
> +
>  /*
>   * video ioctls
>   */
> @@ -806,6 +815,7 @@ static void vim2m_stop_streaming(struct vb2_queue
> *q)
>   struct vb2_v4l2_buffer *vbuf;
>   unsigned long flags;
>  
> + flush_scheduled_work();
>   for (;;) {
>   if (V4L2_TYPE_IS_OUTPUT(q->type))
>   vbuf = v4l2_m2m_src_buf_remove(ctx-
> >fh.m2m_ctx);
> @@ -1011,6 +1021,7 @@ static int vim2m_probe(struct platform_device
> *pdev)
>   vfd = >vfd;
>   vfd->lock = >dev_mutex;
>   vfd->v4l2_dev = >v4l2_dev;
> + INIT_WORK(>work_run, device_work);
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
>   dev->mdev.dev = >dev;
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

signature.asc
Description: This is a digitally signed message part

[RFCv11 PATCH 26/29] vim2m: use workqueue

[Hans Verkuil]

From: Hans Verkuil 

v4l2_ctrl uses mutexes, so we can't setup a ctrl_handler in
interrupt context. Switch to a workqueue instead.

Signed-off-by: Hans Verkuil 
---
 drivers/media/platform/vim2m.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index ef970434af13..9b18b32c255d 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -150,6 +150,7 @@ struct vim2m_dev {
spinlock_t  irqlock;
 
struct timer_list   timer;
+   struct work_struct  work_run;
 
struct v4l2_m2m_dev *m2m_dev;
 };
@@ -392,9 +393,10 @@ static void device_run(void *priv)
schedule_irq(dev, ctx->transtime);
 }
 
-static void device_isr(struct timer_list *t)
+static void device_work(struct work_struct *w)
 {
-   struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t, timer);
+   struct vim2m_dev *vim2m_dev =
+   container_of(w, struct vim2m_dev, work_run);
struct vim2m_ctx *curr_ctx;
struct vb2_v4l2_buffer *src_vb, *dst_vb;
unsigned long flags;
@@ -426,6 +428,13 @@ static void device_isr(struct timer_list *t)
}
 }
 
+static void device_isr(struct timer_list *t)
+{
+   struct vim2m_dev *vim2m_dev = from_timer(vim2m_dev, t, timer);
+
+   schedule_work(_dev->work_run);
+}
+
 /*
  * video ioctls
  */
@@ -806,6 +815,7 @@ static void vim2m_stop_streaming(struct vb2_queue *q)
struct vb2_v4l2_buffer *vbuf;
unsigned long flags;
 
+   flush_scheduled_work();
for (;;) {
if (V4L2_TYPE_IS_OUTPUT(q->type))
vbuf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
@@ -1011,6 +1021,7 @@ static int vim2m_probe(struct platform_device *pdev)
vfd = >vfd;
vfd->lock = >dev_mutex;
vfd->v4l2_dev = >v4l2_dev;
+   INIT_WORK(>work_run, device_work);
 
 #ifdef CONFIG_MEDIA_CONTROLLER
dev->mdev.dev = >dev;
-- 
2.16.3