[PATCH v6 0/6] media: Fix coccinelle warning/errors

[PATCH v6 0/6] media: Fix coccinelle warning/errors

[Ricardo Ribalda]

These is the last set of patches to fix all the relevant patchwork
warnings (TM).

--

---
Changes in v6:
- Improve comments for tda10048, thanks Kosta.
- Link to v5: https://lore.kernel.org/r/20250107-fix-cocci-v5-0-b26da641f730@chromium.org

Changes in v5:
- venus: Ignore fps > 240
- venus: Clamp invalid fps instead of -EINVAL
- Link to v4: https://lore.kernel.org/r/20250106-fix-cocci-v4-0-3c8eb97995ba@chromium.org

Changes in v4:
- Remove all merged patches
- Improve commit messages.
- media: Remove timeperframe from inst
- Ignore 0 fps (Thanks Hans)
- Link to v3: https://lore.kernel.org/r/20240429-fix-cocci-v3-0-3c4865f5a4b0@chromium.org

Changes in v3: Thanks Bryan, Dan, Markus, Sakary and Hans
- Improve commit messages.
- Use div64_u64 when possible
- Link to v2: https://lore.kernel.org/r/20240419-fix-cocci-v2-0-2119e692309c@chromium.org

Changes in v2:
- Remove all the min() retval, and send a patch for cocci:  https://lore.kernel.org/lkml/20240415-minimax-v1-1-5feb20d66a79@chromium.org/T/#u
- platform_get_irq() cannot return 0, fix that (Thanks Dan).
- Fix stb0800 patch. chip_id can be 0 (Thanks Dan).
- Use runtime (IS_ENABLED), code looks nicer. (Thanks Dan).
- Do not replace do_div for venus (Thanks Dan).
- Do not replace do_div for tda10048 (Thanks Dan).
- Link to v1: https://lore.kernel.org/r/20240415-fix-cocci-v1-0-477afb23728b@chromium.org

To: Mauro Carvalho Chehab <mchehab@kernel.org>
To: Stanimir Varbanov <stanimir.k.varbanov@gmail.com>
To: Vikash Garodia <quic_vgarodia@quicinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-msm@vger.kernel.org

---
Ricardo Ribalda (6):
      media: dvb-frontends: tda10048: Make the range of z explicit.
      media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.
      media: venus: venc: Clamp parm smaller than 1fps and bigger than 240
      media: venus: Remove timeperframe from inst
      media: venus: venc: Make the range of us_per_frame explicit
      media: venus: vdec: Make the range of us_per_frame explicit

 drivers/media/dvb-frontends/tda10048.c   |  8 +++++++-
 drivers/media/platform/qcom/venus/core.h |  4 ++--
 drivers/media/platform/qcom/venus/vdec.c | 23 +++++++++++------------
 drivers/media/platform/qcom/venus/venc.c | 24 +++++++++++-------------
 4 files changed, 31 insertions(+), 28 deletions(-)
---
base-commit: 4db312bbce420e3f874302549db072211e03c569
change-id: 20240415-fix-cocci-2df3ef22a6f7

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v6 1/6] media: dvb-frontends: tda10048: Make the range of z explicit.

[Ricardo Ribalda]

The datasheet recommends 55MHz frequency sampling, with a max of 69 MHz.
(Kudos to Kosta Stefanov for the calculations).

Replace z with a 32 bit uint, and make the range of the variable
explicit.

Found by cocci:
drivers/media/dvb-frontends/tda10048.c:345:1-7: WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead.

Reviewed-by: Kosta Stefanov <costa.stephanoff@gmail.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/dvb-frontends/tda10048.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/tda10048.c b/drivers/media/dvb-frontends/tda10048.c
index 3e725cdcc66b..1f87eb0dcf2a 100644
--- a/drivers/media/dvb-frontends/tda10048.c
+++ b/drivers/media/dvb-frontends/tda10048.c
@@ -328,7 +328,8 @@ static int tda10048_set_wref(struct dvb_frontend *fe, u32 sample_freq_hz,
 			     u32 bw)
 {
 	struct tda10048_state *state = fe->demodulator_priv;
-	u64 t, z;
+	u64 t;
+	u32 z;
 
 	dprintk(1, "%s()\n", __func__);
 
@@ -341,6 +342,11 @@ static int tda10048_set_wref(struct dvb_frontend *fe, u32 sample_freq_hz,
 	/* t *= 2147483648 on 32bit platforms */
 	t *= (2048 * 1024);
 	t *= 1024;
+
+	/*
+	 * Sample frequency is typically 55 MHz, with a theoretical maximum of
+	 * 69 MHz. With a 32 bit z we have enough accuracy for up to 613 MHz.
+	 */
 	z = 7 * sample_freq_hz;
 	do_div(t, z);
 	t += 5;

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.

[Ricardo Ribalda]

The driver uses "whole" fps in all its calculations (e.g. in
load_per_instance()). Those calculation expect an fps bigger than 1, and
not big enough to overflow.

Clamp the value if the user provides a parm that will result in an invalid
fps.

Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
Closes: https://lore.kernel.org/linux-media/f11653a7-bc49-48cd-9cdb-1659147453e4@xs4all.nl/T/#m91cd962ac942834654f94c92206e2f85ff7d97f0
Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/platform/qcom/venus/core.h | 2 ++
 drivers/media/platform/qcom/venus/vdec.c | 5 ++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 44f1c3bc4186..afae2b9fdaf7 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -28,6 +28,8 @@
 #define VIDC_RESETS_NUM_MAX		2
 #define VIDC_MAX_HIER_CODING_LAYER 6
 
+#define VENUS_MAX_FPS			240
+
 extern int venus_fw_debug;
 
 struct freq_tbl {
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 98c22b9f9372..c1d5f94e16b4 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -481,11 +481,10 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 	us_per_frame = timeperframe->numerator * (u64)USEC_PER_SEC;
 	do_div(us_per_frame, timeperframe->denominator);
 
-	if (!us_per_frame)
-		return -EINVAL;
-
+	us_per_frame = max(USEC_PER_SEC, us_per_frame);
 	fps = (u64)USEC_PER_SEC;
 	do_div(fps, us_per_frame);
+	fps = min(VENUS_MAX_FPS, fps);
 
 	inst->fps = fps;
 	inst->timeperframe = *timeperframe;

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v6 3/6] media: venus: venc: Clamp parm smaller than 1fps and bigger than 240

[Ricardo Ribalda]

The driver uses "whole" fps in all its calculations (e.g. in
load_per_instance()). Those calculation expect an fps bigger than 1, and
not big enough to overflow.

Clamp the parm if the user provides a value that will result in an invalid
fps.

Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
Closes: https://lore.kernel.org/linux-media/f11653a7-bc49-48cd-9cdb-1659147453e4@xs4all.nl/T/#m91cd962ac942834654f94c92206e2f85ff7d97f0
Fixes: aaaa93eda64b ("[media] media: venus: venc: add video encoder files")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/platform/qcom/venus/venc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index c1c543535aaf..943d432b6568 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -411,11 +411,10 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 	us_per_frame = timeperframe->numerator * (u64)USEC_PER_SEC;
 	do_div(us_per_frame, timeperframe->denominator);
 
-	if (!us_per_frame)
-		return -EINVAL;
-
+	us_per_frame = max(USEC_PER_SEC, us_per_frame);
 	fps = (u64)USEC_PER_SEC;
 	do_div(fps, us_per_frame);
+	fps = min(VENUS_MAX_FPS, fps);
 
 	inst->timeperframe = *timeperframe;
 	inst->fps = fps;

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v6 4/6] media: venus: Remove timeperframe from inst

[Ricardo Ribalda]

The driver only cares about whole fps. We can infer the timeperframe
from the fps field. Remove the redundant field.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/platform/qcom/venus/core.h |  2 --
 drivers/media/platform/qcom/venus/vdec.c | 15 ++++++++-------
 drivers/media/platform/qcom/venus/venc.c | 16 ++++++++--------
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index afae2b9fdaf7..1d4fd5cc75d9 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -407,7 +407,6 @@ enum venus_inst_modes {
  * @tss:		timestamp metadata
  * @payloads:		cache plane payload to use it for clock/BW scaling
  * @fps:		holds current FPS
- * @timeperframe:	holds current time per frame structure
  * @fmt_out:	a reference to output format structure
  * @fmt_cap:	a reference to capture format structure
  * @num_input_bufs:	holds number of input buffers
@@ -478,7 +477,6 @@ struct venus_inst {
 	struct venus_ts_metadata tss[VIDEO_MAX_FRAME];
 	unsigned long payloads[VIDEO_MAX_FRAME];
 	u64 fps;
-	struct v4l2_fract timeperframe;
 	const struct venus_format *fmt_out;
 	const struct venus_format *fmt_cap;
 	unsigned int num_input_bufs;
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index c1d5f94e16b4..e160a5508154 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -471,10 +471,12 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 		return -EINVAL;
 
 	memset(cap->reserved, 0, sizeof(cap->reserved));
-	if (!timeperframe->denominator)
-		timeperframe->denominator = inst->timeperframe.denominator;
-	if (!timeperframe->numerator)
-		timeperframe->numerator = inst->timeperframe.numerator;
+
+	if (!timeperframe->numerator || !timeperframe->denominator) {
+		timeperframe->numerator = 1;
+		timeperframe->denominator = inst->fps;
+	}
+
 	cap->readbuffers = 0;
 	cap->extendedmode = 0;
 	cap->capability = V4L2_CAP_TIMEPERFRAME;
@@ -487,7 +489,8 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 	fps = min(VENUS_MAX_FPS, fps);
 
 	inst->fps = fps;
-	inst->timeperframe = *timeperframe;
+	timeperframe->numerator = 1;
+	timeperframe->denominator = inst->fps;
 
 	return 0;
 }
@@ -1612,8 +1615,6 @@ static void vdec_inst_init(struct venus_inst *inst)
 	inst->out_width = frame_width_min(inst);
 	inst->out_height = frame_height_min(inst);
 	inst->fps = 30;
-	inst->timeperframe.numerator = 1;
-	inst->timeperframe.denominator = 30;
 	inst->opb_buftype = HFI_BUFFER_OUTPUT;
 }
 
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 943d432b6568..17bec44c9825 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -401,10 +401,10 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 
 	memset(out->reserved, 0, sizeof(out->reserved));
 
-	if (!timeperframe->denominator)
-		timeperframe->denominator = inst->timeperframe.denominator;
-	if (!timeperframe->numerator)
-		timeperframe->numerator = inst->timeperframe.numerator;
+	if (!timeperframe->numerator || !timeperframe->denominator) {
+		timeperframe->numerator = 1;
+		timeperframe->denominator = inst->fps;
+	}
 
 	out->capability = V4L2_CAP_TIMEPERFRAME;
 
@@ -416,8 +416,9 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 	do_div(fps, us_per_frame);
 	fps = min(VENUS_MAX_FPS, fps);
 
-	inst->timeperframe = *timeperframe;
 	inst->fps = fps;
+	timeperframe->numerator = 1;
+	timeperframe->denominator = inst->fps;
 
 	return 0;
 }
@@ -431,7 +432,8 @@ static int venc_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 		return -EINVAL;
 
 	a->parm.output.capability |= V4L2_CAP_TIMEPERFRAME;
-	a->parm.output.timeperframe = inst->timeperframe;
+	a->parm.output.timeperframe.numerator = 1;
+	a->parm.output.timeperframe.denominator = inst->fps;
 
 	return 0;
 }
@@ -1454,8 +1456,6 @@ static void venc_inst_init(struct venus_inst *inst)
 	inst->out_width = 1280;
 	inst->out_height = 720;
 	inst->fps = 15;
-	inst->timeperframe.numerator = 1;
-	inst->timeperframe.denominator = 15;
 	inst->hfi_codec = HFI_VIDEO_CODEC_H264;
 }
 

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v6 5/6] media: venus: venc: Make the range of us_per_frame explicit

[Ricardo Ribalda]

Fps bigger than 0.000232829 fps, this fits in a 32 bit us_per_frame.
There is no need to do a 64 bit division here.

Also, the driver only works with whole fps.

Found with cocci:
drivers/media/platform/qcom/venus/venc.c:418:1-7: WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/platform/qcom/venus/venc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 17bec44c9825..2c1836712362 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -412,8 +412,7 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 	do_div(us_per_frame, timeperframe->denominator);
 
 	us_per_frame = max(USEC_PER_SEC, us_per_frame);
-	fps = (u64)USEC_PER_SEC;
-	do_div(fps, us_per_frame);
+	fps = USEC_PER_SEC / (u32)us_per_frame;
 	fps = min(VENUS_MAX_FPS, fps);
 
 	inst->fps = fps;

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v6 6/6] media: venus: vdec: Make the range of us_per_frame explicit

[Ricardo Ribalda]

Fps bigger than 0.000232829 fps, this fits in a 32 bit us_per_frame.
There is no need to do a 64 bit division here.
Also, the driver only works with whole fps.

Found by cocci:
drivers/media/platform/qcom/venus/vdec.c:488:1-7: WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index e160a5508154..aa9ba38186b8 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -484,8 +484,7 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 	do_div(us_per_frame, timeperframe->denominator);
 
 	us_per_frame = max(USEC_PER_SEC, us_per_frame);
-	fps = (u64)USEC_PER_SEC;
-	do_div(fps, us_per_frame);
+	fps = USEC_PER_SEC / (u32)us_per_frame;
 	fps = min(VENUS_MAX_FPS, fps);
 
 	inst->fps = fps;

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.

[Bryan O'Donoghue]

On 11/01/2025 09:55, Ricardo Ribalda wrote:
> The driver uses "whole" fps in all its calculations (e.g. in
> load_per_instance()). Those calculation expect an fps bigger than 1, and
> not big enough to overflow.
> 
> Clamp the value if the user provides a parm that will result in an invalid
> fps.
> 
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> Closes: https://lore.kernel.org/linux-media/f11653a7-bc49-48cd-9cdb-1659147453e4@xs4all.nl/T/#m91cd962ac942834654f94c92206e2f85ff7d97f0
> Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>   drivers/media/platform/qcom/venus/core.h | 2 ++
>   drivers/media/platform/qcom/venus/vdec.c | 5 ++---
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 44f1c3bc4186..afae2b9fdaf7 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -28,6 +28,8 @@
>   #define VIDC_RESETS_NUM_MAX		2
>   #define VIDC_MAX_HIER_CODING_LAYER 6
>   
> +#define VENUS_MAX_FPS			240
> +
>   extern int venus_fw_debug;
>   
>   struct freq_tbl {
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 98c22b9f9372..c1d5f94e16b4 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -481,11 +481,10 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>   	us_per_frame = timeperframe->numerator * (u64)USEC_PER_SEC;
>   	do_div(us_per_frame, timeperframe->denominator);
>   
> -	if (!us_per_frame)
> -		return -EINVAL;
> -
> +	us_per_frame = max(USEC_PER_SEC, us_per_frame);
>   	fps = (u64)USEC_PER_SEC;
>   	do_div(fps, us_per_frame);
> +	fps = min(VENUS_MAX_FPS, fps);
>   
>   	inst->fps = fps;
>   	inst->timeperframe = *timeperframe;
> 

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # qrb5615-rb5
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH v6 6/6] media: venus: vdec: Make the range of us_per_frame explicit

[Bryan O'Donoghue]

On 11/01/2025 09:55, Ricardo Ribalda wrote:
> Fps bigger than 0.000232829 fps, this fits in a 32 bit us_per_frame.
> There is no need to do a 64 bit division here.
> Also, the driver only works with whole fps.
> 
> Found by cocci:
> drivers/media/platform/qcom/venus/vdec.c:488:1-7: WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>   drivers/media/platform/qcom/venus/vdec.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index e160a5508154..aa9ba38186b8 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -484,8 +484,7 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>   	do_div(us_per_frame, timeperframe->denominator);
>   
>   	us_per_frame = max(USEC_PER_SEC, us_per_frame);
> -	fps = (u64)USEC_PER_SEC;
> -	do_div(fps, us_per_frame);
> +	fps = USEC_PER_SEC / (u32)us_per_frame;
>   	fps = min(VENUS_MAX_FPS, fps);
>   
>   	inst->fps = fps;
> 

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # qrb5615-rb5
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH v6 0/6] media: Fix coccinelle warning/errors

[Bryan O'Donoghue]

On 11/01/2025 09:55, Ricardo Ribalda wrote:
> These is the last set of patches to fix all the relevant patchwork
> warnings (TM).
> 
> --
> 
> ---
> Changes in v6:
> - Improve comments for tda10048, thanks Kosta.
> - Link to v5: https://lore.kernel.org/r/20250107-fix-cocci-v5-0-b26da641f730@chromium.org
> 
> Changes in v5:
> - venus: Ignore fps > 240
> - venus: Clamp invalid fps instead of -EINVAL
> - Link to v4: https://lore.kernel.org/r/20250106-fix-cocci-v4-0-3c8eb97995ba@chromium.org
> 
> Changes in v4:
> - Remove all merged patches
> - Improve commit messages.
> - media: Remove timeperframe from inst
> - Ignore 0 fps (Thanks Hans)
> - Link to v3: https://lore.kernel.org/r/20240429-fix-cocci-v3-0-3c4865f5a4b0@chromium.org
> 
> Changes in v3: Thanks Bryan, Dan, Markus, Sakary and Hans
> - Improve commit messages.
> - Use div64_u64 when possible
> - Link to v2: https://lore.kernel.org/r/20240419-fix-cocci-v2-0-2119e692309c@chromium.org
> 
> Changes in v2:
> - Remove all the min() retval, and send a patch for cocci:  https://lore.kernel.org/lkml/20240415-minimax-v1-1-5feb20d66a79@chromium.org/T/#u
> - platform_get_irq() cannot return 0, fix that (Thanks Dan).
> - Fix stb0800 patch. chip_id can be 0 (Thanks Dan).
> - Use runtime (IS_ENABLED), code looks nicer. (Thanks Dan).
> - Do not replace do_div for venus (Thanks Dan).
> - Do not replace do_div for tda10048 (Thanks Dan).
> - Link to v1: https://lore.kernel.org/r/20240415-fix-cocci-v1-0-477afb23728b@chromium.org
> 
> To: Mauro Carvalho Chehab <mchehab@kernel.org>
> To: Stanimir Varbanov <stanimir.k.varbanov@gmail.com>
> To: Vikash Garodia <quic_vgarodia@quicinc.com>
> To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> To: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: linux-media@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-msm@vger.kernel.org
> 
> ---
> Ricardo Ribalda (6):
>        media: dvb-frontends: tda10048: Make the range of z explicit.
>        media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.
>        media: venus: venc: Clamp parm smaller than 1fps and bigger than 240
>        media: venus: Remove timeperframe from inst
>        media: venus: venc: Make the range of us_per_frame explicit
>        media: venus: vdec: Make the range of us_per_frame explicit
> 
>   drivers/media/dvb-frontends/tda10048.c   |  8 +++++++-
>   drivers/media/platform/qcom/venus/core.h |  4 ++--
>   drivers/media/platform/qcom/venus/vdec.c | 23 +++++++++++------------
>   drivers/media/platform/qcom/venus/venc.c | 24 +++++++++++-------------
>   4 files changed, 31 insertions(+), 28 deletions(-)
> ---
> base-commit: 4db312bbce420e3f874302549db072211e03c569
> change-id: 20240415-fix-cocci-2df3ef22a6f7
> 
> Best regards,

@Vikash || @Dikshita

Are you happy enough with these changes for venus ?

---
bod

Re: [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.

[Vikash Garodia]

On 1/11/2025 3:25 PM, Ricardo Ribalda wrote:
> The driver uses "whole" fps in all its calculations (e.g. in
> load_per_instance()). Those calculation expect an fps bigger than 1, and
> not big enough to overflow.
> 
> Clamp the value if the user provides a parm that will result in an invalid
> fps.
> 
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> Closes: https://lore.kernel.org/linux-media/f11653a7-bc49-48cd-9cdb-1659147453e4@xs4all.nl/T/#m91cd962ac942834654f94c92206e2f85ff7d97f0
> Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/platform/qcom/venus/core.h | 2 ++
>  drivers/media/platform/qcom/venus/vdec.c | 5 ++---
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 44f1c3bc4186..afae2b9fdaf7 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -28,6 +28,8 @@
>  #define VIDC_RESETS_NUM_MAX		2
>  #define VIDC_MAX_HIER_CODING_LAYER 6
>  
> +#define VENUS_MAX_FPS			240
> +
>  extern int venus_fw_debug;
>  
>  struct freq_tbl {
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 98c22b9f9372..c1d5f94e16b4 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -481,11 +481,10 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  	us_per_frame = timeperframe->numerator * (u64)USEC_PER_SEC;
>  	do_div(us_per_frame, timeperframe->denominator);
>  
> -	if (!us_per_frame)
> -		return -EINVAL;
> -
> +	us_per_frame = max(USEC_PER_SEC, us_per_frame);
This logic changes the actual fps from client. Consider a regular encode usecase
from client setting an fps as 30. The "max(USEC_PER_SEC, us_per_frame)" would
override it to USEC_PER_SEC and then the subsequent logic would eventually make
fps to 1.
Please make it conditional to handle the 0 fps case, i guess that the objective
in above code, something like below
if (!us_per_frame)
  us_per_frame = USEC_PER_SEC;

Regards,
Vikash
>  	fps = (u64)USEC_PER_SEC;
>  	do_div(fps, us_per_frame);
> +	fps = min(VENUS_MAX_FPS, fps);
>  
>  	inst->fps = fps;
>  	inst->timeperframe = *timeperframe;
> 

Re: [PATCH v6 3/6] media: venus: venc: Clamp parm smaller than 1fps and bigger than 240

[Vikash Garodia]

On 1/11/2025 3:25 PM, Ricardo Ribalda wrote:
> The driver uses "whole" fps in all its calculations (e.g. in
> load_per_instance()). Those calculation expect an fps bigger than 1, and
> not big enough to overflow.
> 
> Clamp the parm if the user provides a value that will result in an invalid
> fps.
> 
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> Closes: https://lore.kernel.org/linux-media/f11653a7-bc49-48cd-9cdb-1659147453e4@xs4all.nl/T/#m91cd962ac942834654f94c92206e2f85ff7d97f0
> Fixes: aaaa93eda64b ("[media] media: venus: venc: add video encoder files")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/platform/qcom/venus/venc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index c1c543535aaf..943d432b6568 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -411,11 +411,10 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  	us_per_frame = timeperframe->numerator * (u64)USEC_PER_SEC;
>  	do_div(us_per_frame, timeperframe->denominator);
>  
> -	if (!us_per_frame)
> -		return -EINVAL;
> -
> +	us_per_frame = max(USEC_PER_SEC, us_per_frame);
Same comment[1] made on 2/6 patch is applicable here as well.

[1]
https://patchwork.kernel.org/project/linux-arm-msm/patch/20250111-fix-cocci-v6-2-1aa7842006cc@chromium.org/

Regards,
Vikash

>  	fps = (u64)USEC_PER_SEC;
>  	do_div(fps, us_per_frame);
> +	fps = min(VENUS_MAX_FPS, fps);
>  
>  	inst->timeperframe = *timeperframe;
>  	inst->fps = fps;
> 

Re: [PATCH v6 4/6] media: venus: Remove timeperframe from inst

[Vikash Garodia]

On 1/11/2025 3:25 PM, Ricardo Ribalda wrote:
> The driver only cares about whole fps. We can infer the timeperframe
> from the fps field. Remove the redundant field.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/platform/qcom/venus/core.h |  2 --
>  drivers/media/platform/qcom/venus/vdec.c | 15 ++++++++-------
>  drivers/media/platform/qcom/venus/venc.c | 16 ++++++++--------
>  3 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index afae2b9fdaf7..1d4fd5cc75d9 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -407,7 +407,6 @@ enum venus_inst_modes {
>   * @tss:		timestamp metadata
>   * @payloads:		cache plane payload to use it for clock/BW scaling
>   * @fps:		holds current FPS
> - * @timeperframe:	holds current time per frame structure
>   * @fmt_out:	a reference to output format structure
>   * @fmt_cap:	a reference to capture format structure
>   * @num_input_bufs:	holds number of input buffers
> @@ -478,7 +477,6 @@ struct venus_inst {
>  	struct venus_ts_metadata tss[VIDEO_MAX_FRAME];
>  	unsigned long payloads[VIDEO_MAX_FRAME];
>  	u64 fps;
> -	struct v4l2_fract timeperframe;
>  	const struct venus_format *fmt_out;
>  	const struct venus_format *fmt_cap;
>  	unsigned int num_input_bufs;
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index c1d5f94e16b4..e160a5508154 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -471,10 +471,12 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  		return -EINVAL;
>  
>  	memset(cap->reserved, 0, sizeof(cap->reserved));
> -	if (!timeperframe->denominator)
> -		timeperframe->denominator = inst->timeperframe.denominator;
> -	if (!timeperframe->numerator)
> -		timeperframe->numerator = inst->timeperframe.numerator;
> +
> +	if (!timeperframe->numerator || !timeperframe->denominator) {
> +		timeperframe->numerator = 1;
> +		timeperframe->denominator = inst->fps;
> +	}
> +
>  	cap->readbuffers = 0;
>  	cap->extendedmode = 0;
>  	cap->capability = V4L2_CAP_TIMEPERFRAME;
> @@ -487,7 +489,8 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  	fps = min(VENUS_MAX_FPS, fps);
>  
>  	inst->fps = fps;
> -	inst->timeperframe = *timeperframe;
> +	timeperframe->numerator = 1;
> +	timeperframe->denominator = inst->fps;
>  
>  	return 0;
>  }
> @@ -1612,8 +1615,6 @@ static void vdec_inst_init(struct venus_inst *inst)
>  	inst->out_width = frame_width_min(inst);
>  	inst->out_height = frame_height_min(inst);
>  	inst->fps = 30;
> -	inst->timeperframe.numerator = 1;
> -	inst->timeperframe.denominator = 30;
>  	inst->opb_buftype = HFI_BUFFER_OUTPUT;
>  }
>  
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 943d432b6568..17bec44c9825 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -401,10 +401,10 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  
>  	memset(out->reserved, 0, sizeof(out->reserved));
>  
> -	if (!timeperframe->denominator)
> -		timeperframe->denominator = inst->timeperframe.denominator;
> -	if (!timeperframe->numerator)
> -		timeperframe->numerator = inst->timeperframe.numerator;
> +	if (!timeperframe->numerator || !timeperframe->denominator) {
> +		timeperframe->numerator = 1;
> +		timeperframe->denominator = inst->fps;
> +	}
>  
>  	out->capability = V4L2_CAP_TIMEPERFRAME;
>  
> @@ -416,8 +416,9 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  	do_div(fps, us_per_frame);
>  	fps = min(VENUS_MAX_FPS, fps);
>  
> -	inst->timeperframe = *timeperframe;
>  	inst->fps = fps;
> +	timeperframe->numerator = 1;
> +	timeperframe->denominator = inst->fps;
>  
>  	return 0;
>  }
> @@ -431,7 +432,8 @@ static int venc_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  		return -EINVAL;
>  
>  	a->parm.output.capability |= V4L2_CAP_TIMEPERFRAME;
> -	a->parm.output.timeperframe = inst->timeperframe;
> +	a->parm.output.timeperframe.numerator = 1;
> +	a->parm.output.timeperframe.denominator = inst->fps;
>  
>  	return 0;
>  }
> @@ -1454,8 +1456,6 @@ static void venc_inst_init(struct venus_inst *inst)
>  	inst->out_width = 1280;
>  	inst->out_height = 720;
>  	inst->fps = 15;
> -	inst->timeperframe.numerator = 1;
> -	inst->timeperframe.denominator = 15;
>  	inst->hfi_codec = HFI_VIDEO_CODEC_H264;
>  }
>  
> 
Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>

Re: [PATCH v6 5/6] media: venus: venc: Make the range of us_per_frame explicit

[Vikash Garodia]

On 1/11/2025 3:25 PM, Ricardo Ribalda wrote:
> Fps bigger than 0.000232829 fps, this fits in a 32 bit us_per_frame.
> There is no need to do a 64 bit division here.
> 
> Also, the driver only works with whole fps.
> 
> Found with cocci:
> drivers/media/platform/qcom/venus/venc.c:418:1-7: WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/platform/qcom/venus/venc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 17bec44c9825..2c1836712362 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -412,8 +412,7 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  	do_div(us_per_frame, timeperframe->denominator);
>  
>  	us_per_frame = max(USEC_PER_SEC, us_per_frame);
> -	fps = (u64)USEC_PER_SEC;
> -	do_div(fps, us_per_frame);
> +	fps = USEC_PER_SEC / (u32)us_per_frame;
>  	fps = min(VENUS_MAX_FPS, fps);
>  
>  	inst->fps = fps;
> 
Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>

Re: [PATCH v6 6/6] media: venus: vdec: Make the range of us_per_frame explicit

[Vikash Garodia]

On 1/11/2025 3:25 PM, Ricardo Ribalda wrote:
> Fps bigger than 0.000232829 fps, this fits in a 32 bit us_per_frame.
> There is no need to do a 64 bit division here.
> Also, the driver only works with whole fps.
> 
> Found by cocci:
> drivers/media/platform/qcom/venus/vdec.c:488:1-7: WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index e160a5508154..aa9ba38186b8 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -484,8 +484,7 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  	do_div(us_per_frame, timeperframe->denominator);
>  
>  	us_per_frame = max(USEC_PER_SEC, us_per_frame);
> -	fps = (u64)USEC_PER_SEC;
> -	do_div(fps, us_per_frame);
> +	fps = USEC_PER_SEC / (u32)us_per_frame;
>  	fps = min(VENUS_MAX_FPS, fps);
>  
>  	inst->fps = fps;
> 
Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>

Re: [PATCH v6 0/6] media: Fix coccinelle warning/errors

[Vikash Garodia]

Hi Ricardo,

On 1/11/2025 3:25 PM, Ricardo Ribalda wrote:
> These is the last set of patches to fix all the relevant patchwork
> warnings (TM).
> 
> --
> 
> ---
> Changes in v6:
> - Improve comments for tda10048, thanks Kosta.
> - Link to v5: https://lore.kernel.org/r/20250107-fix-cocci-v5-0-b26da641f730@chromium.org
> 
> Changes in v5:
> - venus: Ignore fps > 240
> - venus: Clamp invalid fps instead of -EINVAL
> - Link to v4: https://lore.kernel.org/r/20250106-fix-cocci-v4-0-3c8eb97995ba@chromium.org
> 
> Changes in v4:
> - Remove all merged patches
> - Improve commit messages.
> - media: Remove timeperframe from inst
> - Ignore 0 fps (Thanks Hans)
> - Link to v3: https://lore.kernel.org/r/20240429-fix-cocci-v3-0-3c4865f5a4b0@chromium.org
> 
> Changes in v3: Thanks Bryan, Dan, Markus, Sakary and Hans
> - Improve commit messages.
> - Use div64_u64 when possible
> - Link to v2: https://lore.kernel.org/r/20240419-fix-cocci-v2-0-2119e692309c@chromium.org
> 
> Changes in v2:
> - Remove all the min() retval, and send a patch for cocci:  https://lore.kernel.org/lkml/20240415-minimax-v1-1-5feb20d66a79@chromium.org/T/#u
> - platform_get_irq() cannot return 0, fix that (Thanks Dan).
> - Fix stb0800 patch. chip_id can be 0 (Thanks Dan).
> - Use runtime (IS_ENABLED), code looks nicer. (Thanks Dan).
> - Do not replace do_div for venus (Thanks Dan).
> - Do not replace do_div for tda10048 (Thanks Dan).
> - Link to v1: https://lore.kernel.org/r/20240415-fix-cocci-v1-0-477afb23728b@chromium.org
> 
> To: Mauro Carvalho Chehab <mchehab@kernel.org>
> To: Stanimir Varbanov <stanimir.k.varbanov@gmail.com>
> To: Vikash Garodia <quic_vgarodia@quicinc.com>
> To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> To: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: linux-media@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-msm@vger.kernel.org
> 
> ---
> Ricardo Ribalda (6):
>       media: dvb-frontends: tda10048: Make the range of z explicit.
>       media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.
>       media: venus: venc: Clamp parm smaller than 1fps and bigger than 240
>       media: venus: Remove timeperframe from inst
>       media: venus: venc: Make the range of us_per_frame explicit
>       media: venus: vdec: Make the range of us_per_frame explicit
> 
>  drivers/media/dvb-frontends/tda10048.c   |  8 +++++++-
>  drivers/media/platform/qcom/venus/core.h |  4 ++--
>  drivers/media/platform/qcom/venus/vdec.c | 23 +++++++++++------------
>  drivers/media/platform/qcom/venus/venc.c | 24 +++++++++++-------------
>  4 files changed, 31 insertions(+), 28 deletions(-)
> ---

Apologies for delay in review for this series.

Regards,
Vikash

Re: [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.

[Ricardo Ribalda]

Hi Vikash

On Mon, 16 Jun 2025 at 13:04, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
>
>
> On 1/11/2025 3:25 PM, Ricardo Ribalda wrote:
> > The driver uses "whole" fps in all its calculations (e.g. in
> > load_per_instance()). Those calculation expect an fps bigger than 1, and
> > not big enough to overflow.
> >
> > Clamp the value if the user provides a parm that will result in an invalid
> > fps.
> >
> > Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> > Closes: https://lore.kernel.org/linux-media/f11653a7-bc49-48cd-9cdb-1659147453e4@xs4all.nl/T/#m91cd962ac942834654f94c92206e2f85ff7d97f0
> > Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/platform/qcom/venus/core.h | 2 ++
> >  drivers/media/platform/qcom/venus/vdec.c | 5 ++---
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> > index 44f1c3bc4186..afae2b9fdaf7 100644
> > --- a/drivers/media/platform/qcom/venus/core.h
> > +++ b/drivers/media/platform/qcom/venus/core.h
> > @@ -28,6 +28,8 @@
> >  #define VIDC_RESETS_NUM_MAX          2
> >  #define VIDC_MAX_HIER_CODING_LAYER 6
> >
> > +#define VENUS_MAX_FPS                        240
> > +
> >  extern int venus_fw_debug;
> >
> >  struct freq_tbl {
> > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> > index 98c22b9f9372..c1d5f94e16b4 100644
> > --- a/drivers/media/platform/qcom/venus/vdec.c
> > +++ b/drivers/media/platform/qcom/venus/vdec.c
> > @@ -481,11 +481,10 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> >       us_per_frame = timeperframe->numerator * (u64)USEC_PER_SEC;
> >       do_div(us_per_frame, timeperframe->denominator);
> >
> > -     if (!us_per_frame)
> > -             return -EINVAL;
> > -
> > +     us_per_frame = max(USEC_PER_SEC, us_per_frame);
> This logic changes the actual fps from client. Consider a regular encode usecase
> from client setting an fps as 30. The "max(USEC_PER_SEC, us_per_frame)" would
> override it to USEC_PER_SEC and then the subsequent logic would eventually make
> fps to 1.
> Please make it conditional to handle the 0 fps case, i guess that the objective
> in above code, something like below
> if (!us_per_frame)
>   us_per_frame = USEC_PER_SEC;

You are correct. Thanks for catching it!

I think I prefer:
us_per_frame = clamp(us_per_frame, 1, USEC_PER_SEC);

Regards



>
> Regards,
> Vikash
> >       fps = (u64)USEC_PER_SEC;
> >       do_div(fps, us_per_frame);
> > +     fps = min(VENUS_MAX_FPS, fps);
> >
> >       inst->fps = fps;
> >       inst->timeperframe = *timeperframe;
> >



--
Ricardo Ribalda

Re: [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.

[Vikash Garodia]

On 6/16/2025 5:14 PM, Ricardo Ribalda wrote:
> Hi Vikash
> 
> On Mon, 16 Jun 2025 at 13:04, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
>>
>>
>> On 1/11/2025 3:25 PM, Ricardo Ribalda wrote:
>>> The driver uses "whole" fps in all its calculations (e.g. in
>>> load_per_instance()). Those calculation expect an fps bigger than 1, and
>>> not big enough to overflow.
>>>
>>> Clamp the value if the user provides a parm that will result in an invalid
>>> fps.
>>>
>>> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
>>> Closes: https://lore.kernel.org/linux-media/f11653a7-bc49-48cd-9cdb-1659147453e4@xs4all.nl/T/#m91cd962ac942834654f94c92206e2f85ff7d97f0
>>> Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>>  drivers/media/platform/qcom/venus/core.h | 2 ++
>>>  drivers/media/platform/qcom/venus/vdec.c | 5 ++---
>>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>>> index 44f1c3bc4186..afae2b9fdaf7 100644
>>> --- a/drivers/media/platform/qcom/venus/core.h
>>> +++ b/drivers/media/platform/qcom/venus/core.h
>>> @@ -28,6 +28,8 @@
>>>  #define VIDC_RESETS_NUM_MAX          2
>>>  #define VIDC_MAX_HIER_CODING_LAYER 6
>>>
>>> +#define VENUS_MAX_FPS                        240
>>> +
>>>  extern int venus_fw_debug;
>>>
>>>  struct freq_tbl {
>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
>>> index 98c22b9f9372..c1d5f94e16b4 100644
>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>> @@ -481,11 +481,10 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>>>       us_per_frame = timeperframe->numerator * (u64)USEC_PER_SEC;
>>>       do_div(us_per_frame, timeperframe->denominator);
>>>
>>> -     if (!us_per_frame)
>>> -             return -EINVAL;
>>> -
>>> +     us_per_frame = max(USEC_PER_SEC, us_per_frame);
>> This logic changes the actual fps from client. Consider a regular encode usecase
>> from client setting an fps as 30. The "max(USEC_PER_SEC, us_per_frame)" would
>> override it to USEC_PER_SEC and then the subsequent logic would eventually make
>> fps to 1.
>> Please make it conditional to handle the 0 fps case, i guess that the objective
>> in above code, something like below
>> if (!us_per_frame)
>>   us_per_frame = USEC_PER_SEC;
> 
> You are correct. Thanks for catching it!
> 
> I think I prefer:
> us_per_frame = clamp(us_per_frame, 1, USEC_PER_SEC);
This is good.

Regards,
Vikash
> 
> Regards
> 
> 
> 
>>
>> Regards,
>> Vikash
>>>       fps = (u64)USEC_PER_SEC;
>>>       do_div(fps, us_per_frame);
>>> +     fps = min(VENUS_MAX_FPS, fps);
>>>
>>>       inst->fps = fps;
>>>       inst->timeperframe = *timeperframe;
>>>
> 
> 
> 
> --
> Ricardo Ribalda