| Message ID | 20231127165454.166373-17-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up queue_setup()/min_buffers_needed (ab)use | expand |
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:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER) > CC: Vikash Garodia <quic_vgarodia@quicinc.com> (maintainer:QUALCOMM VENUS 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 SUPPORT) > CC: Konrad Dybcio <konrad.dybcio@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT) > 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/platform/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 = inst; > src_vq->buf_struct_size = sizeof(struct venus_buffer); > src_vq->allow_zero_bytesused = 1; > - src_vq->min_buffers_needed = 0; > + src_vq->min_reqbufs_allocation = 0; Just drop this. > src_vq->dev = inst->core->dev; > src_vq->lock = &inst->ctx_q_lock; > ret = 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 = inst; > dst_vq->buf_struct_size = sizeof(struct venus_buffer); > dst_vq->allow_zero_bytesused = 1; > - dst_vq->min_buffers_needed = 0; > + dst_vq->min_reqbufs_allocation = 0; > dst_vq->dev = inst->core->dev; > dst_vq->lock = &inst->ctx_q_lock; > return vb2_queue_init(dst_vq); > diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/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 = inst; > src_vq->buf_struct_size = sizeof(struct venus_buffer); > src_vq->allow_zero_bytesused = 1; > - src_vq->min_buffers_needed = 1; > + src_vq->min_reqbufs_allocation = 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? 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_queue). So just delete the src_vq->min_buffers_needed = 1; line, and there is no need to set min_reqbufs_allocation. > src_vq->dev = inst->core->dev; > src_vq->lock = &inst->ctx_q_lock; > if (inst->core->res->hfi_version == HFI_VERSION_1XX) > @@ -1415,7 +1415,7 @@ static int m2m_queue_init(void *priv, struct vb2_queue *src_vq, > dst_vq->drv_priv = inst; > dst_vq->buf_struct_size = sizeof(struct venus_buffer); > dst_vq->allow_zero_bytesused = 1; > - dst_vq->min_buffers_needed = 1; > + dst_vq->min_reqbufs_allocation = 1; > dst_vq->dev = inst->core->dev; > dst_vq->lock = &inst->ctx_q_lock; > return vb2_queue_init(dst_vq); Ditto. Regards, Hans
On Tue, Nov 28, 2023 at 7:26 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > 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:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER) > > CC: Vikash Garodia <quic_vgarodia@quicinc.com> (maintainer:QUALCOMM VENUS 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 SUPPORT) > > CC: Konrad Dybcio <konrad.dybcio@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT) > > 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/platform/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 = inst; > > src_vq->buf_struct_size = sizeof(struct venus_buffer); > > src_vq->allow_zero_bytesused = 1; > > - src_vq->min_buffers_needed = 0; > > + src_vq->min_reqbufs_allocation = 0; > > Just drop this. > > > src_vq->dev = inst->core->dev; > > src_vq->lock = &inst->ctx_q_lock; > > ret = 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 = inst; > > dst_vq->buf_struct_size = sizeof(struct venus_buffer); > > dst_vq->allow_zero_bytesused = 1; > > - dst_vq->min_buffers_needed = 0; > > + dst_vq->min_reqbufs_allocation = 0; > > dst_vq->dev = inst->core->dev; > > dst_vq->lock = &inst->ctx_q_lock; > > return vb2_queue_init(dst_vq); > > diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/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 = inst; > > src_vq->buf_struct_size = sizeof(struct venus_buffer); > > src_vq->allow_zero_bytesused = 1; > > - src_vq->min_buffers_needed = 1; > > + src_vq->min_reqbufs_allocation = 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_queue). > > So just delete the src_vq->min_buffers_needed = 1; line, and there is > no need to set min_reqbufs_allocation. > > > src_vq->dev = inst->core->dev; > > src_vq->lock = &inst->ctx_q_lock; > > if (inst->core->res->hfi_version == HFI_VERSION_1XX) > > @@ -1415,7 +1415,7 @@ static int m2m_queue_init(void *priv, struct vb2_queue *src_vq, > > dst_vq->drv_priv = inst; > > dst_vq->buf_struct_size = sizeof(struct venus_buffer); > > dst_vq->allow_zero_bytesused = 1; > > - dst_vq->min_buffers_needed = 1; > > + dst_vq->min_reqbufs_allocation = 1; > > dst_vq->dev = inst->core->dev; > > dst_vq->lock = &inst->ctx_q_lock; > > return vb2_queue_init(dst_vq); > > Ditto. > > Regards, > > Hans
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/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 = inst; src_vq->buf_struct_size = sizeof(struct venus_buffer); src_vq->allow_zero_bytesused = 1; - src_vq->min_buffers_needed = 0; + src_vq->min_reqbufs_allocation = 0; src_vq->dev = inst->core->dev; src_vq->lock = &inst->ctx_q_lock; ret = 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 = inst; dst_vq->buf_struct_size = sizeof(struct venus_buffer); dst_vq->allow_zero_bytesused = 1; - dst_vq->min_buffers_needed = 0; + dst_vq->min_reqbufs_allocation = 0; dst_vq->dev = inst->core->dev; dst_vq->lock = &inst->ctx_q_lock; return vb2_queue_init(dst_vq); diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/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 = inst; src_vq->buf_struct_size = sizeof(struct venus_buffer); src_vq->allow_zero_bytesused = 1; - src_vq->min_buffers_needed = 1; + src_vq->min_reqbufs_allocation = 1; src_vq->dev = inst->core->dev; src_vq->lock = &inst->ctx_q_lock; if (inst->core->res->hfi_version == HFI_VERSION_1XX) @@ -1415,7 +1415,7 @@ static int m2m_queue_init(void *priv, struct vb2_queue *src_vq, dst_vq->drv_priv = inst; dst_vq->buf_struct_size = sizeof(struct venus_buffer); dst_vq->allow_zero_bytesused = 1; - dst_vq->min_buffers_needed = 1; + dst_vq->min_reqbufs_allocation = 1; dst_vq->dev = inst->core->dev; dst_vq->lock = &inst->ctx_q_lock; return vb2_queue_init(dst_vq);
'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:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER) CC: Vikash Garodia <quic_vgarodia@quicinc.com> (maintainer:QUALCOMM VENUS 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 SUPPORT) CC: Konrad Dybcio <konrad.dybcio@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT) 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(-)