Re: [PATCH 2/2] media: v4l2-mem2mem: add a list for buf used by hw
From: Hsia-Jun Li
Date: Thu Jul 27 2023 - 03:40:46 EST
On 7/27/23 15:25, Tomasz Figa wrote:
CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
On Mon, Jul 17, 2023 at 4:15 PM Hsia-Jun Li <Randy.Li@xxxxxxxxxxxxx> wrote:
On 7/12/23 17:33, Tomasz Figa wrote:
CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
On Tue, Jul 04, 2023 at 12:00:38PM +0800, Hsia-Jun Li wrote:
From: "Hsia-Jun(Randy) Li" <randy.li@xxxxxxxxxxxxx>
Many drivers have to create its own buf_struct for a
vb2_queue to track such a state. Also driver has to
iterate over rdy_queue every times to find out a buffer
which is not sent to hardware(or firmware), this new
list just offers the driver a place to store the buffer
that hardware(firmware) has acknowledged.
One important advance about this list, it doesn't like
rdy_queue which both bottom half of the user calling
could operate it, while the v4l2 worker would as well.
The v4l2 core could only operate this queue when its
v4l2_context is not running, the driver would only
access this new hw_queue in its own worker.
Could you describe in what case such a list would be useful for a
mem2mem driver?
This list, as its description, just for saving us from creating a
private buffer struct to track buffer state.
The queue in the kernel is not the queue that hardware(codec firmware)
are using.
Sorry, I find the description difficult to understand. It might make
sense to have the text proofread by someone experienced in writing
technical documentation in English before posting in the future.
Thanks.
Sorry, I may not be able to get more resource here. I would try to ask a
chatbot to fix my description next time.
I think I got the point from Nicolas' explanation, though.
Signed-off-by: Hsia-Jun(Randy) Li <randy.li@xxxxxxxxxxxxx>
---
drivers/media/v4l2-core/v4l2-mem2mem.c | 25 +++++++++++++++++--------
include/media/v4l2-mem2mem.h | 10 +++++++++-
2 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index c771aba42015..b4151147d5bd 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -321,15 +321,21 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
goto job_unlock;
}
- src = v4l2_m2m_next_src_buf(m2m_ctx);
- dst = v4l2_m2m_next_dst_buf(m2m_ctx);
- if (!src && !m2m_ctx->out_q_ctx.buffered) {
- dprintk("No input buffers available\n");
- goto job_unlock;
+ if (list_empty(&m2m_ctx->out_q_ctx.hw_queue)) {
+ src = v4l2_m2m_next_src_buf(m2m_ctx);
+
+ if (!src && !m2m_ctx->out_q_ctx.buffered) {
+ dprintk("No input buffers available\n");
+ goto job_unlock;
+ }
}
- if (!dst && !m2m_ctx->cap_q_ctx.buffered) {
- dprintk("No output buffers available\n");
- goto job_unlock;
+
+ if (list_empty(&m2m_ctx->cap_q_ctx.hw_queue)) {
+ dst = v4l2_m2m_next_dst_buf(m2m_ctx);
+ if (!dst && !m2m_ctx->cap_q_ctx.buffered) {
+ dprintk("No output buffers available\n");
+ goto job_unlock;
+ }
}
src and dst would be referenced unitialized below if neither of the
above ifs hits...
I think they have been initialized at v4l2_m2m_ctx_init()
What do you mean? They are local variables in this function.
Sorry, I didn't notice the sentence after that.
Best regards,
Tomasz
m2m_ctx->new_frame = true;
@@ -896,6 +902,7 @@ int v4l2_m2m_streamoff(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
INIT_LIST_HEAD(&q_ctx->rdy_queue);
q_ctx->num_rdy = 0;
spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags);
+ INIT_LIST_HEAD(&q_ctx->hw_queue);
if (m2m_dev->curr_ctx == m2m_ctx) {
m2m_dev->curr_ctx = NULL;
@@ -1234,6 +1241,8 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
INIT_LIST_HEAD(&out_q_ctx->rdy_queue);
INIT_LIST_HEAD(&cap_q_ctx->rdy_queue);
+ INIT_LIST_HEAD(&out_q_ctx->hw_queue);
+ INIT_LIST_HEAD(&cap_q_ctx->hw_queue);
spin_lock_init(&out_q_ctx->rdy_spinlock);
spin_lock_init(&cap_q_ctx->rdy_spinlock);
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index d6c8eb2b5201..2342656e582d 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -53,9 +53,16 @@ struct v4l2_m2m_dev;
* processed
*
* @q: pointer to struct &vb2_queue
- * @rdy_queue: List of V4L2 mem-to-mem queues
+ * @rdy_queue: List of V4L2 mem-to-mem queues. If v4l2_m2m_buf_queue() is
+ * called in struct vb2_ops->buf_queue(), the buffer enqueued
+ * by user would be added to this list.
* @rdy_spinlock: spin lock to protect the struct usage
* @num_rdy: number of buffers ready to be processed
+ * @hw_queue: A list for tracking the buffer is occupied by the hardware
+ * (or device's firmware). A buffer could only be in either
+ * this list or @rdy_queue.
+ * Driver may choose not to use this list while uses its own
+ * private data to do this work.
* @buffered: is the queue buffered?
*
* Queue for buffers ready to be processed as soon as this
@@ -68,6 +75,7 @@ struct v4l2_m2m_queue_ctx {
struct list_head rdy_queue;
spinlock_t rdy_spinlock;
u8 num_rdy;
+ struct list_head hw_queue;
bool buffered;
};
--
2.17.1
--
Hsia-Jun(Randy) Li
--
Hsia-Jun(Randy) Li