linux-kernel - Re: [PATCH 16/55] media: qcom: venus: Stop abusing of min_buffers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cf444351-4305-450d-9b3a-a02a310543c7@xs4all.nl>
Date:   Tue, 28 Nov 2023 11:26:23 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        mchehab@...nel.org, tfiga@...omium.org, m.szyprowski@...sung.com,
        matt.ranostay@...sulko.com
Cc:     linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        linux-staging@...ts.linux.dev, kernel@...labora.com,
        Stanimir Varbanov <stanimir.k.varbanov@...il.com>,
        Vikash Garodia <quic_vgarodia@...cinc.com>,
        Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 16/55] media: qcom: venus: Stop abusing of
 min_buffers_needed field

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@...labora.com>
> CC: Stanimir Varbanov <stanimir.k.varbanov@...il.com> (maintainer:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER)
> CC: Vikash Garodia <quic_vgarodia@...cinc.com> (maintainer:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER)
> CC: "Bryan O'Donoghue" <bryan.odonoghue@...aro.org> (reviewer:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER)
> CC: Andy Gross <agross@...nel.org> (maintainer:ARM/QUALCOMM SUPPORT)
> CC: Bjorn Andersson <andersson@...nel.org> (maintainer:ARM/QUALCOMM SUPPORT)
> CC: Konrad Dybcio <konrad.dybcio@...aro.org> (maintainer:ARM/QUALCOMM SUPPORT)
> CC: linux-arm-msm@...r.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