linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/5] media: Fix coccinelle warning/errors
@ 2025-06-16 15:29 Ricardo Ribalda
  2025-06-16 15:29 ` [PATCH v7 1/5] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240 Ricardo Ribalda
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Ricardo Ribalda @ 2025-06-16 15:29 UTC (permalink / raw)
  To: Vikash Garodia, Dikshita Agarwal, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Stanimir Varbanov, Hans Verkuil
  Cc: linux-media, linux-arm-msm, linux-kernel, Ricardo Ribalda,
	Hans Verkuil

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

Changes in v7:
- Fix error in clamp logic (Thanks Vikash)
- Link to v6: https://lore.kernel.org/r/20250107-fix-cocci-v5-0-b26da641f730@chromium.org

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

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (5):
      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/platform/qcom/venus/core.h |  4 ++--
 drivers/media/platform/qcom/venus/vdec.c | 23 +++++++++++------------
 drivers/media/platform/qcom/venus/venc.c | 24 +++++++++++-------------
 3 files changed, 24 insertions(+), 27 deletions(-)
---
base-commit: 4d2c3d70799f5eb210003613766bbd113bbebc1a
change-id: 20250616-test-cd10d8184190

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v7 1/5] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.
  2025-06-16 15:29 [PATCH v7 0/5] media: Fix coccinelle warning/errors Ricardo Ribalda
@ 2025-06-16 15:29 ` Ricardo Ribalda
  2025-06-16 15:29 ` [PATCH v7 2/5] media: venus: venc: " Ricardo Ribalda
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Ricardo Ribalda @ 2025-06-16 15:29 UTC (permalink / raw)
  To: Vikash Garodia, Dikshita Agarwal, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Stanimir Varbanov, Hans Verkuil
  Cc: linux-media, linux-arm-msm, linux-kernel, Ricardo Ribalda,
	Hans Verkuil

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")
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # qrb5615-rb5
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
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 b412e0c5515a0916f6a8969ad3be01841e09edca..5b1ba1c69adba14c3560a4bc6d09435529f295a6 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 99ce5fd41577284098dca54c7772da0691cf29a5..fca27be61f4b869840904cc0577949635bc63cab 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 = clamp(us_per_frame, 1, USEC_PER_SEC);
 	fps = (u64)USEC_PER_SEC;
 	do_div(fps, us_per_frame);
+	fps = min(VENUS_MAX_FPS, fps);
 
 	inst->fps = fps;
 	inst->timeperframe = *timeperframe;

-- 
2.50.0.rc1.591.g9c95f17f64-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v7 2/5] media: venus: venc: Clamp parm smaller than 1fps and bigger than 240
  2025-06-16 15:29 [PATCH v7 0/5] media: Fix coccinelle warning/errors Ricardo Ribalda
  2025-06-16 15:29 ` [PATCH v7 1/5] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240 Ricardo Ribalda
@ 2025-06-16 15:29 ` Ricardo Ribalda
  2025-06-16 15:29 ` [PATCH v7 3/5] media: venus: Remove timeperframe from inst Ricardo Ribalda
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Ricardo Ribalda @ 2025-06-16 15:29 UTC (permalink / raw)
  To: Vikash Garodia, Dikshita Agarwal, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Stanimir Varbanov, Hans Verkuil
  Cc: linux-media, linux-arm-msm, linux-kernel, Ricardo Ribalda,
	Hans Verkuil

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 c7f8e37dba9b2218c0988dbfad1f7c04e055bbb0..b9ccee870c3d1238e04cef5e9344bd992d86d737 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 = clamp(us_per_frame, 1, USEC_PER_SEC);
 	fps = (u64)USEC_PER_SEC;
 	do_div(fps, us_per_frame);
+	fps = min(VENUS_MAX_FPS, fps);
 
 	inst->timeperframe = *timeperframe;
 	inst->fps = fps;

-- 
2.50.0.rc1.591.g9c95f17f64-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v7 3/5] media: venus: Remove timeperframe from inst
  2025-06-16 15:29 [PATCH v7 0/5] media: Fix coccinelle warning/errors Ricardo Ribalda
  2025-06-16 15:29 ` [PATCH v7 1/5] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240 Ricardo Ribalda
  2025-06-16 15:29 ` [PATCH v7 2/5] media: venus: venc: " Ricardo Ribalda
@ 2025-06-16 15:29 ` Ricardo Ribalda
  2025-06-16 19:37   ` Nicolas Dufresne
  2025-06-16 15:29 ` [PATCH v7 4/5] media: venus: venc: Make the range of us_per_frame explicit Ricardo Ribalda
  2025-06-16 15:29 ` [PATCH v7 5/5] media: venus: vdec: " Ricardo Ribalda
  4 siblings, 1 reply; 8+ messages in thread
From: Ricardo Ribalda @ 2025-06-16 15:29 UTC (permalink / raw)
  To: Vikash Garodia, Dikshita Agarwal, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Stanimir Varbanov, Hans Verkuil
  Cc: linux-media, linux-arm-msm, linux-kernel, Ricardo Ribalda

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

Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
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 5b1ba1c69adba14c3560a4bc6d09435529f295a6..9cfb860e01e752bf9856a3550f59c8c7b43647d2 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -413,7 +413,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
@@ -484,7 +483,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 fca27be61f4b869840904cc0577949635bc63cab..7d6612234d18a49573dc502d48ee61a900b63194 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;
 }
@@ -1622,8 +1625,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 b9ccee870c3d1238e04cef5e9344bd992d86d737..4979392aa20b6dc94895c7089878531b92b57754 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.50.0.rc1.591.g9c95f17f64-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v7 4/5] media: venus: venc: Make the range of us_per_frame explicit
  2025-06-16 15:29 [PATCH v7 0/5] media: Fix coccinelle warning/errors Ricardo Ribalda
                   ` (2 preceding siblings ...)
  2025-06-16 15:29 ` [PATCH v7 3/5] media: venus: Remove timeperframe from inst Ricardo Ribalda
@ 2025-06-16 15:29 ` Ricardo Ribalda
  2025-06-16 15:29 ` [PATCH v7 5/5] media: venus: vdec: " Ricardo Ribalda
  4 siblings, 0 replies; 8+ messages in thread
From: Ricardo Ribalda @ 2025-06-16 15:29 UTC (permalink / raw)
  To: Vikash Garodia, Dikshita Agarwal, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Stanimir Varbanov, Hans Verkuil
  Cc: linux-media, linux-arm-msm, linux-kernel, 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: Vikash Garodia <quic_vgarodia@quicinc.com>
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 4979392aa20b6dc94895c7089878531b92b57754..13395aaf06bbb1f381d809c18be61c5d6da85723 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 = clamp(us_per_frame, 1, USEC_PER_SEC);
-	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.50.0.rc1.591.g9c95f17f64-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v7 5/5] media: venus: vdec: Make the range of us_per_frame explicit
  2025-06-16 15:29 [PATCH v7 0/5] media: Fix coccinelle warning/errors Ricardo Ribalda
                   ` (3 preceding siblings ...)
  2025-06-16 15:29 ` [PATCH v7 4/5] media: venus: venc: Make the range of us_per_frame explicit Ricardo Ribalda
@ 2025-06-16 15:29 ` Ricardo Ribalda
  4 siblings, 0 replies; 8+ messages in thread
From: Ricardo Ribalda @ 2025-06-16 15:29 UTC (permalink / raw)
  To: Vikash Garodia, Dikshita Agarwal, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Stanimir Varbanov, Hans Verkuil
  Cc: linux-media, linux-arm-msm, linux-kernel, 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: Vikash Garodia <quic_vgarodia@quicinc.com>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # qrb5615-rb5
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 7d6612234d18a49573dc502d48ee61a900b63194..6042eb1e8705196b754dffc16bdb714378bb4cd4 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 = clamp(us_per_frame, 1, USEC_PER_SEC);
-	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.50.0.rc1.591.g9c95f17f64-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 3/5] media: venus: Remove timeperframe from inst
  2025-06-16 15:29 ` [PATCH v7 3/5] media: venus: Remove timeperframe from inst Ricardo Ribalda
@ 2025-06-16 19:37   ` Nicolas Dufresne
  2025-06-16 19:54     ` Ricardo Ribalda
  0 siblings, 1 reply; 8+ messages in thread
From: Nicolas Dufresne @ 2025-06-16 19:37 UTC (permalink / raw)
  To: Ricardo Ribalda, Vikash Garodia, Dikshita Agarwal,
	Bryan O'Donoghue, Mauro Carvalho Chehab, Stanimir Varbanov,
	Hans Verkuil
  Cc: linux-media, linux-arm-msm, linux-kernel

Hi Ricardo,

Le lundi 16 juin 2025 à 15:29 +0000, Ricardo Ribalda a écrit :
> The driver only cares about whole fps. We can infer the timeperframe
> from the fps field. Remove the redundant field.

I do have reserved about this change. Video standards commonly uses fractional
rates for videos. If my memory is correct, venus uses Q16 ... So with this change,
we now round all frame rate passed to encoders to an integer, which will introduce
error in the resulting bitrate.

Perhaps it was already broken, but if so, it should be fixed instead ?

regards,
Nicolas

> 
> Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> 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 5b1ba1c69adba14c3560a4bc6d09435529f295a6..9cfb860e01e752bf9856a3550f59c8c7b43647d2 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -413,7 +413,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
> @@ -484,7 +483,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 fca27be61f4b869840904cc0577949635bc63cab..7d6612234d18a49573dc502d48ee61a900b63194 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;
>  }
> @@ -1622,8 +1625,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 b9ccee870c3d1238e04cef5e9344bd992d86d737..4979392aa20b6dc94895c7089878531b92b57754 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;
>  }
>  

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 3/5] media: venus: Remove timeperframe from inst
  2025-06-16 19:37   ` Nicolas Dufresne
@ 2025-06-16 19:54     ` Ricardo Ribalda
  0 siblings, 0 replies; 8+ messages in thread
From: Ricardo Ribalda @ 2025-06-16 19:54 UTC (permalink / raw)
  To: Nicolas Dufresne
  Cc: Vikash Garodia, Dikshita Agarwal, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Stanimir Varbanov, Hans Verkuil,
	linux-media, linux-arm-msm, linux-kernel

Hi Nicolas


On Mon, 16 Jun 2025 at 21:37, Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Hi Ricardo,
>
> Le lundi 16 juin 2025 à 15:29 +0000, Ricardo Ribalda a écrit :
> > The driver only cares about whole fps. We can infer the timeperframe
> > from the fps field. Remove the redundant field.
>
> I do have reserved about this change. Video standards commonly uses fractional
> rates for videos. If my memory is correct, venus uses Q16 ... So with this change,
> we now round all frame rate passed to encoders to an integer, which will introduce
> error in the resulting bitrate.
>
> Perhaps it was already broken, but if so, it should be fixed instead ?

It seems like today it only supports integer frame rates, or at least
timeperframe is not used anywhere in the code, this is why I
implemented the change.

In other words, when the driver was returning a fractional it was lying (a bit).

I do not have the hardware to test this, the best thing I can offer is
to remove this patch from the set and rebase the code.

My main concern is to fix the "division errors" from the static analyzer :)

>
> regards,
> Nicolas
>
> >
> > Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> > 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 5b1ba1c69adba14c3560a4bc6d09435529f295a6..9cfb860e01e752bf9856a3550f59c8c7b43647d2 100644
> > --- a/drivers/media/platform/qcom/venus/core.h
> > +++ b/drivers/media/platform/qcom/venus/core.h
> > @@ -413,7 +413,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
> > @@ -484,7 +483,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 fca27be61f4b869840904cc0577949635bc63cab..7d6612234d18a49573dc502d48ee61a900b63194 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;
> >  }
> > @@ -1622,8 +1625,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 b9ccee870c3d1238e04cef5e9344bd992d86d737..4979392aa20b6dc94895c7089878531b92b57754 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;
> >  }
> >



-- 
Ricardo Ribalda

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-06-16 19:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-16 15:29 [PATCH v7 0/5] media: Fix coccinelle warning/errors Ricardo Ribalda
2025-06-16 15:29 ` [PATCH v7 1/5] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240 Ricardo Ribalda
2025-06-16 15:29 ` [PATCH v7 2/5] media: venus: venc: " Ricardo Ribalda
2025-06-16 15:29 ` [PATCH v7 3/5] media: venus: Remove timeperframe from inst Ricardo Ribalda
2025-06-16 19:37   ` Nicolas Dufresne
2025-06-16 19:54     ` Ricardo Ribalda
2025-06-16 15:29 ` [PATCH v7 4/5] media: venus: venc: Make the range of us_per_frame explicit Ricardo Ribalda
2025-06-16 15:29 ` [PATCH v7 5/5] media: venus: vdec: " Ricardo Ribalda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).