* [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).