Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230704040044.681850-1-randy.li@synaptics.com>
 <20230704040044.681850-3-randy.li@synaptics.com> <20230712093301.nkj2vok2x7esdhb3@chromium.org>
 <4e70c8b8-d459-2b79-2b3d-40875f701d97@synaptics.com>
In-Reply-To: <4e70c8b8-d459-2b79-2b3d-40875f701d97@synaptics.com>
From:   Tomasz Figa <tfiga@chromium.org>
Date:   Thu, 27 Jul 2023 16:25:00 +0900
Message-ID: <CAAFQd5CGO4iiC7HX9wTetuzjZyaAsKStFOpxqcFkZ_o-eJOPtg@mail.gmail.com>
Subject: Re: [PATCH 2/2] media: v4l2-mem2mem: add a list for buf used by hw
To:     Hsia-Jun Li <Randy.Li@synaptics.com>
Cc:     linux-media@vger.kernel.org, ayaka@soulik.info,
        hans.verkuil@cisco.com, mchehab@kernel.org,
        laurent.pinchart@ideasonboard.com, hiroh@chromium.org,
        hverkuil@xs4all.nl, linux-kernel@vger.kernel.org,
        nicolas@ndufresne.ca
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jul 17, 2023 at 4:15=E2=80=AFPM 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 attach=
ments 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.

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/v4=
l2-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 =3D v4l2_m2m_next_src_buf(m2m_ctx);
> >> -     dst =3D 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 =3D 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 =3D 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.

> >
> > Best regards,
> > Tomasz
> >
> >>        m2m_ctx->new_frame =3D true;
> >> @@ -896,6 +902,7 @@ int v4l2_m2m_streamoff(struct file *file, struct v=
4l2_m2m_ctx *m2m_ctx,
> >>        INIT_LIST_HEAD(&q_ctx->rdy_queue);
> >>        q_ctx->num_rdy =3D 0;
> >>        spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags);
> >> +     INIT_LIST_HEAD(&q_ctx->hw_queue);
> >>
> >>        if (m2m_dev->curr_ctx =3D=3D m2m_ctx) {
> >>                m2m_dev->curr_ctx =3D NULL;
> >> @@ -1234,6 +1241,8 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4=
l2_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 enqueu=
ed
> >> + *           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 th=
e 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 ow=
n
> >> + *           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
>