Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20231127165454.166373-1-benjamin.gaignard@collabora.com>
 <20231127165454.166373-17-benjamin.gaignard@collabora.com> <cf444351-4305-450d-9b3a-a02a310543c7@xs4all.nl>
In-Reply-To: <cf444351-4305-450d-9b3a-a02a310543c7@xs4all.nl>
From:   Tomasz Figa <tfiga@chromium.org>
Date:   Wed, 29 Nov 2023 18:48:15 +0900
Message-ID: <CAAFQd5DHQvvodpKTYN4AE6Q=48Mby+qtt_kE9xjgarn1jQextw@mail.gmail.com>
Subject: Re: [PATCH 16/55] media: qcom: venus: Stop abusing of
 min_buffers_needed field
To:     Hans Verkuil <hverkuil@xs4all.nl>
Cc:     Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        mchehab@kernel.org, m.szyprowski@samsung.com,
        matt.ranostay@konsulko.com, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org, linux-staging@lists.linux.dev,
        kernel@collabora.com,
        Stanimir Varbanov <stanimir.k.varbanov@gmail.com>,
        Vikash Garodia <quic_vgarodia@quicinc.com>,
        "Bryan O'Donoghue" <bryan.odonoghue@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        linux-arm-msm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Nov 28, 2023 at 7:26=E2=80=AFPM Hans Verkuil <hverkuil@xs4all.nl> w=
rote:
>
> On 27/11/2023 17:54, Benjamin Gaignard wrote:
> > 'min_buffers_needed' is suppose to be used to indicate the number
> > of buffers needed by DMA engine to start streaming.
> > venus driver doesn't use DMA engine and just want to specify
> > the minimum number of buffers to allocate when calling VIDIOC_REQBUFS.
> > That 'min_reqbufs_allocation' field purpose so use it.
> >
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > CC: Stanimir Varbanov <stanimir.k.varbanov@gmail.com> (maintainer:QUALC=
OMM VENUS VIDEO ACCELERATOR DRIVER)
> > CC: Vikash Garodia <quic_vgarodia@quicinc.com> (maintainer:QUALCOMM VEN=
US VIDEO ACCELERATOR DRIVER)
> > CC: "Bryan O'Donoghue" <bryan.odonoghue@linaro.org> (reviewer:QUALCOMM =
VENUS VIDEO ACCELERATOR DRIVER)
> > CC: Andy Gross <agross@kernel.org> (maintainer:ARM/QUALCOMM SUPPORT)
> > CC: Bjorn Andersson <andersson@kernel.org> (maintainer:ARM/QUALCOMM SUP=
PORT)
> > CC: Konrad Dybcio <konrad.dybcio@linaro.org> (maintainer:ARM/QUALCOMM S=
UPPORT)
> > CC: linux-arm-msm@vger.kernel.org
> > ---
> >  drivers/media/platform/qcom/venus/vdec.c | 4 ++--
> >  drivers/media/platform/qcom/venus/venc.c | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/p=
latform/qcom/venus/vdec.c
> > index dbf305cec120..16b8d0dde10d 100644
> > --- a/drivers/media/platform/qcom/venus/vdec.c
> > +++ b/drivers/media/platform/qcom/venus/vdec.c
> > @@ -1641,7 +1641,7 @@ static int m2m_queue_init(void *priv, struct vb2_=
queue *src_vq,
> >       src_vq->drv_priv =3D inst;
> >       src_vq->buf_struct_size =3D sizeof(struct venus_buffer);
> >       src_vq->allow_zero_bytesused =3D 1;
> > -     src_vq->min_buffers_needed =3D 0;
> > +     src_vq->min_reqbufs_allocation =3D 0;
>
> Just drop this.
>
> >       src_vq->dev =3D inst->core->dev;
> >       src_vq->lock =3D &inst->ctx_q_lock;
> >       ret =3D vb2_queue_init(src_vq);
> > @@ -1656,7 +1656,7 @@ static int m2m_queue_init(void *priv, struct vb2_=
queue *src_vq,
> >       dst_vq->drv_priv =3D inst;
> >       dst_vq->buf_struct_size =3D sizeof(struct venus_buffer);
> >       dst_vq->allow_zero_bytesused =3D 1;
> > -     dst_vq->min_buffers_needed =3D 0;
> > +     dst_vq->min_reqbufs_allocation =3D 0;
> >       dst_vq->dev =3D inst->core->dev;
> >       dst_vq->lock =3D &inst->ctx_q_lock;
> >       return vb2_queue_init(dst_vq);
> > diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/p=
latform/qcom/venus/venc.c
> > index 44b13696cf82..e399d01c208c 100644
> > --- a/drivers/media/platform/qcom/venus/venc.c
> > +++ b/drivers/media/platform/qcom/venus/venc.c
> > @@ -1398,7 +1398,7 @@ static int m2m_queue_init(void *priv, struct vb2_=
queue *src_vq,
> >       src_vq->drv_priv =3D inst;
> >       src_vq->buf_struct_size =3D sizeof(struct venus_buffer);
> >       src_vq->allow_zero_bytesused =3D 1;
> > -     src_vq->min_buffers_needed =3D 1;
> > +     src_vq->min_reqbufs_allocation =3D 1;
>
> So for platform drivers like this it is going to be more difficult to
> determine which meaning min_buffers_needed had: is at least one queued
> buffer needed before you can start streaming, or is this for a minimum
> buffer allocation?

By the way, for stateful decoders, we also have the
V4L2_CID_MIN_BUFFERS_FOR_CAPTURE control, which tells the userspace
the minimum buffers that need to be allocated and the drivers also
should adjust the number requested in REQBUFS to be at least that.

>
> In the case of m2m devices using the v4l2-mem2mem framework it is almost
> certainly the minimum buffer allocation since the m2m framework already
> checks that there are input and output buffers queued (__v4l2_m2m_try_que=
ue).
>
> So just delete the src_vq->min_buffers_needed =3D 1; line, and there is
> no need to set min_reqbufs_allocation.
>
> >       src_vq->dev =3D inst->core->dev;
> >       src_vq->lock =3D &inst->ctx_q_lock;
> >       if (inst->core->res->hfi_version =3D=3D HFI_VERSION_1XX)
> > @@ -1415,7 +1415,7 @@ static int m2m_queue_init(void *priv, struct vb2_=
queue *src_vq,
> >       dst_vq->drv_priv =3D inst;
> >       dst_vq->buf_struct_size =3D sizeof(struct venus_buffer);
> >       dst_vq->allow_zero_bytesused =3D 1;
> > -     dst_vq->min_buffers_needed =3D 1;
> > +     dst_vq->min_reqbufs_allocation =3D 1;
> >       dst_vq->dev =3D inst->core->dev;
> >       dst_vq->lock =3D &inst->ctx_q_lock;
> >       return vb2_queue_init(dst_vq);
>
> Ditto.
>
> Regards,
>
>         Hans