[PATCH v5 0/5] Make sure .device_run is always called in non-atomic context

[PATCH v5 0/5] Make sure .device_run is always called in non-atomic context

[Ezequiel Garcia]

This series goal is to avoid drivers from having ad-hoc code
to call .device_run in non-atomic context. Currently, .device_run
can be called via v4l2_m2m_job_finish(), not only running
in interrupt context, but also creating a nasty re-entrant
path into mem2mem drivers.

The proposed solution is to add a per-device worker that is scheduled
by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt
or similar.

This change allows v4l2_m2m_job_finish() to be called in interrupt
context, separating .device_run and v4l2_m2m_job_finish() contexts.

It's worth mentioning that v4l2_m2m_cancel_job() doesn't need
to flush or cancel the new worker, because the job_spinlock
synchronizes both and also because the core prevents simultaneous
jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the
worker will be unable to run a new job.

Patches apply on top of Request API and the Cedrus VPU
driver.

Tested with cedrus driver using v4l2-request-test and
vicodec driver using gstreamer.

Ezequiel Garcia (4):
  mem2mem: Require capture and output mutexes to match
  v4l2-ioctl.c: Simplify locking for m2m devices
  v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
  media: cedrus: Get rid of interrupt bottom-half

Sakari Ailus (1):
  v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule

 drivers/media/v4l2-core/v4l2-ioctl.c          | 47 +------------
 drivers/media/v4l2-core/v4l2-mem2mem.c        | 66 ++++++++++++-------
 .../staging/media/sunxi/cedrus/cedrus_hw.c    | 26 ++------
 3 files changed, 51 insertions(+), 88 deletions(-)

-- 
2.19.1

[PATCH v5 1/5] mem2mem: Require capture and output mutexes to match

[Ezequiel Garcia]

Currently, all the mem2mem driver either use a single mutex
to lock the capture and output videobuf2 queues, or don't
set any mutex.

This means the mutexes match, and so the mem2mem framework
is able to set the m2m context lock.

Enforce this by making it mandatory for drivers to set
the same capture and output mutex, or not set any mutex at all.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index d7806db222d8..bc72243eb91d 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -908,12 +908,14 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
 	if (ret)
 		goto err;
 	/*
-	 * If both queues use same mutex assign it as the common buffer
-	 * queues lock to the m2m context. This lock is used in the
-	 * v4l2_m2m_ioctl_* helpers.
+	 * Both queues should use same the mutex to lock the m2m context.
+	 * This lock is used in some v4l2_m2m_* helpers.
 	 */
-	if (out_q_ctx->q.lock == cap_q_ctx->q.lock)
-		m2m_ctx->q_lock = out_q_ctx->q.lock;
+	if (WARN_ON(out_q_ctx->q.lock != cap_q_ctx->q.lock)) {
+		ret = -EINVAL;
+		goto err;
+	}
+	m2m_ctx->q_lock = out_q_ctx->q.lock;
 
 	return m2m_ctx;
 err:
-- 
2.19.1

[PATCH v5 2/5] v4l2-ioctl.c: Simplify locking for m2m devices

[Ezequiel Garcia]

Now that the mutexes for output and capture vb2 queues match,
it is possible to refer to the context q_lock as the
m2m lock for a given m2m context.

Remove the output/capture lock selection.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 47 ++--------------------------
 1 file changed, 2 insertions(+), 45 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index c63746968fa3..54919c227bc1 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2679,45 +2679,6 @@ static bool v4l2_is_known_ioctl(unsigned int cmd)
 	return v4l2_ioctls[_IOC_NR(cmd)].ioctl == cmd;
 }
 
-#if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
-static bool v4l2_ioctl_m2m_queue_is_output(unsigned int cmd, void *arg)
-{
-	switch (cmd) {
-	case VIDIOC_CREATE_BUFS: {
-		struct v4l2_create_buffers *cbufs = arg;
-
-		return V4L2_TYPE_IS_OUTPUT(cbufs->format.type);
-	}
-	case VIDIOC_REQBUFS: {
-		struct v4l2_requestbuffers *rbufs = arg;
-
-		return V4L2_TYPE_IS_OUTPUT(rbufs->type);
-	}
-	case VIDIOC_QBUF:
-	case VIDIOC_DQBUF:
-	case VIDIOC_QUERYBUF:
-	case VIDIOC_PREPARE_BUF: {
-		struct v4l2_buffer *buf = arg;
-
-		return V4L2_TYPE_IS_OUTPUT(buf->type);
-	}
-	case VIDIOC_EXPBUF: {
-		struct v4l2_exportbuffer *expbuf = arg;
-
-		return V4L2_TYPE_IS_OUTPUT(expbuf->type);
-	}
-	case VIDIOC_STREAMON:
-	case VIDIOC_STREAMOFF: {
-		int *type = arg;
-
-		return V4L2_TYPE_IS_OUTPUT(*type);
-	}
-	default:
-		return false;
-	}
-}
-#endif
-
 static struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev,
 					 struct v4l2_fh *vfh, unsigned int cmd,
 					 void *arg)
@@ -2727,12 +2688,8 @@ static struct mutex *v4l2_ioctl_get_lock(struct video_device *vdev,
 #if IS_ENABLED(CONFIG_V4L2_MEM2MEM_DEV)
 	if (vfh && vfh->m2m_ctx &&
 	    (v4l2_ioctls[_IOC_NR(cmd)].flags & INFO_FL_QUEUE)) {
-		bool is_output = v4l2_ioctl_m2m_queue_is_output(cmd, arg);
-		struct v4l2_m2m_queue_ctx *ctx = is_output ?
-			&vfh->m2m_ctx->out_q_ctx : &vfh->m2m_ctx->cap_q_ctx;
-
-		if (ctx->q.lock)
-			return ctx->q.lock;
+		if (vfh->m2m_ctx->q_lock)
+			return vfh->m2m_ctx->q_lock;
 	}
 #endif
 	if (vdev->queue && vdev->queue->lock &&
-- 
2.19.1

[PATCH v5 3/5] v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule

[Ezequiel Garcia]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The __v4l2_m2m_try_schedule function acquires and releases multiple
spinlocks. Simplify unlocking the job lock by adding labels to unlock
the lock and exit the function.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 29 ++++++++++++--------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index bc72243eb91d..0665a97ed89e 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -297,51 +297,48 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
 
 	/* If the context is aborted then don't schedule it */
 	if (m2m_ctx->job_flags & TRANS_ABORT) {
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("Aborted context\n");
-		return;
+		goto job_unlock;
 	}
 
 	if (m2m_ctx->job_flags & TRANS_QUEUED) {
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("On job queue already\n");
-		return;
+		goto job_unlock;
 	}
 
 	spin_lock_irqsave(&m2m_ctx->out_q_ctx.rdy_spinlock, flags_out);
 	if (list_empty(&m2m_ctx->out_q_ctx.rdy_queue)
 	    && !m2m_ctx->out_q_ctx.buffered) {
-		spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock,
-					flags_out);
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("No input buffers available\n");
-		return;
+		goto out_unlock;
 	}
 	spin_lock_irqsave(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags_cap);
 	if (list_empty(&m2m_ctx->cap_q_ctx.rdy_queue)
 	    && !m2m_ctx->cap_q_ctx.buffered) {
-		spin_unlock_irqrestore(&m2m_ctx->cap_q_ctx.rdy_spinlock,
-					flags_cap);
-		spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock,
-					flags_out);
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("No output buffers available\n");
-		return;
+		goto cap_unlock;
 	}
 	spin_unlock_irqrestore(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags_cap);
 	spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock, flags_out);
 
 	if (m2m_dev->m2m_ops->job_ready
 		&& (!m2m_dev->m2m_ops->job_ready(m2m_ctx->priv))) {
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("Driver not ready\n");
-		return;
+		goto job_unlock;
 	}
 
 	list_add_tail(&m2m_ctx->queue, &m2m_dev->job_queue);
 	m2m_ctx->job_flags |= TRANS_QUEUED;
 
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
+	return;
+
+cap_unlock:
+	spin_unlock_irqrestore(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags_cap);
+out_unlock:
+	spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock, flags_out);
+job_unlock:
+	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 }
 
 /**
-- 
2.19.1

[PATCH v5 4/5] v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish

[Ezequiel Garcia]

v4l2_m2m_job_finish() is typically called when
DMA operations complete, in interrupt handlers or DMA
completion callbacks. Calling .device_run from v4l2_m2m_job_finish
creates a nasty re-entrancy path into the driver.

Moreover, some implementation of .device_run might need to sleep,
as is the case for drivers supporting the Request API,
where controls are applied via v4l2_ctrl_request_setup,
which takes the ctrl handler mutex.

This commit adds a deferred context that calls v4l2_m2m_try_run,
and gets scheduled by v4l2_m2m_job_finish().

Before this change, device_run would be called from these
paths:

vb2_m2m_request_queue, or
v4l2_m2m_streamon, or
v4l2_m2m_qbuf
  v4l2_m2m_try_schedule
    v4l2_m2m_try_run
      .device_run

v4l2_m2m_job_finish
  v4l2_m2m_try_run
    .device_run

After this change, the latter is now gone and instead:

v4l2_m2m_device_run_work
  v4l2_m2m_try_run
    .device_run

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 0665a97ed89e..2b250fd10531 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -87,6 +87,7 @@ static const char * const m2m_entity_name[] = {
  * @curr_ctx:		currently running instance
  * @job_queue:		instances queued to run
  * @job_spinlock:	protects job_queue
+ * @job_work:		worker to run queued jobs.
  * @m2m_ops:		driver callbacks
  */
 struct v4l2_m2m_dev {
@@ -103,6 +104,7 @@ struct v4l2_m2m_dev {
 
 	struct list_head	job_queue;
 	spinlock_t		job_spinlock;
+	struct work_struct	job_work;
 
 	const struct v4l2_m2m_ops *m2m_ops;
 };
@@ -244,6 +246,9 @@ EXPORT_SYMBOL(v4l2_m2m_get_curr_priv);
  * @m2m_dev: per-device context
  *
  * Get next transaction (if present) from the waiting jobs list and run it.
+ *
+ * Note that this function can run on a given v4l2_m2m_ctx context,
+ * but call .device_run for another context.
  */
 static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
 {
@@ -362,6 +367,18 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
 
+/**
+ * v4l2_m2m_device_run_work() - run pending jobs for the context
+ * @work: Work structure used for scheduling the execution of this function.
+ */
+static void v4l2_m2m_device_run_work(struct work_struct *work)
+{
+	struct v4l2_m2m_dev *m2m_dev =
+		container_of(work, struct v4l2_m2m_dev, job_work);
+
+	v4l2_m2m_try_run(m2m_dev);
+}
+
 /**
  * v4l2_m2m_cancel_job() - cancel pending jobs for the context
  * @m2m_ctx: m2m context with jobs to be canceled
@@ -421,7 +438,12 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
 	/* This instance might have more buffers ready, but since we do not
 	 * allow more than one job on the job_queue per instance, each has
 	 * to be scheduled separately after the previous one finishes. */
-	v4l2_m2m_try_schedule(m2m_ctx);
+	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
+
+	/* We might be running in atomic context,
+	 * but the job must be run in non-atomic context.
+	 */
+	schedule_work(&m2m_dev->job_work);
 }
 EXPORT_SYMBOL(v4l2_m2m_job_finish);
 
@@ -863,6 +885,7 @@ struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops)
 	m2m_dev->m2m_ops = m2m_ops;
 	INIT_LIST_HEAD(&m2m_dev->job_queue);
 	spin_lock_init(&m2m_dev->job_spinlock);
+	INIT_WORK(&m2m_dev->job_work, v4l2_m2m_device_run_work);
 
 	return m2m_dev;
 }
-- 
2.19.1

[PATCH v5 5/5] media: cedrus: Get rid of interrupt bottom-half

[Ezequiel Garcia]

Now that the mem2mem framework guarantees that .device_run
won't be called from interrupt context, it is safe to call
v4l2_m2m_job_finish directly in the top-half.

So this means the bottom-half is no longer needed and we
can get rid of it.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 .../staging/media/sunxi/cedrus/cedrus_hw.c    | 26 ++++---------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
index 32adbcbe6175..493e65b17b30 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
@@ -98,23 +98,6 @@ void cedrus_dst_format_set(struct cedrus_dev *dev,
 	}
 }
 
-static irqreturn_t cedrus_bh(int irq, void *data)
-{
-	struct cedrus_dev *dev = data;
-	struct cedrus_ctx *ctx;
-
-	ctx = v4l2_m2m_get_curr_priv(dev->m2m_dev);
-	if (!ctx) {
-		v4l2_err(&dev->v4l2_dev,
-			 "Instance released before the end of transaction\n");
-		return IRQ_HANDLED;
-	}
-
-	v4l2_m2m_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx);
-
-	return IRQ_HANDLED;
-}
-
 static irqreturn_t cedrus_irq(int irq, void *data)
 {
 	struct cedrus_dev *dev = data;
@@ -165,7 +148,9 @@ static irqreturn_t cedrus_irq(int irq, void *data)
 
 	spin_unlock_irqrestore(&dev->irq_lock, flags);
 
-	return IRQ_WAKE_THREAD;
+	v4l2_m2m_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx);
+
+	return IRQ_HANDLED;
 }
 
 int cedrus_hw_probe(struct cedrus_dev *dev)
@@ -187,9 +172,8 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
 
 		return irq_dec;
 	}
-	ret = devm_request_threaded_irq(dev->dev, irq_dec, cedrus_irq,
-					cedrus_bh, 0, dev_name(dev->dev),
-					dev);
+	ret = devm_request_irq(dev->dev, irq_dec, cedrus_irq,
+			       0, dev_name(dev->dev), dev);
 	if (ret) {
 		v4l2_err(&dev->v4l2_dev, "Failed to request IRQ\n");
 
-- 
2.19.1

Re: [PATCH v5 5/5] media: cedrus: Get rid of interrupt bottom-half

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]

Hi,

On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote:
> Now that the mem2mem framework guarantees that .device_run
> won't be called from interrupt context, it is safe to call
> v4l2_m2m_job_finish directly in the top-half.
> 
> So this means the bottom-half is no longer needed and we
> can get rid of it.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> ---
>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 26 ++++---------------
>  1 file changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> index 32adbcbe6175..493e65b17b30 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -98,23 +98,6 @@ void cedrus_dst_format_set(struct cedrus_dev *dev,
>  	}
>  }
>  
> -static irqreturn_t cedrus_bh(int irq, void *data)
> -{
> -	struct cedrus_dev *dev = data;
> -	struct cedrus_ctx *ctx;
> -
> -	ctx = v4l2_m2m_get_curr_priv(dev->m2m_dev);
> -	if (!ctx) {
> -		v4l2_err(&dev->v4l2_dev,
> -			 "Instance released before the end of transaction\n");
> -		return IRQ_HANDLED;
> -	}
> -
> -	v4l2_m2m_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx);
> -
> -	return IRQ_HANDLED;
> -}
> -
>  static irqreturn_t cedrus_irq(int irq, void *data)
>  {
>  	struct cedrus_dev *dev = data;
> @@ -165,7 +148,9 @@ static irqreturn_t cedrus_irq(int irq, void *data)
>  
>  	spin_unlock_irqrestore(&dev->irq_lock, flags);
>  
> -	return IRQ_WAKE_THREAD;
> +	v4l2_m2m_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx);
> +
> +	return IRQ_HANDLED;
>  }
>  
>  int cedrus_hw_probe(struct cedrus_dev *dev)
> @@ -187,9 +172,8 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
>  
>  		return irq_dec;
>  	}
> -	ret = devm_request_threaded_irq(dev->dev, irq_dec, cedrus_irq,
> -					cedrus_bh, 0, dev_name(dev->dev),
> -					dev);
> +	ret = devm_request_irq(dev->dev, irq_dec, cedrus_irq,
> +			       0, dev_name(dev->dev), dev);
>  	if (ret) {
>  		v4l2_err(&dev->v4l2_dev, "Failed to request IRQ\n");
>  
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context

[Ezequiel Garcia]

On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote:
> This series goal is to avoid drivers from having ad-hoc code
> to call .device_run in non-atomic context. Currently, .device_run
> can be called via v4l2_m2m_job_finish(), not only running
> in interrupt context, but also creating a nasty re-entrant
> path into mem2mem drivers.
> 
> The proposed solution is to add a per-device worker that is scheduled
> by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt
> or similar.
> 
> This change allows v4l2_m2m_job_finish() to be called in interrupt
> context, separating .device_run and v4l2_m2m_job_finish() contexts.
> 
> It's worth mentioning that v4l2_m2m_cancel_job() doesn't need
> to flush or cancel the new worker, because the job_spinlock
> synchronizes both and also because the core prevents simultaneous
> jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the
> worker will be unable to run a new job.
> 
> Patches apply on top of Request API and the Cedrus VPU
> driver.
> 
> Tested with cedrus driver using v4l2-request-test and
> vicodec driver using gstreamer.
> 
> Ezequiel Garcia (4):
>   mem2mem: Require capture and output mutexes to match
>   v4l2-ioctl.c: Simplify locking for m2m devices
>   v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
>   media: cedrus: Get rid of interrupt bottom-half
> 
> Sakari Ailus (1):
>   v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule
> 
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 47 +------------
>  drivers/media/v4l2-core/v4l2-mem2mem.c        | 66 ++++++++++++-------
>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 26 ++------
>  3 files changed, 51 insertions(+), 88 deletions(-)
> 

Hans, Maxime:

Any feedback for this?

Thanks,
Eze

Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2382 bytes --]

Hi,

On Sun, 2018-11-11 at 18:26 -0300, Ezequiel Garcia wrote:
> On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote:
> > This series goal is to avoid drivers from having ad-hoc code
> > to call .device_run in non-atomic context. Currently, .device_run
> > can be called via v4l2_m2m_job_finish(), not only running
> > in interrupt context, but also creating a nasty re-entrant
> > path into mem2mem drivers.
> > 
> > The proposed solution is to add a per-device worker that is scheduled
> > by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt
> > or similar.
> > 
> > This change allows v4l2_m2m_job_finish() to be called in interrupt
> > context, separating .device_run and v4l2_m2m_job_finish() contexts.
> > 
> > It's worth mentioning that v4l2_m2m_cancel_job() doesn't need
> > to flush or cancel the new worker, because the job_spinlock
> > synchronizes both and also because the core prevents simultaneous
> > jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the
> > worker will be unable to run a new job.
> > 
> > Patches apply on top of Request API and the Cedrus VPU
> > driver.
> > 
> > Tested with cedrus driver using v4l2-request-test and
> > vicodec driver using gstreamer.
> > 
> > Ezequiel Garcia (4):
> >   mem2mem: Require capture and output mutexes to match
> >   v4l2-ioctl.c: Simplify locking for m2m devices
> >   v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
> >   media: cedrus: Get rid of interrupt bottom-half
> > 
> > Sakari Ailus (1):
> >   v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule
> > 
> >  drivers/media/v4l2-core/v4l2-ioctl.c          | 47 +------------
> >  drivers/media/v4l2-core/v4l2-mem2mem.c        | 66 ++++++++++++-------
> >  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 26 ++------
> >  3 files changed, 51 insertions(+), 88 deletions(-)
> > 
> 
> Hans, Maxime:
> 
> Any feedback for this?

I just tested the whole series with the cedrus driver and everything
looks good!

Removing the interrupt bottom-half in favor of a workqueue in the core
seems like a good way to simplify m2m driver development by avoiding
per-driver workqueues or threaded irqs.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context

[Ezequiel Garcia]

On Mon, 12 Nov 2018 at 13:52, Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Sun, 2018-11-11 at 18:26 -0300, Ezequiel Garcia wrote:
> > On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote:
> > > This series goal is to avoid drivers from having ad-hoc code
> > > to call .device_run in non-atomic context. Currently, .device_run
> > > can be called via v4l2_m2m_job_finish(), not only running
> > > in interrupt context, but also creating a nasty re-entrant
> > > path into mem2mem drivers.
> > >
> > > The proposed solution is to add a per-device worker that is scheduled
> > > by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt
> > > or similar.
> > >
> > > This change allows v4l2_m2m_job_finish() to be called in interrupt
> > > context, separating .device_run and v4l2_m2m_job_finish() contexts.
> > >
> > > It's worth mentioning that v4l2_m2m_cancel_job() doesn't need
> > > to flush or cancel the new worker, because the job_spinlock
> > > synchronizes both and also because the core prevents simultaneous
> > > jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the
> > > worker will be unable to run a new job.
> > >
> > > Patches apply on top of Request API and the Cedrus VPU
> > > driver.
> > >
> > > Tested with cedrus driver using v4l2-request-test and
> > > vicodec driver using gstreamer.
> > >
> > > Ezequiel Garcia (4):
> > >   mem2mem: Require capture and output mutexes to match
> > >   v4l2-ioctl.c: Simplify locking for m2m devices
> > >   v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
> > >   media: cedrus: Get rid of interrupt bottom-half
> > >
> > > Sakari Ailus (1):
> > >   v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule
> > >
> > >  drivers/media/v4l2-core/v4l2-ioctl.c          | 47 +------------
> > >  drivers/media/v4l2-core/v4l2-mem2mem.c        | 66 ++++++++++++-------
> > >  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 26 ++------
> > >  3 files changed, 51 insertions(+), 88 deletions(-)
> > >
> >
> > Hans, Maxime:
> >
> > Any feedback for this?
>
> I just tested the whole series with the cedrus driver and everything
> looks good!
>

Good! So this means we can add a Tested-by for the entire series?

> Removing the interrupt bottom-half in favor of a workqueue in the core
> seems like a good way to simplify m2m driver development by avoiding
> per-driver workqueues or threaded irqs.
>

Thanks for the test!
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2933 bytes --]

Hi,

On Mon, 2018-11-12 at 18:05 -0300, Ezequiel Garcia wrote:
> On Mon, 12 Nov 2018 at 13:52, Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > On Sun, 2018-11-11 at 18:26 -0300, Ezequiel Garcia wrote:
> > > On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote:
> > > > This series goal is to avoid drivers from having ad-hoc code
> > > > to call .device_run in non-atomic context. Currently, .device_run
> > > > can be called via v4l2_m2m_job_finish(), not only running
> > > > in interrupt context, but also creating a nasty re-entrant
> > > > path into mem2mem drivers.
> > > > 
> > > > The proposed solution is to add a per-device worker that is scheduled
> > > > by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt
> > > > or similar.
> > > > 
> > > > This change allows v4l2_m2m_job_finish() to be called in interrupt
> > > > context, separating .device_run and v4l2_m2m_job_finish() contexts.
> > > > 
> > > > It's worth mentioning that v4l2_m2m_cancel_job() doesn't need
> > > > to flush or cancel the new worker, because the job_spinlock
> > > > synchronizes both and also because the core prevents simultaneous
> > > > jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the
> > > > worker will be unable to run a new job.
> > > > 
> > > > Patches apply on top of Request API and the Cedrus VPU
> > > > driver.
> > > > 
> > > > Tested with cedrus driver using v4l2-request-test and
> > > > vicodec driver using gstreamer.
> > > > 
> > > > Ezequiel Garcia (4):
> > > >   mem2mem: Require capture and output mutexes to match
> > > >   v4l2-ioctl.c: Simplify locking for m2m devices
> > > >   v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
> > > >   media: cedrus: Get rid of interrupt bottom-half
> > > > 
> > > > Sakari Ailus (1):
> > > >   v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule
> > > > 
> > > >  drivers/media/v4l2-core/v4l2-ioctl.c          | 47 +------------
> > > >  drivers/media/v4l2-core/v4l2-mem2mem.c        | 66 ++++++++++++-------
> > > >  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 26 ++------
> > > >  3 files changed, 51 insertions(+), 88 deletions(-)
> > > > 
> > > 
> > > Hans, Maxime:
> > > 
> > > Any feedback for this?
> > 
> > I just tested the whole series with the cedrus driver and everything
> > looks good!
> > 
> 
> Good! So this means we can add a Tested-by for the entire series?

Definitely, I just sent the tested-by line on the cover letter!

> > Removing the interrupt bottom-half in favor of a workqueue in the core
> > seems like a good way to simplify m2m driver development by avoiding
> > per-driver workqueues or threaded irqs.
> > 
> 
> Thanks for the test!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 0/5] Make sure .device_run is always called in non-atomic context

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]

Hi,

On Thu, 2018-10-18 at 15:02 -0300, Ezequiel Garcia wrote:
> This series goal is to avoid drivers from having ad-hoc code
> to call .device_run in non-atomic context. Currently, .device_run
> can be called via v4l2_m2m_job_finish(), not only running
> in interrupt context, but also creating a nasty re-entrant
> path into mem2mem drivers.
> 
> The proposed solution is to add a per-device worker that is scheduled
> by v4l2_m2m_job_finish, which replaces drivers having a threaded interrupt
> or similar.
> 
> This change allows v4l2_m2m_job_finish() to be called in interrupt
> context, separating .device_run and v4l2_m2m_job_finish() contexts.
> 
> It's worth mentioning that v4l2_m2m_cancel_job() doesn't need
> to flush or cancel the new worker, because the job_spinlock
> synchronizes both and also because the core prevents simultaneous
> jobs. Either v4l2_m2m_cancel_job() will wait for the worker, or the
> worker will be unable to run a new job.
> 
> Patches apply on top of Request API and the Cedrus VPU
> driver.
> 
> Tested with cedrus driver using v4l2-request-test and
> vicodec driver using gstreamer.

For the entire series, tested with the cedrus driver:
Tested-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> Ezequiel Garcia (4):
>   mem2mem: Require capture and output mutexes to match
>   v4l2-ioctl.c: Simplify locking for m2m devices
>   v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
>   media: cedrus: Get rid of interrupt bottom-half
> 
> Sakari Ailus (1):
>   v4l2-mem2mem: Simplify exiting the function in __v4l2_m2m_try_schedule
> 
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 47 +------------
>  drivers/media/v4l2-core/v4l2-mem2mem.c        | 66 ++++++++++++-------
>  .../staging/media/sunxi/cedrus/cedrus_hw.c    | 26 ++------
>  3 files changed, 51 insertions(+), 88 deletions(-)
> 
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]