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

[PATCH v2 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(), potentially running
in interrupt context.

This series will be useful 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 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.

While working on this series, I found a bug recently introduced on
commit "media: mem2mem: Remove excessive try_run call". The first patch
fixes the bug.

In order to test the change, and make sure no regressions are
introduced, a kselftest test is added to stress the mem2mem framework.

Patches are based on v4.18-rc4 plus:

34dbb848d5e47 "media: mem2mem: Remove excessive try_run call"

Ezequiel Garcia (4):
  v4l2-mem2mem: Fix missing v4l2_m2m_try_run call
  v4l2-mem2mem: Avoid v4l2_m2m_prepare_buf from scheduling a job
  v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
  selftests: media_tests: Add a memory-to-memory concurrent stress test

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

 drivers/media/v4l2-core/v4l2-mem2mem.c        | 104 +++++--
 tools/testing/selftests/media_tests/Makefile  |   4 +-
 .../selftests/media_tests/m2m_job_test.c      | 283 ++++++++++++++++++
 3 files changed, 362 insertions(+), 29 deletions(-)
 create mode 100644 tools/testing/selftests/media_tests/m2m_job_test.c

-- 
2.18.0

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

[ezequiel]

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(), potentially running
in interrupt context.

This series will be useful 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 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.

While working on this series, I found a bug recently introduced on
commit "media: mem2mem: Remove excessive try_run call". The first patch
fixes the bug.

In order to test the change, and make sure no regressions are
introduced, a kselftest test is added to stress the mem2mem framework.

Patches are based on v4.18-rc4 plus:

34dbb848d5e47 "media: mem2mem: Remove excessive try_run call"

Ezequiel Garcia (4):
  v4l2-mem2mem: Fix missing v4l2_m2m_try_run call
  v4l2-mem2mem: Avoid v4l2_m2m_prepare_buf from scheduling a job
  v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
  selftests: media_tests: Add a memory-to-memory concurrent stress test

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

 drivers/media/v4l2-core/v4l2-mem2mem.c        | 104 +++++--
 tools/testing/selftests/media_tests/Makefile  |   4 +-
 .../selftests/media_tests/m2m_job_test.c      | 283 ++++++++++++++++++
 3 files changed, 362 insertions(+), 29 deletions(-)
 create mode 100644 tools/testing/selftests/media_tests/m2m_job_test.c

-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 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(), potentially running
in interrupt context.

This series will be useful 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 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.

While working on this series, I found a bug recently introduced on
commit "media: mem2mem: Remove excessive try_run call". The first patch
fixes the bug.

In order to test the change, and make sure no regressions are
introduced, a kselftest test is added to stress the mem2mem framework.

Patches are based on v4.18-rc4 plus:

34dbb848d5e47 "media: mem2mem: Remove excessive try_run call"

Ezequiel Garcia (4):
  v4l2-mem2mem: Fix missing v4l2_m2m_try_run call
  v4l2-mem2mem: Avoid v4l2_m2m_prepare_buf from scheduling a job
  v4l2-mem2mem: Avoid calling .device_run in v4l2_m2m_job_finish
  selftests: media_tests: Add a memory-to-memory concurrent stress test

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

 drivers/media/v4l2-core/v4l2-mem2mem.c        | 104 +++++--
 tools/testing/selftests/media_tests/Makefile  |   4 +-
 .../selftests/media_tests/m2m_job_test.c      | 283 ++++++++++++++++++
 3 files changed, 362 insertions(+), 29 deletions(-)
 create mode 100644 tools/testing/selftests/media_tests/m2m_job_test.c

-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Ezequiel Garcia]

Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
removed a redundant call to v4l2_m2m_try_run but instead introduced
a bug. Consider the following case:

 1) Context A schedules, queues and runs job A.
 2) While the m2m device is running, context B schedules
    and queues job B. Job B cannot run, because it has to
    wait for job A.
 3) Job A completes, calls v4l2_m2m_job_finish, and tries
    to queue a job for context A, but since the context is
    empty it won't do anything.

In this scenario, queued job B will never run. Fix this by calling
v4l2_m2m_try_run from v4l2_m2m_try_schedule.

While here, add more documentation to these functions.

Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 5f9cd5b74cda..dfd796621b06 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
 	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
 
+	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
 	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
 }
 
-void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
+/*
+ * __v4l2_m2m_try_queue() - queue a job
+ * @m2m_dev: m2m device
+ * @m2m_ctx: m2m context
+ *
+ * Check if this context is ready to queue a job.
+ *
+ * This function can run in interrupt context.
+ */
+static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
 {
-	struct v4l2_m2m_dev *m2m_dev;
 	unsigned long flags_job, flags_out, flags_cap;
 
-	m2m_dev = m2m_ctx->m2m_dev;
 	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
 
 	if (!m2m_ctx->out_q_ctx.q.streaming
@@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 	m2m_ctx->job_flags |= TRANS_QUEUED;
 
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
+}
+
+/**
+ * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
+ * @m2m_ctx: m2m context
+ *
+ * Check if this context is ready to queue a job. If suitable,
+ * run the next queued job on the mem2mem device.
+ *
+ * This function shouldn't run in interrupt context.
+ *
+ * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
+ * and then run another job for another context.
+ */
+void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
+{
+	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
 
+	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
 	v4l2_m2m_try_run(m2m_dev);
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
-- 
2.18.0

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[ezequiel]

Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
removed a redundant call to v4l2_m2m_try_run but instead introduced
a bug. Consider the following case:

 1) Context A schedules, queues and runs job A.
 2) While the m2m device is running, context B schedules
    and queues job B. Job B cannot run, because it has to
    wait for job A.
 3) Job A completes, calls v4l2_m2m_job_finish, and tries
    to queue a job for context A, but since the context is
    empty it won't do anything.

In this scenario, queued job B will never run. Fix this by calling
v4l2_m2m_try_run from v4l2_m2m_try_schedule.

While here, add more documentation to these functions.

Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 5f9cd5b74cda..dfd796621b06 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
 	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
 
+	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
 	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
 }
 
-void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
+/*
+ * __v4l2_m2m_try_queue() - queue a job
+ * @m2m_dev: m2m device
+ * @m2m_ctx: m2m context
+ *
+ * Check if this context is ready to queue a job.
+ *
+ * This function can run in interrupt context.
+ */
+static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
 {
-	struct v4l2_m2m_dev *m2m_dev;
 	unsigned long flags_job, flags_out, flags_cap;
 
-	m2m_dev = m2m_ctx->m2m_dev;
 	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
 
 	if (!m2m_ctx->out_q_ctx.q.streaming
@@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 	m2m_ctx->job_flags |= TRANS_QUEUED;
 
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
+}
+
+/**
+ * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
+ * @m2m_ctx: m2m context
+ *
+ * Check if this context is ready to queue a job. If suitable,
+ * run the next queued job on the mem2mem device.
+ *
+ * This function shouldn't run in interrupt context.
+ *
+ * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
+ * and then run another job for another context.
+ */
+void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
+{
+	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
 
+	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
 	v4l2_m2m_try_run(m2m_dev);
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Ezequiel Garcia]

Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
removed a redundant call to v4l2_m2m_try_run but instead introduced
a bug. Consider the following case:

 1) Context A schedules, queues and runs job A.
 2) While the m2m device is running, context B schedules
    and queues job B. Job B cannot run, because it has to
    wait for job A.
 3) Job A completes, calls v4l2_m2m_job_finish, and tries
    to queue a job for context A, but since the context is
    empty it won't do anything.

In this scenario, queued job B will never run. Fix this by calling
v4l2_m2m_try_run from v4l2_m2m_try_schedule.

While here, add more documentation to these functions.

Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 5f9cd5b74cda..dfd796621b06 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
 	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
 
+	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
 	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
 }
 
-void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
+/*
+ * __v4l2_m2m_try_queue() - queue a job
+ * @m2m_dev: m2m device
+ * @m2m_ctx: m2m context
+ *
+ * Check if this context is ready to queue a job.
+ *
+ * This function can run in interrupt context.
+ */
+static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
 {
-	struct v4l2_m2m_dev *m2m_dev;
 	unsigned long flags_job, flags_out, flags_cap;
 
-	m2m_dev = m2m_ctx->m2m_dev;
 	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
 
 	if (!m2m_ctx->out_q_ctx.q.streaming
@@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 	m2m_ctx->job_flags |= TRANS_QUEUED;
 
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
+}
+
+/**
+ * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
+ * @m2m_ctx: m2m context
+ *
+ * Check if this context is ready to queue a job. If suitable,
+ * run the next queued job on the mem2mem device.
+ *
+ * This function shouldn't run in interrupt context.
+ *
+ * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
+ * and then run another job for another context.
+ */
+void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
+{
+	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
 
+	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
 	v4l2_m2m_try_run(m2m_dev);
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/5] v4l2-mem2mem: Avoid v4l2_m2m_prepare_buf from scheduling a job

[Ezequiel Garcia]

There is no need for v4l2_m2m_prepare_buf to try to schedule a job,
as it only prepares a buffer, but does not queue or changes the
state of the queue.

Remove the call to v4l2_m2m_try_schedule from v4l2_m2m_prepare_buf.

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

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index dfd796621b06..69a91c0030be 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -439,14 +439,9 @@ int v4l2_m2m_prepare_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 			 struct v4l2_buffer *buf)
 {
 	struct vb2_queue *vq;
-	int ret;
 
 	vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
-	ret = vb2_prepare_buf(vq, buf);
-	if (!ret)
-		v4l2_m2m_try_schedule(m2m_ctx);
-
-	return ret;
+	return vb2_prepare_buf(vq, buf);
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_prepare_buf);
 
-- 
2.18.0

[PATCH v2 2/5] v4l2-mem2mem: Avoid v4l2_m2m_prepare_buf from scheduling a job

[ezequiel]

There is no need for v4l2_m2m_prepare_buf to try to schedule a job,
as it only prepares a buffer, but does not queue or changes the
state of the queue.

Remove the call to v4l2_m2m_try_schedule from v4l2_m2m_prepare_buf.

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

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index dfd796621b06..69a91c0030be 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -439,14 +439,9 @@ int v4l2_m2m_prepare_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 			 struct v4l2_buffer *buf)
 {
 	struct vb2_queue *vq;
-	int ret;
 
 	vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
-	ret = vb2_prepare_buf(vq, buf);
-	if (!ret)
-		v4l2_m2m_try_schedule(m2m_ctx);
-
-	return ret;
+	return vb2_prepare_buf(vq, buf);
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_prepare_buf);
 
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/5] v4l2-mem2mem: Avoid v4l2_m2m_prepare_buf from scheduling a job

[Ezequiel Garcia]

There is no need for v4l2_m2m_prepare_buf to try to schedule a job,
as it only prepares a buffer, but does not queue or changes the
state of the queue.

Remove the call to v4l2_m2m_try_schedule from v4l2_m2m_prepare_buf.

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

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index dfd796621b06..69a91c0030be 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -439,14 +439,9 @@ int v4l2_m2m_prepare_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 			 struct v4l2_buffer *buf)
 {
 	struct vb2_queue *vq;
-	int ret;
 
 	vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
-	ret = vb2_prepare_buf(vq, buf);
-	if (!ret)
-		v4l2_m2m_try_schedule(m2m_ctx);
-
-	return ret;
+	return vb2_prepare_buf(vq, buf);
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_prepare_buf);
 
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 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 69a91c0030be..a25b45d8abeb 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -238,51 +238,48 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_c
 
 	/* 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.18.0

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

[ezequiel]

From: Sakari Ailus <sakari.ailus at 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 at linux.intel.com>
Signed-off-by: Ezequiel Garcia <ezequiel at 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 69a91c0030be..a25b45d8abeb 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -238,51 +238,48 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_c
 
 	/* 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.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 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 at linux.intel.com>
Signed-off-by: Ezequiel Garcia <ezequiel at 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 69a91c0030be..a25b45d8abeb 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -238,51 +238,48 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_c
 
 	/* 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.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/5] 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 calls v4l2_m2m_try_run,
and gets scheduled by v4l2_m2m_job_finish().

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

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index a25b45d8abeb..fb876ddec8a2 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -56,6 +56,7 @@ module_param(debug, bool, 0644);
  * @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 {
@@ -63,6 +64,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;
 };
@@ -184,10 +186,11 @@ 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
+ * @try_lock: indicates if the queue lock should be taken
  *
  * 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)
+static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev, bool try_lock)
 {
 	unsigned long flags;
 
@@ -210,7 +213,20 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
 
 	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
+
+	/*
+	 * A m2m context lock is taken only after a m2m context
+	 * is picked from the queue and marked as running.
+	 * The lock is only needed if v4l2_m2m_try_run is called
+	 * from the async worker.
+	 */
+	if (try_lock && m2m_dev->curr_ctx->q_lock)
+		mutex_lock(m2m_dev->curr_ctx->q_lock);
+
 	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
+
+	if (try_lock && m2m_dev->curr_ctx->q_lock)
+		mutex_unlock(m2m_dev->curr_ctx->q_lock);
 }
 
 /*
@@ -299,10 +315,22 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
 
 	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
-	v4l2_m2m_try_run(m2m_dev);
+	v4l2_m2m_try_run(m2m_dev, false);
 }
 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, true);
+}
+
 /**
  * v4l2_m2m_cancel_job() - cancel pending jobs for the context
  * @m2m_ctx: m2m context with jobs to be canceled
@@ -361,7 +389,8 @@ 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);
+	schedule_work(&m2m_dev->job_work);
 }
 EXPORT_SYMBOL(v4l2_m2m_job_finish);
 
@@ -628,6 +657,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.18.0

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

[ezequiel]

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 calls v4l2_m2m_try_run,
and gets scheduled by v4l2_m2m_job_finish().

Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 36 +++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index a25b45d8abeb..fb876ddec8a2 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -56,6 +56,7 @@ module_param(debug, bool, 0644);
  * @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 {
@@ -63,6 +64,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;
 };
@@ -184,10 +186,11 @@ 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
+ * @try_lock: indicates if the queue lock should be taken
  *
  * 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)
+static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev, bool try_lock)
 {
 	unsigned long flags;
 
@@ -210,7 +213,20 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
 
 	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
+
+	/*
+	 * A m2m context lock is taken only after a m2m context
+	 * is picked from the queue and marked as running.
+	 * The lock is only needed if v4l2_m2m_try_run is called
+	 * from the async worker.
+	 */
+	if (try_lock && m2m_dev->curr_ctx->q_lock)
+		mutex_lock(m2m_dev->curr_ctx->q_lock);
+
 	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
+
+	if (try_lock && m2m_dev->curr_ctx->q_lock)
+		mutex_unlock(m2m_dev->curr_ctx->q_lock);
 }
 
 /*
@@ -299,10 +315,22 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
 
 	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
-	v4l2_m2m_try_run(m2m_dev);
+	v4l2_m2m_try_run(m2m_dev, false);
 }
 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, true);
+}
+
 /**
  * v4l2_m2m_cancel_job() - cancel pending jobs for the context
  * @m2m_ctx: m2m context with jobs to be canceled
@@ -361,7 +389,8 @@ 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);
+	schedule_work(&m2m_dev->job_work);
 }
 EXPORT_SYMBOL(v4l2_m2m_job_finish);
 
@@ -628,6 +657,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.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/5] 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 calls v4l2_m2m_try_run,
and gets scheduled by v4l2_m2m_job_finish().

Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 36 +++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index a25b45d8abeb..fb876ddec8a2 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -56,6 +56,7 @@ module_param(debug, bool, 0644);
  * @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 {
@@ -63,6 +64,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;
 };
@@ -184,10 +186,11 @@ 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
+ * @try_lock: indicates if the queue lock should be taken
  *
  * 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)
+static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev, bool try_lock)
 {
 	unsigned long flags;
 
@@ -210,7 +213,20 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
 
 	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
+
+	/*
+	 * A m2m context lock is taken only after a m2m context
+	 * is picked from the queue and marked as running.
+	 * The lock is only needed if v4l2_m2m_try_run is called
+	 * from the async worker.
+	 */
+	if (try_lock && m2m_dev->curr_ctx->q_lock)
+		mutex_lock(m2m_dev->curr_ctx->q_lock);
+
 	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
+
+	if (try_lock && m2m_dev->curr_ctx->q_lock)
+		mutex_unlock(m2m_dev->curr_ctx->q_lock);
 }
 
 /*
@@ -299,10 +315,22 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
 
 	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
-	v4l2_m2m_try_run(m2m_dev);
+	v4l2_m2m_try_run(m2m_dev, false);
 }
 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, true);
+}
+
 /**
  * v4l2_m2m_cancel_job() - cancel pending jobs for the context
  * @m2m_ctx: m2m context with jobs to be canceled
@@ -361,7 +389,8 @@ 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);
+	schedule_work(&m2m_dev->job_work);
 }
 EXPORT_SYMBOL(v4l2_m2m_job_finish);
 
@@ -628,6 +657,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.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] selftests: media_tests: Add a memory-to-memory concurrent stress test

[Ezequiel Garcia]

Add a test for the memory-to-memory framework, to exercise the
scheduling of concurrent jobs, using multiple contexts.

This test needs to be run using the vim2m virtual driver,
and so needs no hardware.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 tools/testing/selftests/media_tests/Makefile  |   4 +-
 .../selftests/media_tests/m2m_job_test.c      | 283 ++++++++++++++++++
 2 files changed, 286 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/media_tests/m2m_job_test.c

diff --git a/tools/testing/selftests/media_tests/Makefile b/tools/testing/selftests/media_tests/Makefile
index 60826d7d37d4..d2e8a6b685fb 100644
--- a/tools/testing/selftests/media_tests/Makefile
+++ b/tools/testing/selftests/media_tests/Makefile
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 #
 CFLAGS += -I../ -I../../../../usr/include/
-TEST_GEN_PROGS := media_device_test media_device_open video_device_test
+TEST_GEN_PROGS := media_device_test media_device_open video_device_test m2m_job_test
 
 include ../lib.mk
+
+LDLIBS += -lpthread
diff --git a/tools/testing/selftests/media_tests/m2m_job_test.c b/tools/testing/selftests/media_tests/m2m_job_test.c
new file mode 100644
index 000000000000..673e7b5cea3a
--- /dev/null
+++ b/tools/testing/selftests/media_tests/m2m_job_test.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) Collabora, Ltd.
+
+/*
+ * This file adds a test for the memory-to-memory
+ * framework.
+ *
+ * This test opens a user specified video device and then
+ * queues concurrent jobs. The jobs are totally dummy,
+ * its purpose is only to verify that each of the queued
+ * jobs is run, and is run only once.
+ *
+ * The vim2m driver is needed in order to run the test.
+ *
+ * Usage:
+ *      ./m2m-job-test -d /dev/videoX
+ */
+
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <time.h>
+#include <pthread.h>
+#include <poll.h>
+
+#include <linux/videodev2.h>
+
+#define V4L2_CID_TRANS_TIME_MSEC        (V4L2_CID_USER_BASE + 0x1000)
+#define V4L2_CID_TRANS_NUM_BUFS         (V4L2_CID_USER_BASE + 0x1001)
+
+#define MAX_TRANS_TIME_MSEC 500
+#define MAX_COUNT 300
+#define MAX_BUFFERS 5
+#define W 100
+#define H 100
+
+#ifndef DEBUG
+#define dprintf(fmt, arg...)			\
+	do {					\
+	} while (0)
+#else
+#define dprintf(fmt, arg...) printf(fmt, ## arg)
+#endif
+
+static char video_device[256];
+static int thread_count;
+
+struct context {
+	int vfd;
+	unsigned int width;
+	unsigned int height;
+	int buffers;
+};
+
+static int req_src_buf(struct context *ctx, int buffers)
+{
+	struct v4l2_requestbuffers reqbuf;
+	struct v4l2_buffer buf;
+	int i, ret;
+
+	memset(&reqbuf, 0, sizeof(reqbuf));
+	memset(&buf, 0, sizeof(buf));
+
+	reqbuf.count	= buffers;
+	reqbuf.type	= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	reqbuf.memory	= V4L2_MEMORY_MMAP;
+	ret = ioctl(ctx->vfd, VIDIOC_REQBUFS, &reqbuf);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < buffers; i++) {
+		buf.type	= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_QUERYBUF, &buf);
+		if (ret)
+			return ret;
+		buf.bytesused = W*H*2;
+		ret = ioctl(ctx->vfd, VIDIOC_QBUF, &buf);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int req_dst_buf(struct context *ctx, int buffers)
+{
+	struct v4l2_requestbuffers reqbuf;
+	struct v4l2_buffer buf;
+	int i, ret;
+
+	memset(&reqbuf, 0, sizeof(reqbuf));
+	memset(&buf, 0, sizeof(buf));
+
+	reqbuf.count	= buffers;
+	reqbuf.type	= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	reqbuf.memory	= V4L2_MEMORY_MMAP;
+
+	ret = ioctl(ctx->vfd, VIDIOC_REQBUFS, &reqbuf);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < buffers; i++) {
+		buf.type	= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_QUERYBUF, &buf);
+		if (ret)
+			return ret;
+		ret = ioctl(ctx->vfd, VIDIOC_QBUF, &buf);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static int streamon(struct context *ctx)
+{
+	enum v4l2_buf_type type;
+	int ret;
+
+	type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	ret = ioctl(ctx->vfd, VIDIOC_STREAMON, &type);
+	if (ret)
+		return ret;
+
+	type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	ret = ioctl(ctx->vfd, VIDIOC_STREAMON, &type);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+static int dqbuf(struct context *ctx)
+{
+	struct v4l2_buffer buf;
+	struct pollfd fds[] = {
+		{ .fd = ctx->vfd, .events = POLLIN },
+	};
+	int i, ret, tout;
+
+	tout = (MAX_TRANS_TIME_MSEC + 10) * thread_count * 2;
+	for (i = 0; i < ctx->buffers; i++) {
+		ret = poll(fds, 1, tout);
+		if (ret == -1) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+
+		if (ret == 0) {
+			dprintf("%s: timeout on %p\n", __func__, ctx);
+			return -1;
+		}
+
+		dprintf("%s: event on %p\n", __func__, ctx);
+
+		memset(&buf, 0, sizeof(buf));
+		buf.type	= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_DQBUF, &buf);
+		if (ret) {
+			dprintf("%s: VIDIOC_DQBUF failed %p\n", __func__, ctx);
+			return ret;
+		}
+
+		memset(&buf, 0, sizeof(buf));
+		buf.type	= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_DQBUF, &buf);
+		if (ret) {
+			dprintf("%s: VIDIOC_DQBUF failed %p\n", __func__, ctx);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void *job(void *arg)
+{
+	struct context *ctx = (struct context *)arg;
+
+	dprintf("%s: running %p\n", __func__, ctx);
+
+	assert(streamon(ctx) == 0);
+	assert(dqbuf(ctx) == 0);
+	assert(dqbuf(ctx) != 0);
+	close(ctx->vfd);
+
+	dprintf("%s: %p done\n", __func__, ctx);
+	return NULL;
+}
+
+static void init(struct context *ctx)
+{
+	struct v4l2_ext_controls ext_ctrls;
+	struct v4l2_ext_control ctrls[2];
+	struct v4l2_capability cap;
+	int ret, buffers;
+
+	memset(ctx, 0, sizeof(*ctx));
+
+	ctx->vfd = open(video_device, O_RDWR | O_NONBLOCK, 0);
+	ctx->width = W;
+	ctx->height = H;
+	assert(ctx->vfd >= 0);
+
+	ret = ioctl(ctx->vfd, VIDIOC_QUERYCAP, &cap);
+	assert(ret == 0);
+	assert(cap.device_caps & V4L2_CAP_VIDEO_M2M);
+	assert(strcmp((const char *)cap.driver, "vim2m") == 0);
+
+	ctrls[0].id = V4L2_CID_TRANS_TIME_MSEC;
+	ctrls[0].value = rand() % MAX_TRANS_TIME_MSEC + 10;
+	ctrls[1].id =  V4L2_CID_TRANS_NUM_BUFS;
+	ctrls[1].value = 1;
+
+	memset(&ext_ctrls, 0, sizeof(ext_ctrls));
+	ext_ctrls.count = 2;
+	ext_ctrls.controls = ctrls;
+	ret = ioctl(ctx->vfd, VIDIOC_S_EXT_CTRLS, &ext_ctrls);
+	assert(ret == 0);
+
+	buffers = rand() % MAX_BUFFERS + 1;
+	assert(req_src_buf(ctx, buffers) == 0);
+	assert(req_dst_buf(ctx, buffers) == 0);
+	ctx->buffers = buffers;
+}
+
+int main(int argc, char * const argv[])
+{
+	int i, opt;
+
+	if (argc < 2) {
+		printf("Usage: %s [-d </dev/videoX>]\n", argv[0]);
+		exit(-1);
+	}
+
+	/* Process arguments */
+	while ((opt = getopt(argc, argv, "d:")) != -1) {
+		switch (opt) {
+		case 'd':
+			strncpy(video_device, optarg, sizeof(video_device) - 1);
+			video_device[sizeof(video_device)-1] = '\0';
+			break;
+		default:
+			printf("Usage: %s [-d </dev/videoX>]\n", argv[0]);
+			exit(-1);
+		}
+	}
+
+	/* Generate random number of interations */
+	srand((unsigned int) time(NULL));
+	thread_count = rand() % MAX_COUNT + 1;
+
+	pthread_t in_thread[thread_count];
+	struct context ctx[thread_count];
+
+	printf("Running %d threads\n", thread_count);
+
+	for (i = 0; i < thread_count; i++)
+		init(&ctx[i]);
+
+	for (i = 0; i < thread_count; i++)
+		pthread_create(&in_thread[i], NULL, job, &ctx[i]);
+
+	for (i = 0; i < thread_count; i++)
+		pthread_join(in_thread[i], NULL);
+
+	return 0;
+}
-- 
2.18.0

[PATCH v2 5/5] selftests: media_tests: Add a memory-to-memory concurrent stress test

[ezequiel]

Add a test for the memory-to-memory framework, to exercise the
scheduling of concurrent jobs, using multiple contexts.

This test needs to be run using the vim2m virtual driver,
and so needs no hardware.

Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
---
 tools/testing/selftests/media_tests/Makefile  |   4 +-
 .../selftests/media_tests/m2m_job_test.c      | 283 ++++++++++++++++++
 2 files changed, 286 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/media_tests/m2m_job_test.c

diff --git a/tools/testing/selftests/media_tests/Makefile b/tools/testing/selftests/media_tests/Makefile
index 60826d7d37d4..d2e8a6b685fb 100644
--- a/tools/testing/selftests/media_tests/Makefile
+++ b/tools/testing/selftests/media_tests/Makefile
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 #
 CFLAGS += -I../ -I../../../../usr/include/
-TEST_GEN_PROGS := media_device_test media_device_open video_device_test
+TEST_GEN_PROGS := media_device_test media_device_open video_device_test m2m_job_test
 
 include ../lib.mk
+
+LDLIBS += -lpthread
diff --git a/tools/testing/selftests/media_tests/m2m_job_test.c b/tools/testing/selftests/media_tests/m2m_job_test.c
new file mode 100644
index 000000000000..673e7b5cea3a
--- /dev/null
+++ b/tools/testing/selftests/media_tests/m2m_job_test.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) Collabora, Ltd.
+
+/*
+ * This file adds a test for the memory-to-memory
+ * framework.
+ *
+ * This test opens a user specified video device and then
+ * queues concurrent jobs. The jobs are totally dummy,
+ * its purpose is only to verify that each of the queued
+ * jobs is run, and is run only once.
+ *
+ * The vim2m driver is needed in order to run the test.
+ *
+ * Usage:
+ *      ./m2m-job-test -d /dev/videoX
+ */
+
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <time.h>
+#include <pthread.h>
+#include <poll.h>
+
+#include <linux/videodev2.h>
+
+#define V4L2_CID_TRANS_TIME_MSEC        (V4L2_CID_USER_BASE + 0x1000)
+#define V4L2_CID_TRANS_NUM_BUFS         (V4L2_CID_USER_BASE + 0x1001)
+
+#define MAX_TRANS_TIME_MSEC 500
+#define MAX_COUNT 300
+#define MAX_BUFFERS 5
+#define W 100
+#define H 100
+
+#ifndef DEBUG
+#define dprintf(fmt, arg...)			\
+	do {					\
+	} while (0)
+#else
+#define dprintf(fmt, arg...) printf(fmt, ## arg)
+#endif
+
+static char video_device[256];
+static int thread_count;
+
+struct context {
+	int vfd;
+	unsigned int width;
+	unsigned int height;
+	int buffers;
+};
+
+static int req_src_buf(struct context *ctx, int buffers)
+{
+	struct v4l2_requestbuffers reqbuf;
+	struct v4l2_buffer buf;
+	int i, ret;
+
+	memset(&reqbuf, 0, sizeof(reqbuf));
+	memset(&buf, 0, sizeof(buf));
+
+	reqbuf.count	= buffers;
+	reqbuf.type	= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	reqbuf.memory	= V4L2_MEMORY_MMAP;
+	ret = ioctl(ctx->vfd, VIDIOC_REQBUFS, &reqbuf);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < buffers; i++) {
+		buf.type	= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_QUERYBUF, &buf);
+		if (ret)
+			return ret;
+		buf.bytesused = W*H*2;
+		ret = ioctl(ctx->vfd, VIDIOC_QBUF, &buf);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int req_dst_buf(struct context *ctx, int buffers)
+{
+	struct v4l2_requestbuffers reqbuf;
+	struct v4l2_buffer buf;
+	int i, ret;
+
+	memset(&reqbuf, 0, sizeof(reqbuf));
+	memset(&buf, 0, sizeof(buf));
+
+	reqbuf.count	= buffers;
+	reqbuf.type	= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	reqbuf.memory	= V4L2_MEMORY_MMAP;
+
+	ret = ioctl(ctx->vfd, VIDIOC_REQBUFS, &reqbuf);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < buffers; i++) {
+		buf.type	= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_QUERYBUF, &buf);
+		if (ret)
+			return ret;
+		ret = ioctl(ctx->vfd, VIDIOC_QBUF, &buf);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static int streamon(struct context *ctx)
+{
+	enum v4l2_buf_type type;
+	int ret;
+
+	type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	ret = ioctl(ctx->vfd, VIDIOC_STREAMON, &type);
+	if (ret)
+		return ret;
+
+	type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	ret = ioctl(ctx->vfd, VIDIOC_STREAMON, &type);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+static int dqbuf(struct context *ctx)
+{
+	struct v4l2_buffer buf;
+	struct pollfd fds[] = {
+		{ .fd = ctx->vfd, .events = POLLIN },
+	};
+	int i, ret, tout;
+
+	tout = (MAX_TRANS_TIME_MSEC + 10) * thread_count * 2;
+	for (i = 0; i < ctx->buffers; i++) {
+		ret = poll(fds, 1, tout);
+		if (ret == -1) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+
+		if (ret == 0) {
+			dprintf("%s: timeout on %p\n", __func__, ctx);
+			return -1;
+		}
+
+		dprintf("%s: event on %p\n", __func__, ctx);
+
+		memset(&buf, 0, sizeof(buf));
+		buf.type	= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_DQBUF, &buf);
+		if (ret) {
+			dprintf("%s: VIDIOC_DQBUF failed %p\n", __func__, ctx);
+			return ret;
+		}
+
+		memset(&buf, 0, sizeof(buf));
+		buf.type	= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_DQBUF, &buf);
+		if (ret) {
+			dprintf("%s: VIDIOC_DQBUF failed %p\n", __func__, ctx);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void *job(void *arg)
+{
+	struct context *ctx = (struct context *)arg;
+
+	dprintf("%s: running %p\n", __func__, ctx);
+
+	assert(streamon(ctx) == 0);
+	assert(dqbuf(ctx) == 0);
+	assert(dqbuf(ctx) != 0);
+	close(ctx->vfd);
+
+	dprintf("%s: %p done\n", __func__, ctx);
+	return NULL;
+}
+
+static void init(struct context *ctx)
+{
+	struct v4l2_ext_controls ext_ctrls;
+	struct v4l2_ext_control ctrls[2];
+	struct v4l2_capability cap;
+	int ret, buffers;
+
+	memset(ctx, 0, sizeof(*ctx));
+
+	ctx->vfd = open(video_device, O_RDWR | O_NONBLOCK, 0);
+	ctx->width = W;
+	ctx->height = H;
+	assert(ctx->vfd >= 0);
+
+	ret = ioctl(ctx->vfd, VIDIOC_QUERYCAP, &cap);
+	assert(ret == 0);
+	assert(cap.device_caps & V4L2_CAP_VIDEO_M2M);
+	assert(strcmp((const char *)cap.driver, "vim2m") == 0);
+
+	ctrls[0].id = V4L2_CID_TRANS_TIME_MSEC;
+	ctrls[0].value = rand() % MAX_TRANS_TIME_MSEC + 10;
+	ctrls[1].id =  V4L2_CID_TRANS_NUM_BUFS;
+	ctrls[1].value = 1;
+
+	memset(&ext_ctrls, 0, sizeof(ext_ctrls));
+	ext_ctrls.count = 2;
+	ext_ctrls.controls = ctrls;
+	ret = ioctl(ctx->vfd, VIDIOC_S_EXT_CTRLS, &ext_ctrls);
+	assert(ret == 0);
+
+	buffers = rand() % MAX_BUFFERS + 1;
+	assert(req_src_buf(ctx, buffers) == 0);
+	assert(req_dst_buf(ctx, buffers) == 0);
+	ctx->buffers = buffers;
+}
+
+int main(int argc, char * const argv[])
+{
+	int i, opt;
+
+	if (argc < 2) {
+		printf("Usage: %s [-d </dev/videoX>]\n", argv[0]);
+		exit(-1);
+	}
+
+	/* Process arguments */
+	while ((opt = getopt(argc, argv, "d:")) != -1) {
+		switch (opt) {
+		case 'd':
+			strncpy(video_device, optarg, sizeof(video_device) - 1);
+			video_device[sizeof(video_device)-1] = '\0';
+			break;
+		default:
+			printf("Usage: %s [-d </dev/videoX>]\n", argv[0]);
+			exit(-1);
+		}
+	}
+
+	/* Generate random number of interations */
+	srand((unsigned int) time(NULL));
+	thread_count = rand() % MAX_COUNT + 1;
+
+	pthread_t in_thread[thread_count];
+	struct context ctx[thread_count];
+
+	printf("Running %d threads\n", thread_count);
+
+	for (i = 0; i < thread_count; i++)
+		init(&ctx[i]);
+
+	for (i = 0; i < thread_count; i++)
+		pthread_create(&in_thread[i], NULL, job, &ctx[i]);
+
+	for (i = 0; i < thread_count; i++)
+		pthread_join(in_thread[i], NULL);
+
+	return 0;
+}
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] selftests: media_tests: Add a memory-to-memory concurrent stress test

[Ezequiel Garcia]

Add a test for the memory-to-memory framework, to exercise the
scheduling of concurrent jobs, using multiple contexts.

This test needs to be run using the vim2m virtual driver,
and so needs no hardware.

Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
---
 tools/testing/selftests/media_tests/Makefile  |   4 +-
 .../selftests/media_tests/m2m_job_test.c      | 283 ++++++++++++++++++
 2 files changed, 286 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/media_tests/m2m_job_test.c

diff --git a/tools/testing/selftests/media_tests/Makefile b/tools/testing/selftests/media_tests/Makefile
index 60826d7d37d4..d2e8a6b685fb 100644
--- a/tools/testing/selftests/media_tests/Makefile
+++ b/tools/testing/selftests/media_tests/Makefile
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 #
 CFLAGS += -I../ -I../../../../usr/include/
-TEST_GEN_PROGS := media_device_test media_device_open video_device_test
+TEST_GEN_PROGS := media_device_test media_device_open video_device_test m2m_job_test
 
 include ../lib.mk
+
+LDLIBS += -lpthread
diff --git a/tools/testing/selftests/media_tests/m2m_job_test.c b/tools/testing/selftests/media_tests/m2m_job_test.c
new file mode 100644
index 000000000000..673e7b5cea3a
--- /dev/null
+++ b/tools/testing/selftests/media_tests/m2m_job_test.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) Collabora, Ltd.
+
+/*
+ * This file adds a test for the memory-to-memory
+ * framework.
+ *
+ * This test opens a user specified video device and then
+ * queues concurrent jobs. The jobs are totally dummy,
+ * its purpose is only to verify that each of the queued
+ * jobs is run, and is run only once.
+ *
+ * The vim2m driver is needed in order to run the test.
+ *
+ * Usage:
+ *      ./m2m-job-test -d /dev/videoX
+ */
+
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <time.h>
+#include <pthread.h>
+#include <poll.h>
+
+#include <linux/videodev2.h>
+
+#define V4L2_CID_TRANS_TIME_MSEC        (V4L2_CID_USER_BASE + 0x1000)
+#define V4L2_CID_TRANS_NUM_BUFS         (V4L2_CID_USER_BASE + 0x1001)
+
+#define MAX_TRANS_TIME_MSEC 500
+#define MAX_COUNT 300
+#define MAX_BUFFERS 5
+#define W 100
+#define H 100
+
+#ifndef DEBUG
+#define dprintf(fmt, arg...)			\
+	do {					\
+	} while (0)
+#else
+#define dprintf(fmt, arg...) printf(fmt, ## arg)
+#endif
+
+static char video_device[256];
+static int thread_count;
+
+struct context {
+	int vfd;
+	unsigned int width;
+	unsigned int height;
+	int buffers;
+};
+
+static int req_src_buf(struct context *ctx, int buffers)
+{
+	struct v4l2_requestbuffers reqbuf;
+	struct v4l2_buffer buf;
+	int i, ret;
+
+	memset(&reqbuf, 0, sizeof(reqbuf));
+	memset(&buf, 0, sizeof(buf));
+
+	reqbuf.count	= buffers;
+	reqbuf.type	= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	reqbuf.memory	= V4L2_MEMORY_MMAP;
+	ret = ioctl(ctx->vfd, VIDIOC_REQBUFS, &reqbuf);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < buffers; i++) {
+		buf.type	= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_QUERYBUF, &buf);
+		if (ret)
+			return ret;
+		buf.bytesused = W*H*2;
+		ret = ioctl(ctx->vfd, VIDIOC_QBUF, &buf);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int req_dst_buf(struct context *ctx, int buffers)
+{
+	struct v4l2_requestbuffers reqbuf;
+	struct v4l2_buffer buf;
+	int i, ret;
+
+	memset(&reqbuf, 0, sizeof(reqbuf));
+	memset(&buf, 0, sizeof(buf));
+
+	reqbuf.count	= buffers;
+	reqbuf.type	= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	reqbuf.memory	= V4L2_MEMORY_MMAP;
+
+	ret = ioctl(ctx->vfd, VIDIOC_REQBUFS, &reqbuf);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < buffers; i++) {
+		buf.type	= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_QUERYBUF, &buf);
+		if (ret)
+			return ret;
+		ret = ioctl(ctx->vfd, VIDIOC_QBUF, &buf);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static int streamon(struct context *ctx)
+{
+	enum v4l2_buf_type type;
+	int ret;
+
+	type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	ret = ioctl(ctx->vfd, VIDIOC_STREAMON, &type);
+	if (ret)
+		return ret;
+
+	type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	ret = ioctl(ctx->vfd, VIDIOC_STREAMON, &type);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+static int dqbuf(struct context *ctx)
+{
+	struct v4l2_buffer buf;
+	struct pollfd fds[] = {
+		{ .fd = ctx->vfd, .events = POLLIN },
+	};
+	int i, ret, tout;
+
+	tout = (MAX_TRANS_TIME_MSEC + 10) * thread_count * 2;
+	for (i = 0; i < ctx->buffers; i++) {
+		ret = poll(fds, 1, tout);
+		if (ret == -1) {
+			if (errno == EINTR)
+				continue;
+			return -1;
+		}
+
+		if (ret == 0) {
+			dprintf("%s: timeout on %p\n", __func__, ctx);
+			return -1;
+		}
+
+		dprintf("%s: event on %p\n", __func__, ctx);
+
+		memset(&buf, 0, sizeof(buf));
+		buf.type	= V4L2_BUF_TYPE_VIDEO_OUTPUT;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_DQBUF, &buf);
+		if (ret) {
+			dprintf("%s: VIDIOC_DQBUF failed %p\n", __func__, ctx);
+			return ret;
+		}
+
+		memset(&buf, 0, sizeof(buf));
+		buf.type	= V4L2_BUF_TYPE_VIDEO_CAPTURE;
+		buf.memory	= V4L2_MEMORY_MMAP;
+		buf.index	= i;
+		ret = ioctl(ctx->vfd, VIDIOC_DQBUF, &buf);
+		if (ret) {
+			dprintf("%s: VIDIOC_DQBUF failed %p\n", __func__, ctx);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void *job(void *arg)
+{
+	struct context *ctx = (struct context *)arg;
+
+	dprintf("%s: running %p\n", __func__, ctx);
+
+	assert(streamon(ctx) == 0);
+	assert(dqbuf(ctx) == 0);
+	assert(dqbuf(ctx) != 0);
+	close(ctx->vfd);
+
+	dprintf("%s: %p done\n", __func__, ctx);
+	return NULL;
+}
+
+static void init(struct context *ctx)
+{
+	struct v4l2_ext_controls ext_ctrls;
+	struct v4l2_ext_control ctrls[2];
+	struct v4l2_capability cap;
+	int ret, buffers;
+
+	memset(ctx, 0, sizeof(*ctx));
+
+	ctx->vfd = open(video_device, O_RDWR | O_NONBLOCK, 0);
+	ctx->width = W;
+	ctx->height = H;
+	assert(ctx->vfd >= 0);
+
+	ret = ioctl(ctx->vfd, VIDIOC_QUERYCAP, &cap);
+	assert(ret == 0);
+	assert(cap.device_caps & V4L2_CAP_VIDEO_M2M);
+	assert(strcmp((const char *)cap.driver, "vim2m") == 0);
+
+	ctrls[0].id = V4L2_CID_TRANS_TIME_MSEC;
+	ctrls[0].value = rand() % MAX_TRANS_TIME_MSEC + 10;
+	ctrls[1].id =  V4L2_CID_TRANS_NUM_BUFS;
+	ctrls[1].value = 1;
+
+	memset(&ext_ctrls, 0, sizeof(ext_ctrls));
+	ext_ctrls.count = 2;
+	ext_ctrls.controls = ctrls;
+	ret = ioctl(ctx->vfd, VIDIOC_S_EXT_CTRLS, &ext_ctrls);
+	assert(ret == 0);
+
+	buffers = rand() % MAX_BUFFERS + 1;
+	assert(req_src_buf(ctx, buffers) == 0);
+	assert(req_dst_buf(ctx, buffers) == 0);
+	ctx->buffers = buffers;
+}
+
+int main(int argc, char * const argv[])
+{
+	int i, opt;
+
+	if (argc < 2) {
+		printf("Usage: %s [-d </dev/videoX>]\n", argv[0]);
+		exit(-1);
+	}
+
+	/* Process arguments */
+	while ((opt = getopt(argc, argv, "d:")) != -1) {
+		switch (opt) {
+		case 'd':
+			strncpy(video_device, optarg, sizeof(video_device) - 1);
+			video_device[sizeof(video_device)-1] = '\0';
+			break;
+		default:
+			printf("Usage: %s [-d </dev/videoX>]\n", argv[0]);
+			exit(-1);
+		}
+	}
+
+	/* Generate random number of interations */
+	srand((unsigned int) time(NULL));
+	thread_count = rand() % MAX_COUNT + 1;
+
+	pthread_t in_thread[thread_count];
+	struct context ctx[thread_count];
+
+	printf("Running %d threads\n", thread_count);
+
+	for (i = 0; i < thread_count; i++)
+		init(&ctx[i]);
+
+	for (i = 0; i < thread_count; i++)
+		pthread_create(&in_thread[i], NULL, job, &ctx[i]);
+
+	for (i = 0; i < thread_count; i++)
+		pthread_join(in_thread[i], NULL);
+
+	return 0;
+}
-- 
2.18.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Hans Verkuil]

On 25/07/18 19:15, Ezequiel Garcia wrote:
> Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> removed a redundant call to v4l2_m2m_try_run but instead introduced
> a bug. Consider the following case:
> 
>  1) Context A schedules, queues and runs job A.
>  2) While the m2m device is running, context B schedules
>     and queues job B. Job B cannot run, because it has to
>     wait for job A.
>  3) Job A completes, calls v4l2_m2m_job_finish, and tries
>     to queue a job for context A, but since the context is
>     empty it won't do anything.
> 
> In this scenario, queued job B will never run. Fix this by calling
> v4l2_m2m_try_run from v4l2_m2m_try_schedule.
> 
> While here, add more documentation to these functions.
> 
> Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

So just to be clear: this first patch fixes a regression and can be applied
separately from the other patches?

> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 5f9cd5b74cda..dfd796621b06 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
>  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>  
> +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);

Is this intended? It feels out of place in this patch.

>  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>  }
>  
> -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> +/*
> + * __v4l2_m2m_try_queue() - queue a job
> + * @m2m_dev: m2m device
> + * @m2m_ctx: m2m context
> + *
> + * Check if this context is ready to queue a job.
> + *
> + * This function can run in interrupt context.
> + */
> +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
>  {
> -	struct v4l2_m2m_dev *m2m_dev;
>  	unsigned long flags_job, flags_out, flags_cap;
>  
> -	m2m_dev = m2m_ctx->m2m_dev;
>  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
>  
>  	if (!m2m_ctx->out_q_ctx.q.streaming
> @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>  	m2m_ctx->job_flags |= TRANS_QUEUED;
>  
>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
> +}
> +
> +/**
> + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
> + * @m2m_ctx: m2m context
> + *
> + * Check if this context is ready to queue a job. If suitable,
> + * run the next queued job on the mem2mem device.
> + *
> + * This function shouldn't run in interrupt context.
> + *
> + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
> + * and then run another job for another context.
> + */
> +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> +{
> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>  
> +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);

Why introduce __v4l2_m2m_try_queue? Why not keep it in here?

>  	v4l2_m2m_try_run(m2m_dev);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
> 

Regards,

	Hans

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[hverkuil]

On 25/07/18 19:15, Ezequiel Garcia wrote:
> Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> removed a redundant call to v4l2_m2m_try_run but instead introduced
> a bug. Consider the following case:
> 
>  1) Context A schedules, queues and runs job A.
>  2) While the m2m device is running, context B schedules
>     and queues job B. Job B cannot run, because it has to
>     wait for job A.
>  3) Job A completes, calls v4l2_m2m_job_finish, and tries
>     to queue a job for context A, but since the context is
>     empty it won't do anything.
> 
> In this scenario, queued job B will never run. Fix this by calling
> v4l2_m2m_try_run from v4l2_m2m_try_schedule.
> 
> While here, add more documentation to these functions.
> 
> Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>

So just to be clear: this first patch fixes a regression and can be applied
separately from the other patches?

> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 5f9cd5b74cda..dfd796621b06 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
>  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>  
> +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);

Is this intended? It feels out of place in this patch.

>  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>  }
>  
> -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> +/*
> + * __v4l2_m2m_try_queue() - queue a job
> + * @m2m_dev: m2m device
> + * @m2m_ctx: m2m context
> + *
> + * Check if this context is ready to queue a job.
> + *
> + * This function can run in interrupt context.
> + */
> +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
>  {
> -	struct v4l2_m2m_dev *m2m_dev;
>  	unsigned long flags_job, flags_out, flags_cap;
>  
> -	m2m_dev = m2m_ctx->m2m_dev;
>  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
>  
>  	if (!m2m_ctx->out_q_ctx.q.streaming
> @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>  	m2m_ctx->job_flags |= TRANS_QUEUED;
>  
>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
> +}
> +
> +/**
> + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
> + * @m2m_ctx: m2m context
> + *
> + * Check if this context is ready to queue a job. If suitable,
> + * run the next queued job on the mem2mem device.
> + *
> + * This function shouldn't run in interrupt context.
> + *
> + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
> + * and then run another job for another context.
> + */
> +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> +{
> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>  
> +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);

Why introduce __v4l2_m2m_try_queue? Why not keep it in here?

>  	v4l2_m2m_try_run(m2m_dev);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
> 

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Hans Verkuil]

On 25/07/18 19:15, Ezequiel Garcia wrote:
> Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> removed a redundant call to v4l2_m2m_try_run but instead introduced
> a bug. Consider the following case:
> 
>  1) Context A schedules, queues and runs job A.
>  2) While the m2m device is running, context B schedules
>     and queues job B. Job B cannot run, because it has to
>     wait for job A.
>  3) Job A completes, calls v4l2_m2m_job_finish, and tries
>     to queue a job for context A, but since the context is
>     empty it won't do anything.
> 
> In this scenario, queued job B will never run. Fix this by calling
> v4l2_m2m_try_run from v4l2_m2m_try_schedule.
> 
> While here, add more documentation to these functions.
> 
> Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>

So just to be clear: this first patch fixes a regression and can be applied
separately from the other patches?

> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 5f9cd5b74cda..dfd796621b06 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
>  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>  
> +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);

Is this intended? It feels out of place in this patch.

>  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>  }
>  
> -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> +/*
> + * __v4l2_m2m_try_queue() - queue a job
> + * @m2m_dev: m2m device
> + * @m2m_ctx: m2m context
> + *
> + * Check if this context is ready to queue a job.
> + *
> + * This function can run in interrupt context.
> + */
> +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
>  {
> -	struct v4l2_m2m_dev *m2m_dev;
>  	unsigned long flags_job, flags_out, flags_cap;
>  
> -	m2m_dev = m2m_ctx->m2m_dev;
>  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
>  
>  	if (!m2m_ctx->out_q_ctx.q.streaming
> @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>  	m2m_ctx->job_flags |= TRANS_QUEUED;
>  
>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
> +}
> +
> +/**
> + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
> + * @m2m_ctx: m2m context
> + *
> + * Check if this context is ready to queue a job. If suitable,
> + * run the next queued job on the mem2mem device.
> + *
> + * This function shouldn't run in interrupt context.
> + *
> + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
> + * and then run another job for another context.
> + */
> +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> +{
> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>  
> +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);

Why introduce __v4l2_m2m_try_queue? Why not keep it in here?

>  	v4l2_m2m_try_run(m2m_dev);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
> 

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Hans Verkuil]

On 27/07/18 15:35, Hans Verkuil wrote:
> On 25/07/18 19:15, Ezequiel Garcia wrote:
>> Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>> removed a redundant call to v4l2_m2m_try_run but instead introduced
>> a bug. Consider the following case:
>>
>>  1) Context A schedules, queues and runs job A.
>>  2) While the m2m device is running, context B schedules
>>     and queues job B. Job B cannot run, because it has to
>>     wait for job A.
>>  3) Job A completes, calls v4l2_m2m_job_finish, and tries
>>     to queue a job for context A, but since the context is
>>     empty it won't do anything.
>>
>> In this scenario, queued job B will never run. Fix this by calling
>> v4l2_m2m_try_run from v4l2_m2m_try_schedule.
>>
>> While here, add more documentation to these functions.
>>
>> Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> 
> So just to be clear: this first patch fixes a regression and can be applied
> separately from the other patches?
> 
>> ---
>>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
>>  1 file changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> index 5f9cd5b74cda..dfd796621b06 100644
>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
>>  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>>  
>> +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
> 
> Is this intended? It feels out of place in this patch.
> 
>>  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>>  }
>>  
>> -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>> +/*
>> + * __v4l2_m2m_try_queue() - queue a job
>> + * @m2m_dev: m2m device
>> + * @m2m_ctx: m2m context
>> + *
>> + * Check if this context is ready to queue a job.
>> + *
>> + * This function can run in interrupt context.
>> + */
>> +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
>>  {
>> -	struct v4l2_m2m_dev *m2m_dev;
>>  	unsigned long flags_job, flags_out, flags_cap;
>>  
>> -	m2m_dev = m2m_ctx->m2m_dev;
>>  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
>>  
>>  	if (!m2m_ctx->out_q_ctx.q.streaming
>> @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>  	m2m_ctx->job_flags |= TRANS_QUEUED;
>>  
>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
>> +}
>> +
>> +/**
>> + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
>> + * @m2m_ctx: m2m context
>> + *
>> + * Check if this context is ready to queue a job. If suitable,
>> + * run the next queued job on the mem2mem device.
>> + *
>> + * This function shouldn't run in interrupt context.
>> + *
>> + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
>> + * and then run another job for another context.
>> + */
>> +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>> +{
>> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>>  
>> +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> 
> Why introduce __v4l2_m2m_try_queue? Why not keep it in here?
> 
>>  	v4l2_m2m_try_run(m2m_dev);

I'm completely confused: v4l2_m2m_try_schedule() already calls v4l2_m2m_try_run()!

Either my brain has crashed due to the heatwave I'm suffering through, or there
is something amiss with this patch.

Regards,

	Hans

>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
>>
> 
> Regards,
> 
> 	Hans
> 

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[hverkuil]

On 27/07/18 15:35, Hans Verkuil wrote:
> On 25/07/18 19:15, Ezequiel Garcia wrote:
>> Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>> removed a redundant call to v4l2_m2m_try_run but instead introduced
>> a bug. Consider the following case:
>>
>>  1) Context A schedules, queues and runs job A.
>>  2) While the m2m device is running, context B schedules
>>     and queues job B. Job B cannot run, because it has to
>>     wait for job A.
>>  3) Job A completes, calls v4l2_m2m_job_finish, and tries
>>     to queue a job for context A, but since the context is
>>     empty it won't do anything.
>>
>> In this scenario, queued job B will never run. Fix this by calling
>> v4l2_m2m_try_run from v4l2_m2m_try_schedule.
>>
>> While here, add more documentation to these functions.
>>
>> Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>> Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
> 
> So just to be clear: this first patch fixes a regression and can be applied
> separately from the other patches?
> 
>> ---
>>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
>>  1 file changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> index 5f9cd5b74cda..dfd796621b06 100644
>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
>>  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>>  
>> +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
> 
> Is this intended? It feels out of place in this patch.
> 
>>  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>>  }
>>  
>> -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>> +/*
>> + * __v4l2_m2m_try_queue() - queue a job
>> + * @m2m_dev: m2m device
>> + * @m2m_ctx: m2m context
>> + *
>> + * Check if this context is ready to queue a job.
>> + *
>> + * This function can run in interrupt context.
>> + */
>> +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
>>  {
>> -	struct v4l2_m2m_dev *m2m_dev;
>>  	unsigned long flags_job, flags_out, flags_cap;
>>  
>> -	m2m_dev = m2m_ctx->m2m_dev;
>>  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
>>  
>>  	if (!m2m_ctx->out_q_ctx.q.streaming
>> @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>  	m2m_ctx->job_flags |= TRANS_QUEUED;
>>  
>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
>> +}
>> +
>> +/**
>> + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
>> + * @m2m_ctx: m2m context
>> + *
>> + * Check if this context is ready to queue a job. If suitable,
>> + * run the next queued job on the mem2mem device.
>> + *
>> + * This function shouldn't run in interrupt context.
>> + *
>> + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
>> + * and then run another job for another context.
>> + */
>> +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>> +{
>> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>>  
>> +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> 
> Why introduce __v4l2_m2m_try_queue? Why not keep it in here?
> 
>>  	v4l2_m2m_try_run(m2m_dev);

I'm completely confused: v4l2_m2m_try_schedule() already calls v4l2_m2m_try_run()!

Either my brain has crashed due to the heatwave I'm suffering through, or there
is something amiss with this patch.

Regards,

	Hans

>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
>>
> 
> Regards,
> 
> 	Hans
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Hans Verkuil]

On 27/07/18 15:35, Hans Verkuil wrote:
> On 25/07/18 19:15, Ezequiel Garcia wrote:
>> Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>> removed a redundant call to v4l2_m2m_try_run but instead introduced
>> a bug. Consider the following case:
>>
>>  1) Context A schedules, queues and runs job A.
>>  2) While the m2m device is running, context B schedules
>>     and queues job B. Job B cannot run, because it has to
>>     wait for job A.
>>  3) Job A completes, calls v4l2_m2m_job_finish, and tries
>>     to queue a job for context A, but since the context is
>>     empty it won't do anything.
>>
>> In this scenario, queued job B will never run. Fix this by calling
>> v4l2_m2m_try_run from v4l2_m2m_try_schedule.
>>
>> While here, add more documentation to these functions.
>>
>> Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>> Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
> 
> So just to be clear: this first patch fixes a regression and can be applied
> separately from the other patches?
> 
>> ---
>>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
>>  1 file changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> index 5f9cd5b74cda..dfd796621b06 100644
>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
>>  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>>  
>> +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
> 
> Is this intended? It feels out of place in this patch.
> 
>>  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>>  }
>>  
>> -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>> +/*
>> + * __v4l2_m2m_try_queue() - queue a job
>> + * @m2m_dev: m2m device
>> + * @m2m_ctx: m2m context
>> + *
>> + * Check if this context is ready to queue a job.
>> + *
>> + * This function can run in interrupt context.
>> + */
>> +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
>>  {
>> -	struct v4l2_m2m_dev *m2m_dev;
>>  	unsigned long flags_job, flags_out, flags_cap;
>>  
>> -	m2m_dev = m2m_ctx->m2m_dev;
>>  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
>>  
>>  	if (!m2m_ctx->out_q_ctx.q.streaming
>> @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>  	m2m_ctx->job_flags |= TRANS_QUEUED;
>>  
>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
>> +}
>> +
>> +/**
>> + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
>> + * @m2m_ctx: m2m context
>> + *
>> + * Check if this context is ready to queue a job. If suitable,
>> + * run the next queued job on the mem2mem device.
>> + *
>> + * This function shouldn't run in interrupt context.
>> + *
>> + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
>> + * and then run another job for another context.
>> + */
>> +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>> +{
>> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>>  
>> +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> 
> Why introduce __v4l2_m2m_try_queue? Why not keep it in here?
> 
>>  	v4l2_m2m_try_run(m2m_dev);

I'm completely confused: v4l2_m2m_try_schedule() already calls v4l2_m2m_try_run()!

Either my brain has crashed due to the heatwave I'm suffering through, or there
is something amiss with this patch.

Regards,

	Hans

>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
>>
> 
> Regards,
> 
> 	Hans
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Ezequiel Garcia]

On Fri, 2018-07-27 at 15:41 +0200, Hans Verkuil wrote:
> On 27/07/18 15:35, Hans Verkuil wrote:
> > On 25/07/18 19:15, Ezequiel Garcia wrote:
> > > Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> > > removed a redundant call to v4l2_m2m_try_run but instead introduced
> > > a bug. Consider the following case:
> > > 
> > >  1) Context A schedules, queues and runs job A.
> > >  2) While the m2m device is running, context B schedules
> > >     and queues job B. Job B cannot run, because it has to
> > >     wait for job A.
> > >  3) Job A completes, calls v4l2_m2m_job_finish, and tries
> > >     to queue a job for context A, but since the context is
> > >     empty it won't do anything.
> > > 
> > > In this scenario, queued job B will never run. Fix this by calling
> > > v4l2_m2m_try_run from v4l2_m2m_try_schedule.
> > > 
> > > While here, add more documentation to these functions.
> > > 
> > > Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > 
> > So just to be clear: this first patch fixes a regression and can be applied
> > separately from the other patches?
> > 

That is correct, it's a regression and can be applied independently.

This patch has been tested independently of the rest of the series,
using the test introduced in "selftests: media_tests: Add a
memory-to-memory concurrent stress test".

Without this patch, the test fails.

> > > ---
> > >  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
> > >  1 file changed, 29 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > index 5f9cd5b74cda..dfd796621b06 100644
> > > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
> > >  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
> > >  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> > >  
> > > +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
> > 
> > Is this intended? It feels out of place in this patch.
> > 

It was intended :) Since the patch is adding some more documentation,
it felt OK to add this debug message, as it also helps the testing.

But I can drop it if you think it's out of place.

> > >  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
> > >  }
> > >  
> > > -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> > > +/*
> > > + * __v4l2_m2m_try_queue() - queue a job
> > > + * @m2m_dev: m2m device
> > > + * @m2m_ctx: m2m context
> > > + *
> > > + * Check if this context is ready to queue a job.
> > > + *
> > > + * This function can run in interrupt context.
> > > + */
> > > +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
> > >  {
> > > -	struct v4l2_m2m_dev *m2m_dev;
> > >  	unsigned long flags_job, flags_out, flags_cap;
> > >  
> > > -	m2m_dev = m2m_ctx->m2m_dev;
> > >  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
> > >  
> > >  	if (!m2m_ctx->out_q_ctx.q.streaming
> > > @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> > >  	m2m_ctx->job_flags |= TRANS_QUEUED;
> > >  
> > >  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
> > > +}
> > > +
> > > +/**
> > > + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
> > > + * @m2m_ctx: m2m context
> > > + *
> > > + * Check if this context is ready to queue a job. If suitable,
> > > + * run the next queued job on the mem2mem device.
> > > + *
> > > + * This function shouldn't run in interrupt context.
> > > + *
> > > + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
> > > + * and then run another job for another context.
> > > + */
> > > +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> > > +{
> > > +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
> > >  
> > > +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> > 
> > Why introduce __v4l2_m2m_try_queue? Why not keep it in here?
> > 

Well, specifically we need to make sure xxx_try_run is called
even if no job was queued. After a lot of back and forth, I ended
up thinking the queue and run operations were different and needed
different functions.

If we were to keep the code in try_schedule, we would have to use
some extra conditionals or gotos to make sure try_run is called even
if no job was queued.

The important thing to keep in mind here is that these functions
could be called on a given context, but then actually run a job
for another context.

> > >  	v4l2_m2m_try_run(m2m_dev);
> 
> I'm completely confused: v4l2_m2m_try_schedule() already calls v4l2_m2m_try_run()!
> 
> Either my brain has crashed due to the heatwave I'm suffering through, or there
> is something amiss with this patch.
> 
> 

Right, v4l2_m2m_try_schedule calls v4l2_m2m_try_run *if* a job was queued.
This might not always be the case.

Consider you schedule two jobs, both will be queued but only one will run.
When will the second job run? Answer: never :)

Hope it's clear now!
Eze

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[ezequiel]

On Fri, 2018-07-27 at 15:41 +0200, Hans Verkuil wrote:
> On 27/07/18 15:35, Hans Verkuil wrote:
> > On 25/07/18 19:15, Ezequiel Garcia wrote:
> > > Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> > > removed a redundant call to v4l2_m2m_try_run but instead introduced
> > > a bug. Consider the following case:
> > > 
> > >  1) Context A schedules, queues and runs job A.
> > >  2) While the m2m device is running, context B schedules
> > >     and queues job B. Job B cannot run, because it has to
> > >     wait for job A.
> > >  3) Job A completes, calls v4l2_m2m_job_finish, and tries
> > >     to queue a job for context A, but since the context is
> > >     empty it won't do anything.
> > > 
> > > In this scenario, queued job B will never run. Fix this by calling
> > > v4l2_m2m_try_run from v4l2_m2m_try_schedule.
> > > 
> > > While here, add more documentation to these functions.
> > > 
> > > Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> > > Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
> > 
> > So just to be clear: this first patch fixes a regression and can be applied
> > separately from the other patches?
> > 

That is correct, it's a regression and can be applied independently.

This patch has been tested independently of the rest of the series,
using the test introduced in "selftests: media_tests: Add a
memory-to-memory concurrent stress test".

Without this patch, the test fails.

> > > ---
> > >  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
> > >  1 file changed, 29 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > index 5f9cd5b74cda..dfd796621b06 100644
> > > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
> > >  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
> > >  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> > >  
> > > +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
> > 
> > Is this intended? It feels out of place in this patch.
> > 

It was intended :) Since the patch is adding some more documentation,
it felt OK to add this debug message, as it also helps the testing.

But I can drop it if you think it's out of place.

> > >  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
> > >  }
> > >  
> > > -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> > > +/*
> > > + * __v4l2_m2m_try_queue() - queue a job
> > > + * @m2m_dev: m2m device
> > > + * @m2m_ctx: m2m context
> > > + *
> > > + * Check if this context is ready to queue a job.
> > > + *
> > > + * This function can run in interrupt context.
> > > + */
> > > +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
> > >  {
> > > -	struct v4l2_m2m_dev *m2m_dev;
> > >  	unsigned long flags_job, flags_out, flags_cap;
> > >  
> > > -	m2m_dev = m2m_ctx->m2m_dev;
> > >  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
> > >  
> > >  	if (!m2m_ctx->out_q_ctx.q.streaming
> > > @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> > >  	m2m_ctx->job_flags |= TRANS_QUEUED;
> > >  
> > >  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
> > > +}
> > > +
> > > +/**
> > > + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
> > > + * @m2m_ctx: m2m context
> > > + *
> > > + * Check if this context is ready to queue a job. If suitable,
> > > + * run the next queued job on the mem2mem device.
> > > + *
> > > + * This function shouldn't run in interrupt context.
> > > + *
> > > + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
> > > + * and then run another job for another context.
> > > + */
> > > +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> > > +{
> > > +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
> > >  
> > > +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> > 
> > Why introduce __v4l2_m2m_try_queue? Why not keep it in here?
> > 

Well, specifically we need to make sure xxx_try_run is called
even if no job was queued. After a lot of back and forth, I ended
up thinking the queue and run operations were different and needed
different functions.

If we were to keep the code in try_schedule, we would have to use
some extra conditionals or gotos to make sure try_run is called even
if no job was queued.

The important thing to keep in mind here is that these functions
could be called on a given context, but then actually run a job
for another context.

> > >  	v4l2_m2m_try_run(m2m_dev);
> 
> I'm completely confused: v4l2_m2m_try_schedule() already calls v4l2_m2m_try_run()!
> 
> Either my brain has crashed due to the heatwave I'm suffering through, or there
> is something amiss with this patch.
> 
> 

Right, v4l2_m2m_try_schedule calls v4l2_m2m_try_run *if* a job was queued.
This might not always be the case.

Consider you schedule two jobs, both will be queued but only one will run.
When will the second job run? Answer: never :)

Hope it's clear now!
Eze

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Ezequiel Garcia]

On Fri, 2018-07-27@15:41 +0200, Hans Verkuil wrote:
> On 27/07/18 15:35, Hans Verkuil wrote:
> > On 25/07/18 19:15, Ezequiel Garcia wrote:
> > > Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> > > removed a redundant call to v4l2_m2m_try_run but instead introduced
> > > a bug. Consider the following case:
> > > 
> > >  1) Context A schedules, queues and runs job A.
> > >  2) While the m2m device is running, context B schedules
> > >     and queues job B. Job B cannot run, because it has to
> > >     wait for job A.
> > >  3) Job A completes, calls v4l2_m2m_job_finish, and tries
> > >     to queue a job for context A, but since the context is
> > >     empty it won't do anything.
> > > 
> > > In this scenario, queued job B will never run. Fix this by calling
> > > v4l2_m2m_try_run from v4l2_m2m_try_schedule.
> > > 
> > > While here, add more documentation to these functions.
> > > 
> > > Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
> > > Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
> > 
> > So just to be clear: this first patch fixes a regression and can be applied
> > separately from the other patches?
> > 

That is correct, it's a regression and can be applied independently.

This patch has been tested independently of the rest of the series,
using the test introduced in "selftests: media_tests: Add a
memory-to-memory concurrent stress test".

Without this patch, the test fails.

> > > ---
> > >  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
> > >  1 file changed, 29 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > index 5f9cd5b74cda..dfd796621b06 100644
> > > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > > @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
> > >  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
> > >  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> > >  
> > > +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
> > 
> > Is this intended? It feels out of place in this patch.
> > 

It was intended :) Since the patch is adding some more documentation,
it felt OK to add this debug message, as it also helps the testing.

But I can drop it if you think it's out of place.

> > >  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
> > >  }
> > >  
> > > -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> > > +/*
> > > + * __v4l2_m2m_try_queue() - queue a job
> > > + * @m2m_dev: m2m device
> > > + * @m2m_ctx: m2m context
> > > + *
> > > + * Check if this context is ready to queue a job.
> > > + *
> > > + * This function can run in interrupt context.
> > > + */
> > > +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
> > >  {
> > > -	struct v4l2_m2m_dev *m2m_dev;
> > >  	unsigned long flags_job, flags_out, flags_cap;
> > >  
> > > -	m2m_dev = m2m_ctx->m2m_dev;
> > >  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
> > >  
> > >  	if (!m2m_ctx->out_q_ctx.q.streaming
> > > @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> > >  	m2m_ctx->job_flags |= TRANS_QUEUED;
> > >  
> > >  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
> > > +}
> > > +
> > > +/**
> > > + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
> > > + * @m2m_ctx: m2m context
> > > + *
> > > + * Check if this context is ready to queue a job. If suitable,
> > > + * run the next queued job on the mem2mem device.
> > > + *
> > > + * This function shouldn't run in interrupt context.
> > > + *
> > > + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
> > > + * and then run another job for another context.
> > > + */
> > > +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
> > > +{
> > > +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
> > >  
> > > +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> > 
> > Why introduce __v4l2_m2m_try_queue? Why not keep it in here?
> > 

Well, specifically we need to make sure xxx_try_run is called
even if no job was queued. After a lot of back and forth, I ended
up thinking the queue and run operations were different and needed
different functions.

If we were to keep the code in try_schedule, we would have to use
some extra conditionals or gotos to make sure try_run is called even
if no job was queued.

The important thing to keep in mind here is that these functions
could be called on a given context, but then actually run a job
for another context.

> > >  	v4l2_m2m_try_run(m2m_dev);
> 
> I'm completely confused: v4l2_m2m_try_schedule() already calls v4l2_m2m_try_run()!
> 
> Either my brain has crashed due to the heatwave I'm suffering through, or there
> is something amiss with this patch.
> 
> 

Right, v4l2_m2m_try_schedule calls v4l2_m2m_try_run *if* a job was queued.
This might not always be the case.

Consider you schedule two jobs, both will be queued but only one will run.
When will the second job run? Answer: never :)

Hope it's clear now!
Eze

--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Hans Verkuil]

On 27/07/18 17:16, Ezequiel Garcia wrote:
> On Fri, 2018-07-27 at 15:41 +0200, Hans Verkuil wrote:
>> On 27/07/18 15:35, Hans Verkuil wrote:
>>> On 25/07/18 19:15, Ezequiel Garcia wrote:
>>>> Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>>>> removed a redundant call to v4l2_m2m_try_run but instead introduced
>>>> a bug. Consider the following case:
>>>>
>>>>  1) Context A schedules, queues and runs job A.
>>>>  2) While the m2m device is running, context B schedules
>>>>     and queues job B. Job B cannot run, because it has to
>>>>     wait for job A.
>>>>  3) Job A completes, calls v4l2_m2m_job_finish, and tries
>>>>     to queue a job for context A, but since the context is
>>>>     empty it won't do anything.
>>>>
>>>> In this scenario, queued job B will never run. Fix this by calling
>>>> v4l2_m2m_try_run from v4l2_m2m_try_schedule.
>>>>
>>>> While here, add more documentation to these functions.
>>>>
>>>> Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>>
>>> So just to be clear: this first patch fixes a regression and can be applied
>>> separately from the other patches?
>>>
> 
> That is correct, it's a regression and can be applied independently.
> 
> This patch has been tested independently of the rest of the series,
> using the test introduced in "selftests: media_tests: Add a
> memory-to-memory concurrent stress test".
> 
> Without this patch, the test fails.
> 
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
>>>>  1 file changed, 29 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> index 5f9cd5b74cda..dfd796621b06 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
>>>>  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
>>>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>>>>  
>>>> +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
>>>
>>> Is this intended? It feels out of place in this patch.
>>>
> 
> It was intended :) Since the patch is adding some more documentation,
> it felt OK to add this debug message, as it also helps the testing.
> 
> But I can drop it if you think it's out of place.
> 
>>>>  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>>>>  }
>>>>  
>>>> -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>>> +/*
>>>> + * __v4l2_m2m_try_queue() - queue a job
>>>> + * @m2m_dev: m2m device
>>>> + * @m2m_ctx: m2m context
>>>> + *
>>>> + * Check if this context is ready to queue a job.
>>>> + *
>>>> + * This function can run in interrupt context.
>>>> + */
>>>> +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
>>>>  {
>>>> -	struct v4l2_m2m_dev *m2m_dev;
>>>>  	unsigned long flags_job, flags_out, flags_cap;
>>>>  
>>>> -	m2m_dev = m2m_ctx->m2m_dev;
>>>>  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
>>>>  
>>>>  	if (!m2m_ctx->out_q_ctx.q.streaming
>>>> @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>>>  	m2m_ctx->job_flags |= TRANS_QUEUED;
>>>>  
>>>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
>>>> +}
>>>> +
>>>> +/**
>>>> + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
>>>> + * @m2m_ctx: m2m context
>>>> + *
>>>> + * Check if this context is ready to queue a job. If suitable,
>>>> + * run the next queued job on the mem2mem device.
>>>> + *
>>>> + * This function shouldn't run in interrupt context.
>>>> + *
>>>> + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
>>>> + * and then run another job for another context.
>>>> + */
>>>> +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>>> +{
>>>> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>>>>  
>>>> +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
>>>
>>> Why introduce __v4l2_m2m_try_queue? Why not keep it in here?
>>>
> 
> Well, specifically we need to make sure xxx_try_run is called
> even if no job was queued. After a lot of back and forth, I ended
> up thinking the queue and run operations were different and needed
> different functions.
> 
> If we were to keep the code in try_schedule, we would have to use
> some extra conditionals or gotos to make sure try_run is called even
> if no job was queued.
> 
> The important thing to keep in mind here is that these functions
> could be called on a given context, but then actually run a job
> for another context.
> 
>>>>  	v4l2_m2m_try_run(m2m_dev);
>>
>> I'm completely confused: v4l2_m2m_try_schedule() already calls v4l2_m2m_try_run()!
>>
>> Either my brain has crashed due to the heatwave I'm suffering through, or there
>> is something amiss with this patch.
>>
>>
> 
> Right, v4l2_m2m_try_schedule calls v4l2_m2m_try_run *if* a job was queued.
> This might not always be the case.
> 
> Consider you schedule two jobs, both will be queued but only one will run.
> When will the second job run? Answer: never :)

Ah, now I get it. I hadn't seen that the 'return' statements in try_schedule
would prevent the try_run call at the end.

Thanks for the patch, I'll make a pull request for this one.

Regards,

	Hans

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[hverkuil]

On 27/07/18 17:16, Ezequiel Garcia wrote:
> On Fri, 2018-07-27 at 15:41 +0200, Hans Verkuil wrote:
>> On 27/07/18 15:35, Hans Verkuil wrote:
>>> On 25/07/18 19:15, Ezequiel Garcia wrote:
>>>> Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>>>> removed a redundant call to v4l2_m2m_try_run but instead introduced
>>>> a bug. Consider the following case:
>>>>
>>>>  1) Context A schedules, queues and runs job A.
>>>>  2) While the m2m device is running, context B schedules
>>>>     and queues job B. Job B cannot run, because it has to
>>>>     wait for job A.
>>>>  3) Job A completes, calls v4l2_m2m_job_finish, and tries
>>>>     to queue a job for context A, but since the context is
>>>>     empty it won't do anything.
>>>>
>>>> In this scenario, queued job B will never run. Fix this by calling
>>>> v4l2_m2m_try_run from v4l2_m2m_try_schedule.
>>>>
>>>> While here, add more documentation to these functions.
>>>>
>>>> Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>>>> Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
>>>
>>> So just to be clear: this first patch fixes a regression and can be applied
>>> separately from the other patches?
>>>
> 
> That is correct, it's a regression and can be applied independently.
> 
> This patch has been tested independently of the rest of the series,
> using the test introduced in "selftests: media_tests: Add a
> memory-to-memory concurrent stress test".
> 
> Without this patch, the test fails.
> 
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
>>>>  1 file changed, 29 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> index 5f9cd5b74cda..dfd796621b06 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
>>>>  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
>>>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>>>>  
>>>> +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
>>>
>>> Is this intended? It feels out of place in this patch.
>>>
> 
> It was intended :) Since the patch is adding some more documentation,
> it felt OK to add this debug message, as it also helps the testing.
> 
> But I can drop it if you think it's out of place.
> 
>>>>  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>>>>  }
>>>>  
>>>> -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>>> +/*
>>>> + * __v4l2_m2m_try_queue() - queue a job
>>>> + * @m2m_dev: m2m device
>>>> + * @m2m_ctx: m2m context
>>>> + *
>>>> + * Check if this context is ready to queue a job.
>>>> + *
>>>> + * This function can run in interrupt context.
>>>> + */
>>>> +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
>>>>  {
>>>> -	struct v4l2_m2m_dev *m2m_dev;
>>>>  	unsigned long flags_job, flags_out, flags_cap;
>>>>  
>>>> -	m2m_dev = m2m_ctx->m2m_dev;
>>>>  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
>>>>  
>>>>  	if (!m2m_ctx->out_q_ctx.q.streaming
>>>> @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>>>  	m2m_ctx->job_flags |= TRANS_QUEUED;
>>>>  
>>>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
>>>> +}
>>>> +
>>>> +/**
>>>> + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
>>>> + * @m2m_ctx: m2m context
>>>> + *
>>>> + * Check if this context is ready to queue a job. If suitable,
>>>> + * run the next queued job on the mem2mem device.
>>>> + *
>>>> + * This function shouldn't run in interrupt context.
>>>> + *
>>>> + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
>>>> + * and then run another job for another context.
>>>> + */
>>>> +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>>> +{
>>>> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>>>>  
>>>> +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
>>>
>>> Why introduce __v4l2_m2m_try_queue? Why not keep it in here?
>>>
> 
> Well, specifically we need to make sure xxx_try_run is called
> even if no job was queued. After a lot of back and forth, I ended
> up thinking the queue and run operations were different and needed
> different functions.
> 
> If we were to keep the code in try_schedule, we would have to use
> some extra conditionals or gotos to make sure try_run is called even
> if no job was queued.
> 
> The important thing to keep in mind here is that these functions
> could be called on a given context, but then actually run a job
> for another context.
> 
>>>>  	v4l2_m2m_try_run(m2m_dev);
>>
>> I'm completely confused: v4l2_m2m_try_schedule() already calls v4l2_m2m_try_run()!
>>
>> Either my brain has crashed due to the heatwave I'm suffering through, or there
>> is something amiss with this patch.
>>
>>
> 
> Right, v4l2_m2m_try_schedule calls v4l2_m2m_try_run *if* a job was queued.
> This might not always be the case.
> 
> Consider you schedule two jobs, both will be queued but only one will run.
> When will the second job run? Answer: never :)

Ah, now I get it. I hadn't seen that the 'return' statements in try_schedule
would prevent the try_run call at the end.

Thanks for the patch, I'll make a pull request for this one.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Hans Verkuil]

On 27/07/18 17:16, Ezequiel Garcia wrote:
> On Fri, 2018-07-27@15:41 +0200, Hans Verkuil wrote:
>> On 27/07/18 15:35, Hans Verkuil wrote:
>>> On 25/07/18 19:15, Ezequiel Garcia wrote:
>>>> Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>>>> removed a redundant call to v4l2_m2m_try_run but instead introduced
>>>> a bug. Consider the following case:
>>>>
>>>>  1) Context A schedules, queues and runs job A.
>>>>  2) While the m2m device is running, context B schedules
>>>>     and queues job B. Job B cannot run, because it has to
>>>>     wait for job A.
>>>>  3) Job A completes, calls v4l2_m2m_job_finish, and tries
>>>>     to queue a job for context A, but since the context is
>>>>     empty it won't do anything.
>>>>
>>>> In this scenario, queued job B will never run. Fix this by calling
>>>> v4l2_m2m_try_run from v4l2_m2m_try_schedule.
>>>>
>>>> While here, add more documentation to these functions.
>>>>
>>>> Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>>>> Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
>>>
>>> So just to be clear: this first patch fixes a regression and can be applied
>>> separately from the other patches?
>>>
> 
> That is correct, it's a regression and can be applied independently.
> 
> This patch has been tested independently of the rest of the series,
> using the test introduced in "selftests: media_tests: Add a
> memory-to-memory concurrent stress test".
> 
> Without this patch, the test fails.
> 
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
>>>>  1 file changed, 29 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> index 5f9cd5b74cda..dfd796621b06 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
>>>>  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
>>>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>>>>  
>>>> +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
>>>
>>> Is this intended? It feels out of place in this patch.
>>>
> 
> It was intended :) Since the patch is adding some more documentation,
> it felt OK to add this debug message, as it also helps the testing.
> 
> But I can drop it if you think it's out of place.
> 
>>>>  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>>>>  }
>>>>  
>>>> -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>>> +/*
>>>> + * __v4l2_m2m_try_queue() - queue a job
>>>> + * @m2m_dev: m2m device
>>>> + * @m2m_ctx: m2m context
>>>> + *
>>>> + * Check if this context is ready to queue a job.
>>>> + *
>>>> + * This function can run in interrupt context.
>>>> + */
>>>> +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
>>>>  {
>>>> -	struct v4l2_m2m_dev *m2m_dev;
>>>>  	unsigned long flags_job, flags_out, flags_cap;
>>>>  
>>>> -	m2m_dev = m2m_ctx->m2m_dev;
>>>>  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
>>>>  
>>>>  	if (!m2m_ctx->out_q_ctx.q.streaming
>>>> @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>>>  	m2m_ctx->job_flags |= TRANS_QUEUED;
>>>>  
>>>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
>>>> +}
>>>> +
>>>> +/**
>>>> + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
>>>> + * @m2m_ctx: m2m context
>>>> + *
>>>> + * Check if this context is ready to queue a job. If suitable,
>>>> + * run the next queued job on the mem2mem device.
>>>> + *
>>>> + * This function shouldn't run in interrupt context.
>>>> + *
>>>> + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
>>>> + * and then run another job for another context.
>>>> + */
>>>> +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>>> +{
>>>> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>>>>  
>>>> +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
>>>
>>> Why introduce __v4l2_m2m_try_queue? Why not keep it in here?
>>>
> 
> Well, specifically we need to make sure xxx_try_run is called
> even if no job was queued. After a lot of back and forth, I ended
> up thinking the queue and run operations were different and needed
> different functions.
> 
> If we were to keep the code in try_schedule, we would have to use
> some extra conditionals or gotos to make sure try_run is called even
> if no job was queued.
> 
> The important thing to keep in mind here is that these functions
> could be called on a given context, but then actually run a job
> for another context.
> 
>>>>  	v4l2_m2m_try_run(m2m_dev);
>>
>> I'm completely confused: v4l2_m2m_try_schedule() already calls v4l2_m2m_try_run()!
>>
>> Either my brain has crashed due to the heatwave I'm suffering through, or there
>> is something amiss with this patch.
>>
>>
> 
> Right, v4l2_m2m_try_schedule calls v4l2_m2m_try_run *if* a job was queued.
> This might not always be the case.
> 
> Consider you schedule two jobs, both will be queued but only one will run.
> When will the second job run? Answer: never :)

Ah, now I get it. I hadn't seen that the 'return' statements in try_schedule
would prevent the try_run call at the end.

Thanks for the patch, I'll make a pull request for this one.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html