Re: [PATCH 2/2] media: v4l2-mem2mem: add a list for buf used by hw

[Tomasz Figa]

On Thu, Jul 27, 2023 at 4:33 PM Hsia-Jun Li <Randy.Li@xxxxxxxxxxxxx> wrote:
>
>
>
> 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 people on the linux-media IRC channel (including me) could
also be willing to help with rephrasing things, but if a chatbot can
do it too, it's even better. :)

> > 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