[PATCH RFC 0/3] media: Ensure access to dma-buf-imported reference buffers

[PATCH RFC 0/3] media: Ensure access to dma-buf-imported reference buffers

[Paul Kocialkowski]

This is a first attempt at implementing proper access to buffers
imported with dma-buf and used as reference frames for decoding
subsequent frames.

The main concern associated with this scenario was that memory could be
liberated while the buffer is not in a queued state. After careful
checking, it appears that this is not the case as the refcount is
always increased when importing the buffer. It is only decreased when
the same buffer is queued with a different dma-buf fd or when all
buffers are liberated. In the meantime (when decoding using the frame as
a reference happens), the buffer is thus guaranteed to stay alive.

However, buffers imported with dma-buf require calls to dma-buf-specific
map/unmap memops before they are accessed. This introduces the plumbing
for allowing that.

One major issue is that it cannot be done in atomic context, where
the job is marked as completed. To solve this, a new m2m operation
(job_done) is introduced which allows releasing access to the
reference buffers from the m2m worker thread.

Since this series touches the core of vb2 and m2m and may not be the
best way to implement this, it is marked as RFC.

This series is based on the series starting with:
  media: cedrus: Cleanup duplicate declarations from cedrus_dec header

Paul Kocialkowski (4):
  media: vb2: Add helpers to access unselected buffers
  media: v4l2-mem2mem: Add an optional job_done operation
  media: cedrus: Request access to reference buffers when decoding
  media: cedrus: Remove completed item from TODO list (dma-buf
    references)

 .../media/common/videobuf2/videobuf2-core.c   | 46 +++++++++++++++++++
 drivers/media/v4l2-core/v4l2-mem2mem.c        |  8 +++-
 drivers/staging/media/sunxi/cedrus/TODO       |  5 --
 drivers/staging/media/sunxi/cedrus/cedrus.c   |  1 +
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  1 +
 .../staging/media/sunxi/cedrus/cedrus_dec.c   | 18 ++++++++
 .../staging/media/sunxi/cedrus/cedrus_dec.h   |  1 +
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c |  8 ++++
 include/media/v4l2-mem2mem.h                  |  4 ++
 include/media/videobuf2-core.h                | 15 ++++++
 10 files changed, 100 insertions(+), 7 deletions(-)

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 1/4] media: vb2: Add helpers to access unselected buffers

[Paul Kocialkowski]

Introduce helpers to request and release access to buffers that are
not currently selected as current output or capture buffers.

This is useful to ensure proper access to buffers imported via dma-buf
that are used as reference and thus require associated map/unmap calls
before access.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 .../media/common/videobuf2/videobuf2-core.c   | 46 +++++++++++++++++++
 include/media/videobuf2-core.h                | 15 ++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 70e8c3366f9c..2a0c5de4d683 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -986,6 +986,52 @@ void vb2_discard_done(struct vb2_queue *q)
 }
 EXPORT_SYMBOL_GPL(vb2_discard_done);
 
+int vb2_buffer_access_request(struct vb2_buffer *vb)
+{
+	struct vb2_queue *q = vb->vb2_queue;
+	unsigned int plane;
+	int ret;
+
+	/* Only dmabuf-imported buffers need to be mapped before access. */
+	if (q->memory != VB2_MEMORY_DMABUF)
+		return -EINVAL;
+
+	for (plane = 0; plane < vb->num_planes; ++plane) {
+		if (vb->planes[plane].dbuf_mapped)
+			continue;
+
+		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
+		if (ret) {
+			dprintk(1, "failed to map dmabuf for plane %d\n",
+				plane);
+			return ret;
+		}
+		vb->planes[plane].dbuf_mapped = 1;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_buffer_access_request);
+
+void vb2_buffer_access_release(struct vb2_buffer *vb)
+{
+	struct vb2_queue *q = vb->vb2_queue;
+	unsigned int plane;
+
+	/* Only dmabuf-imported buffers need to be unmapped after access. */
+	if (q->memory != VB2_MEMORY_DMABUF)
+		return;
+
+	for (plane = 0; plane < vb->num_planes; ++plane) {
+		if (!vb->planes[plane].dbuf_mapped)
+			continue;
+
+		call_void_memop(vb, unmap_dmabuf, vb->planes[plane].mem_priv);
+		vb->planes[plane].dbuf_mapped = 0;
+	}
+}
+EXPORT_SYMBOL_GPL(vb2_buffer_access_release);
+
 /*
  * __prepare_mmap() - prepare an MMAP buffer
  */
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 4a737b2c610b..bf378c1e718b 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -1199,4 +1199,19 @@ bool vb2_request_object_is_buffer(struct media_request_object *obj);
  */
 unsigned int vb2_request_buffer_cnt(struct media_request *req);
 
+/**
+ * vb2_buffer_access_request() - request out-of-band data access to a buffer
+ *
+ * @vb:		buffer to request data access for
+ */
+int vb2_buffer_access_request(struct vb2_buffer *vb);
+
+
+/**
+ * vb2_buffer_access_release() - release out-of-band data access to a buffer
+ *
+ * @vb:		buffer to release data access for
+ */
+void vb2_buffer_access_release(struct vb2_buffer *vb);
+
 #endif /* _MEDIA_VIDEOBUF2_CORE_H */
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 2/4] media: v4l2-mem2mem: Add an optional job_done operation

[Paul Kocialkowski]

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.

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.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 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;
+
 	v4l2_m2m_try_run(m2m_dev);
 }
 
@@ -431,8 +436,7 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
 	list_del(&m2m_dev->curr_ctx->queue);
 	m2m_dev->curr_ctx->job_flags &= ~(TRANS_QUEUED | TRANS_RUNNING);
 	wake_up(&m2m_dev->curr_ctx->finished);
-	m2m_dev->curr_ctx = NULL;
-
+	/* The current context pointer is cleared after the job_done step. */
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
 
 	/* This instance might have more buffers ready, but since we do not
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 43e447dcf69d..261bcd661b2d 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -40,11 +40,15 @@
  *		v4l2_m2m_job_finish() (as if the transaction ended normally).
  *		This function does not have to (and will usually not) wait
  *		until the device enters a state when it can be stopped.
+ * @job_done:	optional. Informs the driver that the current job was completed.
+ *		This can be useful to release access to extra buffers that were
+ *		required for the job, such as reference buffers for decoding.
  */
 struct v4l2_m2m_ops {
 	void (*device_run)(void *priv);
 	int (*job_ready)(void *priv);
 	void (*job_abort)(void *priv);
+	void (*job_done)(void *priv);
 };
 
 struct video_device;
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 2/4] media: v4l2-mem2mem: Add an optional job_done operation

[Ezequiel Garcia]

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 <paul.kocialkowski@bootlin.com>
> ---
>  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.

Thanks,
Eze


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 3/4] media: cedrus: Request access to reference buffers when decoding

[Paul Kocialkowski]

Because we need to request and release access to reference buffers
that are backed by a dma-buf import, keep track of the buffers used
as reference and add the appropriate calls in the device_run and
job_done m2m callbacks. The latter is introduced for this purpose.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c    |  1 +
 drivers/staging/media/sunxi/cedrus/cedrus.h    |  1 +
 .../staging/media/sunxi/cedrus/cedrus_dec.c    | 18 ++++++++++++++++++
 .../staging/media/sunxi/cedrus/cedrus_dec.h    |  1 +
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c  |  8 ++++++++
 5 files changed, 29 insertions(+)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index ff11cbeba205..a2c75eb01c2c 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -250,6 +250,7 @@ static const struct video_device cedrus_video_device = {
 
 static const struct v4l2_m2m_ops cedrus_m2m_ops = {
 	.device_run	= cedrus_device_run,
+	.job_done	= cedrus_job_done,
 };
 
 static const struct media_device_ops cedrus_m2m_media_ops = {
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 4aedd24a9848..c1ce3c4ae2a1 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -77,6 +77,7 @@ struct cedrus_ctx {
 	struct v4l2_ctrl		**ctrls;
 
 	struct vb2_buffer		*dst_bufs[VIDEO_MAX_FRAME];
+	bool				reference_bufs[VIDEO_MAX_FRAME];
 };
 
 struct cedrus_dec_ops {
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index 2c295286766c..90bf51e71f4f 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -41,6 +41,7 @@ void cedrus_device_run(void *priv)
 	struct cedrus_dev *dev = ctx->dev;
 	struct cedrus_run run = {};
 	struct media_request *src_req;
+	unsigned int i;
 
 	run.src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
 	run.dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
@@ -63,10 +64,17 @@ void cedrus_device_run(void *priv)
 		break;
 	}
 
+	for (i = 0; i < VIDEO_MAX_FRAME; i++)
+		ctx->reference_bufs[i] = false;
+
 	v4l2_m2m_buf_copy_data(run.src, run.dst, true);
 
 	dev->dec_ops[ctx->current_codec]->setup(ctx, &run);
 
+	for (i = 0; i < VIDEO_MAX_FRAME; i++)
+		if (ctx->reference_bufs[i] && ctx->dst_bufs[i])
+			vb2_buffer_access_request(ctx->dst_bufs[i]);
+
 	/* Complete request(s) controls if needed. */
 
 	if (src_req)
@@ -74,3 +82,13 @@ void cedrus_device_run(void *priv)
 
 	dev->dec_ops[ctx->current_codec]->trigger(ctx);
 }
+
+void cedrus_job_done(void *priv)
+{
+	struct cedrus_ctx *ctx = priv;
+	unsigned int i;
+
+	for (i = 0; i < VIDEO_MAX_FRAME; i++)
+		if (ctx->reference_bufs[i] && ctx->dst_bufs[i])
+			vb2_buffer_access_release(ctx->dst_bufs[i]);
+}
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.h b/drivers/staging/media/sunxi/cedrus/cedrus_dec.h
index 8d0fc248220f..9265f28e6dbe 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.h
@@ -19,5 +19,6 @@
 int cedrus_reference_index_find(struct vb2_queue *queue,
 				struct vb2_buffer *vb2_buf, u64 timestamp);
 void cedrus_device_run(void *priv);
+void cedrus_job_done(void *priv);
 
 #endif
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index 81c66a8aa1ac..2e7deded17aa 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
@@ -162,6 +162,10 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 	/* Forward and backward prediction reference buffers. */
 	forward_idx = cedrus_reference_index_find(cap_q, &run->dst->vb2_buf,
 						  slice_params->forward_ref_ts);
+	if (forward_idx < 0)
+		return;
+
+	ctx->reference_bufs[forward_idx] = true;
 
 	fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
 	fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
@@ -171,6 +175,10 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 
 	backward_idx = cedrus_reference_index_find(cap_q, &run->dst->vb2_buf,
 						   slice_params->backward_ref_ts);
+	if (forward_idx < 0)
+		return;
+
+	ctx->reference_bufs[backward_idx] = true;
 
 	bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
 	bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 4/4] media: cedrus: Remove completed item from TODO list (dma-buf references)

[Paul Kocialkowski]

Access to reference frames that were imported from dma-buf was taken
care of and is no longer a pending item on the driver's TODO list.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/staging/media/sunxi/cedrus/TODO | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/TODO b/drivers/staging/media/sunxi/cedrus/TODO
index a951b3fd1ea1..ec277ece47af 100644
--- a/drivers/staging/media/sunxi/cedrus/TODO
+++ b/drivers/staging/media/sunxi/cedrus/TODO
@@ -5,8 +5,3 @@ Before this stateless decoder driver can leave the staging area:
 * Userspace support for the Request API needs to be reviewed;
 * Another stateless decoder driver should be submitted;
 * At least one stateless encoder driver should be submitted.
-* When queueing a request containing references to I frames, the
-  refcount of the memory for those I frames needs to be incremented
-  and decremented when the request is completed. This will likely
-  require some help from vb2. The driver should fail the request
-  if the memory/buffer is gone.
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel