[lkml]   [2019]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH RFC 2/4] media: v4l2-mem2mem: Add an optional job_done operation
On Mon, 2019-01-14 at 14:38 +0100, Paul Kocialkowski wrote:
> Introduce a new optional job_done operation, which allows calling back
> to the driver when a job is done. Since the job might be completed
> from interrupt context where some operations are not available, having
> a callback from non-atomic context allows performing these operations
> upon completion of a job. This is particularly useful for releasing
> access to a reference buffer, which cannot be done in atomic context.

I'm not exactly sure it makes a lot of sense to review this patch,
since the approach could change.

However, let me point out a few fundamental issues here.

> Use the already existing v4l2_m2m_device_run_work work queue for that
> and clear the M2M device current context after calling job_done in the
> worker thread, so that the private data can be passed to the operation.
> Delaying the current context clearing should not be a problem since the
> next call to v4l2_m2m_try_run happens right after that.

Careful here. It's misleading to think an event will happen
"right after". I'd say it's either synchronously, or asynchronously.

It's quite the opposite I'd say, the clearing will happen
"who-knows-when the scheduler picks the thread to run" :-)

Before this patch the curr_ctx was cleared in v4l2_m2m_job_finish,
atomically with the ctx job flags clearing and before waking up
threads waiting in v4l2_m2m_cancel_job.

You are now changing this, by clearing curr_ctx in a worker.
It's perfectly possible that v4l2_m2m_try_schedule will run
before the worker, trying to run with the old context, which
apparently would be safely refused.

> Signed-off-by: Paul Kocialkowski <>
> ---
> drivers/media/v4l2-core/v4l2-mem2mem.c | 8 ++++++--
> include/media/v4l2-mem2mem.h | 4 ++++
> 2 files changed, 10 insertions(+), 2 deletions(-)
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 631f4e2aa942..d5bccb0192f9 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -376,6 +376,11 @@ 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);
> + if (m2m_dev->m2m_ops->job_done && m2m_dev->curr_ctx)
> + m2m_dev->m2m_ops->job_done(m2m_dev->curr_ctx->priv);
> +
> + m2m_dev->curr_ctx = NULL;
> +

I don't think you can access this without taking the job spinlock.

> v4l2_m2m_try_run(m2m_dev);
> }

Aside from this, it seems we might need this hook sooner or later.


 \ /
  Last update: 2019-01-15 10:44    [W:0.046 / U:5.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site