Messages in this thread | | | Date | Thu, 27 Jul 2023 15:33:03 +0800 | Subject | Re: [PATCH 2/2] media: v4l2-mem2mem: add a list for buf used by hw | From | Hsia-Jun Li <> |
| |
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@synaptics.com> 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@synaptics.com> >>>> >>>> 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@synaptics.com> >>>> --- >>>> 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
| |