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

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

[Ezequiel Garcia]

This series goal is avoiding 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(), potentially running
in interrupt context.

This is needed for the upcoming Request API, where drivers typically
require .device_run to be called in non-atomic context for
v4l2_ctrl_request_setup() calls.

The solution is quite simple, instead of drivers having a threaded interrupt
or similar, the mem2mem core has a per-context worker that is scheduled
by v4l2_m2m_job_finish.

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.

Paul, Maxime: This should avoid the threaded interrupt in the Cedrus
driver. Please, take a look and let me know how it goes.

Patches are based on v4.18-rc4 plus:

c1dbb540e35e "v4l2-mem2mem: Simplify exiting the function in v4l2_m2m_try_schedule"
be3d3b78573b "media: mem2mem: Remove excessive try_run call"

Ezequiel Garcia (2):
  v4l2-core: Simplify v4l2_m2m_try_{schedule,run}
  v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish

 drivers/media/v4l2-core/v4l2-mem2mem.c | 58 ++++++++++----------------
 include/media/v4l2-mem2mem.h           |  2 +
 2 files changed, 25 insertions(+), 35 deletions(-)

-- 
2.18.0.rc2

[PATCH 1/2] v4l2-core: Simplify v4l2_m2m_try_{schedule,run}

[Ezequiel Garcia]

v4l2_m2m_try_run() has only one caller and so it's possible
to move its contents.

Although this de-modularization change could reduce clarity,
in this case it allows to remove a spinlock lock/unlock pair
and an unneeded sanity check.

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

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 6bce8dcd182a..c2e9c2b7dcd1 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -181,37 +181,6 @@ void *v4l2_m2m_get_curr_priv(struct v4l2_m2m_dev *m2m_dev)
 }
 EXPORT_SYMBOL(v4l2_m2m_get_curr_priv);
 
-/**
- * v4l2_m2m_try_run() - select next job to perform and run it if possible
- * @m2m_dev: per-device context
- *
- * Get next transaction (if present) from the waiting jobs list and run it.
- */
-static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
-	if (NULL != m2m_dev->curr_ctx) {
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
-		dprintk("Another instance is running, won't run now\n");
-		return;
-	}
-
-	if (list_empty(&m2m_dev->job_queue)) {
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
-		dprintk("No job pending\n");
-		return;
-	}
-
-	m2m_dev->curr_ctx = list_first_entry(&m2m_dev->job_queue,
-				   struct v4l2_m2m_ctx, queue);
-	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
-	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
-
-	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
-}
-
 void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 {
 	struct v4l2_m2m_dev *m2m_dev;
@@ -269,15 +238,22 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 	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);
+	if (NULL != m2m_dev->curr_ctx) {
+		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
+		dprintk("Another instance is running, won't run now\n");
+		return;
+	}
 
-	v4l2_m2m_try_run(m2m_dev);
+	m2m_dev->curr_ctx = list_first_entry(&m2m_dev->job_queue,
+				   struct v4l2_m2m_ctx, queue);
+	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
+	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 
+	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
 	return;
 
 out_unlock:
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
-
 	return;
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
-- 
2.18.0.rc2

Re: [PATCH 1/2] v4l2-core: Simplify v4l2_m2m_try_{schedule,run}

[Hans Verkuil]

On 12/07/18 17:43, Ezequiel Garcia wrote:
> v4l2_m2m_try_run() has only one caller and so it's possible
> to move its contents.
> 
> Although this de-modularization change could reduce clarity,
> in this case it allows to remove a spinlock lock/unlock pair
> and an unneeded sanity check.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

This patch no longer applies, can you respin?

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 44 ++++++--------------------
>  1 file changed, 10 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 6bce8dcd182a..c2e9c2b7dcd1 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -181,37 +181,6 @@ void *v4l2_m2m_get_curr_priv(struct v4l2_m2m_dev *m2m_dev)
>  }
>  EXPORT_SYMBOL(v4l2_m2m_get_curr_priv);
>  
> -/**
> - * v4l2_m2m_try_run() - select next job to perform and run it if possible
> - * @m2m_dev: per-device context
> - *
> - * Get next transaction (if present) from the waiting jobs list and run it.
> - */
> -static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
> -	if (NULL != m2m_dev->curr_ctx) {
> -		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> -		dprintk("Another instance is running, won't run now\n");
> -		return;
> -	}
> -
> -	if (list_empty(&m2m_dev->job_queue)) {
> -		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> -		dprintk("No job pending\n");
> -		return;
> -	}
> -
> -	m2m_dev->curr_ctx = list_first_entry(&m2m_dev->job_queue,
> -				   struct v4l2_m2m_ctx, queue);
> -	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
> -	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> -
> -	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
> -}
> -
>  void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>  {
>  	struct v4l2_m2m_dev *m2m_dev;
> @@ -269,15 +238,22 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>  	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);
> +	if (NULL != m2m_dev->curr_ctx) {
> +		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
> +		dprintk("Another instance is running, won't run now\n");
> +		return;
> +	}
>  
> -	v4l2_m2m_try_run(m2m_dev);
> +	m2m_dev->curr_ctx = list_first_entry(&m2m_dev->job_queue,
> +				   struct v4l2_m2m_ctx, queue);
> +	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
> +	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
>  
> +	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>  	return;
>  
>  out_unlock:
>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
> -
>  	return;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
> 

Re: [PATCH 1/2] v4l2-core: Simplify v4l2_m2m_try_{schedule,run}

[Ezequiel Garcia]

On Wed, 2018-07-18 at 12:23 +0200, Hans Verkuil wrote:
> On 12/07/18 17:43, Ezequiel Garcia wrote:
> > v4l2_m2m_try_run() has only one caller and so it's possible
> > to move its contents.
> > 
> > Although this de-modularization change could reduce clarity,
> > in this case it allows to remove a spinlock lock/unlock pair
> > and an unneeded sanity check.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> 
> This patch no longer applies, can you respin?
> 

I think that is because it applies on top of:

    v4l2-mem2mem: Simplify exiting the function in v4l2_m2m_try_schedule
    
    The v4l2_m2m_try_schedule function acquires and releases multiple
    spinlocks; simplify unlocking the job lock by adding a label to unlock the
    job lock and exit the function.

which I cherry-picked from the Request API series.

I will include this patch in my next submit, in case you want
to take the series soon(ishly).

Thanks,
Ezequiel

[PATCH 2/2] v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish

[Ezequiel Garcia]

v4l2_m2m_job_finish() is typically called in interrupt context.

Some implementation of .device_run might sleep, and so it's
desirable to avoid calling it directly from
v4l2_m2m_job_finish(), thus avoiding .device_run from running
in interrupt context.

Implement a deferred context that gets scheduled by
v4l2_m2m_job_finish().

The worker calls v4l2_m2m_try_schedule(), which makes sure
a single job is running at the same time, so it's safe to
call it from different executions context.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 14 +++++++++++++-
 include/media/v4l2-mem2mem.h           |  2 ++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index c2e9c2b7dcd1..1d0e20809ffe 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -258,6 +258,17 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
 
+/**
+ * v4l2_m2m_try_schedule_work() - run pending jobs for the context
+ * @work: Work structure used for scheduling the execution of this function.
+ */
+static void v4l2_m2m_try_schedule_work(struct work_struct *work)
+{
+	struct v4l2_m2m_ctx *m2m_ctx =
+		container_of(work, struct v4l2_m2m_ctx, job_work);
+	v4l2_m2m_try_schedule(m2m_ctx);
+}
+
 /**
  * v4l2_m2m_cancel_job() - cancel pending jobs for the context
  * @m2m_ctx: m2m context with jobs to be canceled
@@ -316,7 +327,7 @@ 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);
+	schedule_work(&m2m_ctx->job_work);
 }
 EXPORT_SYMBOL(v4l2_m2m_job_finish);
 
@@ -614,6 +625,7 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
 	m2m_ctx->priv = drv_priv;
 	m2m_ctx->m2m_dev = m2m_dev;
 	init_waitqueue_head(&m2m_ctx->finished);
+	INIT_WORK(&m2m_ctx->job_work, v4l2_m2m_try_schedule_work);
 
 	out_q_ctx = &m2m_ctx->out_q_ctx;
 	cap_q_ctx = &m2m_ctx->cap_q_ctx;
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 3d07ba3a8262..2543fbfdd2b1 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -89,6 +89,7 @@ struct v4l2_m2m_queue_ctx {
  * @job_flags: Job queue flags, used internally by v4l2-mem2mem.c:
  *		%TRANS_QUEUED, %TRANS_RUNNING and %TRANS_ABORT.
  * @finished: Wait queue used to signalize when a job queue finished.
+ * @job_work: Worker to run queued jobs.
  * @priv: Instance private data
  *
  * The memory to memory context is specific to a file handle, NOT to e.g.
@@ -109,6 +110,7 @@ struct v4l2_m2m_ctx {
 	struct list_head		queue;
 	unsigned long			job_flags;
 	wait_queue_head_t		finished;
+	struct work_struct		job_work;
 
 	void				*priv;
 };
-- 
2.18.0.rc2

Re: [PATCH 2/2] v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish

[Hans Verkuil]

On 12/07/18 17:43, Ezequiel Garcia wrote:
> v4l2_m2m_job_finish() is typically called in interrupt context.
> 
> Some implementation of .device_run might sleep, and so it's
> desirable to avoid calling it directly from
> v4l2_m2m_job_finish(), thus avoiding .device_run from running
> in interrupt context.
> 
> Implement a deferred context that gets scheduled by
> v4l2_m2m_job_finish().
> 
> The worker calls v4l2_m2m_try_schedule(), which makes sure
> a single job is running at the same time, so it's safe to
> call it from different executions context.

I am not sure about this. I think that the only thing that needs to
run in the work queue is the call to device_run. Everything else
up to that point runs fine in interrupt context.

While we're on it: I see that v4l2_m2m_prepare_buf() also calls
v4l2_m2m_try_schedule(): I don't think it should do that since
prepare_buf does not actually queue a new buffer to the driver.

Regards,

	Hans

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 14 +++++++++++++-
>  include/media/v4l2-mem2mem.h           |  2 ++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index c2e9c2b7dcd1..1d0e20809ffe 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -258,6 +258,17 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
>  
> +/**
> + * v4l2_m2m_try_schedule_work() - run pending jobs for the context
> + * @work: Work structure used for scheduling the execution of this function.
> + */
> +static void v4l2_m2m_try_schedule_work(struct work_struct *work)
> +{
> +	struct v4l2_m2m_ctx *m2m_ctx =
> +		container_of(work, struct v4l2_m2m_ctx, job_work);
> +	v4l2_m2m_try_schedule(m2m_ctx);
> +}
> +
>  /**
>   * v4l2_m2m_cancel_job() - cancel pending jobs for the context
>   * @m2m_ctx: m2m context with jobs to be canceled
> @@ -316,7 +327,7 @@ 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);
> +	schedule_work(&m2m_ctx->job_work);
>  }
>  EXPORT_SYMBOL(v4l2_m2m_job_finish);
>  
> @@ -614,6 +625,7 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
>  	m2m_ctx->priv = drv_priv;
>  	m2m_ctx->m2m_dev = m2m_dev;
>  	init_waitqueue_head(&m2m_ctx->finished);
> +	INIT_WORK(&m2m_ctx->job_work, v4l2_m2m_try_schedule_work);
>  
>  	out_q_ctx = &m2m_ctx->out_q_ctx;
>  	cap_q_ctx = &m2m_ctx->cap_q_ctx;
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index 3d07ba3a8262..2543fbfdd2b1 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -89,6 +89,7 @@ struct v4l2_m2m_queue_ctx {
>   * @job_flags: Job queue flags, used internally by v4l2-mem2mem.c:
>   *		%TRANS_QUEUED, %TRANS_RUNNING and %TRANS_ABORT.
>   * @finished: Wait queue used to signalize when a job queue finished.
> + * @job_work: Worker to run queued jobs.
>   * @priv: Instance private data
>   *
>   * The memory to memory context is specific to a file handle, NOT to e.g.
> @@ -109,6 +110,7 @@ struct v4l2_m2m_ctx {
>  	struct list_head		queue;
>  	unsigned long			job_flags;
>  	wait_queue_head_t		finished;
> +	struct work_struct		job_work;
>  
>  	void				*priv;
>  };
> 

Re: [PATCH 2/2] v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish

[Ezequiel Garcia]

On Wed, 2018-07-18 at 12:21 +0200, Hans Verkuil wrote:
> On 12/07/18 17:43, Ezequiel Garcia wrote:
> > v4l2_m2m_job_finish() is typically called in interrupt context.
> > 
> > Some implementation of .device_run might sleep, and so it's
> > desirable to avoid calling it directly from
> > v4l2_m2m_job_finish(), thus avoiding .device_run from running
> > in interrupt context.
> > 
> > Implement a deferred context that gets scheduled by
> > v4l2_m2m_job_finish().
> > 
> > The worker calls v4l2_m2m_try_schedule(), which makes sure
> > a single job is running at the same time, so it's safe to
> > call it from different executions context.
> 
> I am not sure about this. I think that the only thing that needs to
> run in the work queue is the call to device_run. Everything else
> up to that point runs fine in interrupt context.
> 

Yes, I think you are right. I originally went for the safer path
of running v4l2_m2m_try_schedule in the worker, but I think
it works to run only .device_run.

So there will be two paths to .device_run:

  * qbuf/streamon which checks if a job is possible
    for this or any other m2m context, and then
    calls .device_run for the selected m2m context.

  * job_finish will check if there is a job possible,
    for this or any other m2m context, then
    select a context by setting the current context
    and setting TRANS_RUNNING, then schedule the worker.

Only one m2m context can be running at the same time, and
v4l2_m2m_cancel_job is called by v4l2_m2m_streamoff and
v4l2_m2m_ctx_release, guaranteeing we don't try to run
a job on a stopped/released m2m context.

> While we're on it: I see that v4l2_m2m_prepare_buf() also calls
> v4l2_m2m_try_schedule(): I don't think it should do that since
> prepare_buf does not actually queue a new buffer to the driver.
> 

Yes, you are right about that. I'll remove it.

Thanks for reviewing,
Eze