lkml.org 
[lkml]   [2023]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2] media: v4l2-mem2mem: add a list for buf used by hw
From


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

\
 
 \ /
  Last update: 2023-07-27 09:42    [W:0.188 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site