* [PATCH v6 1/6] media: dvb-frontends: tda10048: Make the range of z explicit.
2025-01-11 9:55 [PATCH v6 0/6] media: Fix coccinelle warning/errors Ricardo Ribalda
@ 2025-01-11 9:55 ` Ricardo Ribalda
2025-01-11 9:55 ` [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240 Ricardo Ribalda
` (6 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Ricardo Ribalda @ 2025-01-11 9:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Stanimir Varbanov, Vikash Garodia,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm, Kosta Stefanov,
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.
2025-01-11 9:55 [PATCH v6 0/6] media: Fix coccinelle warning/errors Ricardo Ribalda
2025-01-11 9:55 ` [PATCH v6 1/6] media: dvb-frontends: tda10048: Make the range of z explicit Ricardo Ribalda
@ 2025-01-11 9:55 ` Ricardo Ribalda
2025-01-15 1:14 ` Bryan O'Donoghue
2025-06-16 11:04 ` Vikash Garodia
2025-01-11 9:55 ` [PATCH v6 3/6] media: venus: venc: " Ricardo Ribalda
` (5 subsequent siblings)
7 siblings, 2 replies; 18+ messages in thread
From: Ricardo Ribalda @ 2025-01-11 9:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Stanimir Varbanov, Vikash Garodia,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.
2025-01-11 9:55 ` [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240 Ricardo Ribalda
@ 2025-01-15 1:14 ` Bryan O'Donoghue
2025-06-16 11:04 ` Vikash Garodia
1 sibling, 0 replies; 18+ messages in thread
From: Bryan O'Donoghue @ 2025-01-15 1:14 UTC (permalink / raw)
To: Ricardo Ribalda, Mauro Carvalho Chehab, Stanimir Varbanov,
Vikash Garodia, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.
2025-01-11 9:55 ` [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240 Ricardo Ribalda
2025-01-15 1:14 ` Bryan O'Donoghue
@ 2025-06-16 11:04 ` Vikash Garodia
2025-06-16 11:44 ` Ricardo Ribalda
1 sibling, 1 reply; 18+ messages in thread
From: Vikash Garodia @ 2025-06-16 11:04 UTC (permalink / raw)
To: Ricardo Ribalda, Mauro Carvalho Chehab, Stanimir Varbanov,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm
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;
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.
2025-06-16 11:04 ` Vikash Garodia
@ 2025-06-16 11:44 ` Ricardo Ribalda
2025-06-16 15:44 ` Vikash Garodia
0 siblings, 1 reply; 18+ messages in thread
From: Ricardo Ribalda @ 2025-06-16 11:44 UTC (permalink / raw)
To: Vikash Garodia
Cc: Mauro Carvalho Chehab, Stanimir Varbanov, Bryan O'Donoghue,
Hans Verkuil, linux-media, linux-kernel, linux-arm-msm
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240.
2025-06-16 11:44 ` Ricardo Ribalda
@ 2025-06-16 15:44 ` Vikash Garodia
0 siblings, 0 replies; 18+ messages in thread
From: Vikash Garodia @ 2025-06-16 15:44 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Mauro Carvalho Chehab, Stanimir Varbanov, Bryan O'Donoghue,
Hans Verkuil, linux-media, linux-kernel, linux-arm-msm
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 3/6] media: venus: venc: Clamp parm smaller than 1fps and bigger than 240
2025-01-11 9:55 [PATCH v6 0/6] media: Fix coccinelle warning/errors Ricardo Ribalda
2025-01-11 9:55 ` [PATCH v6 1/6] media: dvb-frontends: tda10048: Make the range of z explicit Ricardo Ribalda
2025-01-11 9:55 ` [PATCH v6 2/6] media: venus: vdec: Clamp parm smaller than 1fps and bigger than 240 Ricardo Ribalda
@ 2025-01-11 9:55 ` Ricardo Ribalda
2025-06-16 11:12 ` Vikash Garodia
2025-01-11 9:55 ` [PATCH v6 4/6] media: venus: Remove timeperframe from inst Ricardo Ribalda
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Ricardo Ribalda @ 2025-01-11 9:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Stanimir Varbanov, Vikash Garodia,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v6 3/6] media: venus: venc: Clamp parm smaller than 1fps and bigger than 240
2025-01-11 9:55 ` [PATCH v6 3/6] media: venus: venc: " Ricardo Ribalda
@ 2025-06-16 11:12 ` Vikash Garodia
0 siblings, 0 replies; 18+ messages in thread
From: Vikash Garodia @ 2025-06-16 11:12 UTC (permalink / raw)
To: Ricardo Ribalda, Mauro Carvalho Chehab, Stanimir Varbanov,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm
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;
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 4/6] media: venus: Remove timeperframe from inst
2025-01-11 9:55 [PATCH v6 0/6] media: Fix coccinelle warning/errors Ricardo Ribalda
` (2 preceding siblings ...)
2025-01-11 9:55 ` [PATCH v6 3/6] media: venus: venc: " Ricardo Ribalda
@ 2025-01-11 9:55 ` Ricardo Ribalda
2025-06-16 11:24 ` Vikash Garodia
2025-01-11 9:55 ` [PATCH v6 5/6] media: venus: venc: Make the range of us_per_frame explicit Ricardo Ribalda
` (3 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Ricardo Ribalda @ 2025-01-11 9:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Stanimir Varbanov, Vikash Garodia,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v6 4/6] media: venus: Remove timeperframe from inst
2025-01-11 9:55 ` [PATCH v6 4/6] media: venus: Remove timeperframe from inst Ricardo Ribalda
@ 2025-06-16 11:24 ` Vikash Garodia
0 siblings, 0 replies; 18+ messages in thread
From: Vikash Garodia @ 2025-06-16 11:24 UTC (permalink / raw)
To: Ricardo Ribalda, Mauro Carvalho Chehab, Stanimir Varbanov,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 5/6] media: venus: venc: Make the range of us_per_frame explicit
2025-01-11 9:55 [PATCH v6 0/6] media: Fix coccinelle warning/errors Ricardo Ribalda
` (3 preceding siblings ...)
2025-01-11 9:55 ` [PATCH v6 4/6] media: venus: Remove timeperframe from inst Ricardo Ribalda
@ 2025-01-11 9:55 ` Ricardo Ribalda
2025-06-16 11:31 ` Vikash Garodia
2025-01-11 9:55 ` [PATCH v6 6/6] media: venus: vdec: " Ricardo Ribalda
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Ricardo Ribalda @ 2025-01-11 9:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Stanimir Varbanov, Vikash Garodia,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v6 5/6] media: venus: venc: Make the range of us_per_frame explicit
2025-01-11 9:55 ` [PATCH v6 5/6] media: venus: venc: Make the range of us_per_frame explicit Ricardo Ribalda
@ 2025-06-16 11:31 ` Vikash Garodia
0 siblings, 0 replies; 18+ messages in thread
From: Vikash Garodia @ 2025-06-16 11:31 UTC (permalink / raw)
To: Ricardo Ribalda, Mauro Carvalho Chehab, Stanimir Varbanov,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 6/6] media: venus: vdec: Make the range of us_per_frame explicit
2025-01-11 9:55 [PATCH v6 0/6] media: Fix coccinelle warning/errors Ricardo Ribalda
` (4 preceding siblings ...)
2025-01-11 9:55 ` [PATCH v6 5/6] media: venus: venc: Make the range of us_per_frame explicit Ricardo Ribalda
@ 2025-01-11 9:55 ` Ricardo Ribalda
2025-01-15 1:17 ` Bryan O'Donoghue
2025-06-16 11:32 ` Vikash Garodia
2025-06-12 9:22 ` [PATCH v6 0/6] media: Fix coccinelle warning/errors Bryan O'Donoghue
2025-06-16 11:34 ` Vikash Garodia
7 siblings, 2 replies; 18+ messages in thread
From: Ricardo Ribalda @ 2025-01-11 9:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Stanimir Varbanov, Vikash Garodia,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v6 6/6] media: venus: vdec: Make the range of us_per_frame explicit
2025-01-11 9:55 ` [PATCH v6 6/6] media: venus: vdec: " Ricardo Ribalda
@ 2025-01-15 1:17 ` Bryan O'Donoghue
2025-06-16 11:32 ` Vikash Garodia
1 sibling, 0 replies; 18+ messages in thread
From: Bryan O'Donoghue @ 2025-01-15 1:17 UTC (permalink / raw)
To: Ricardo Ribalda, Mauro Carvalho Chehab, Stanimir Varbanov,
Vikash Garodia, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 6/6] media: venus: vdec: Make the range of us_per_frame explicit
2025-01-11 9:55 ` [PATCH v6 6/6] media: venus: vdec: " Ricardo Ribalda
2025-01-15 1:17 ` Bryan O'Donoghue
@ 2025-06-16 11:32 ` Vikash Garodia
1 sibling, 0 replies; 18+ messages in thread
From: Vikash Garodia @ 2025-06-16 11:32 UTC (permalink / raw)
To: Ricardo Ribalda, Mauro Carvalho Chehab, Stanimir Varbanov,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 0/6] media: Fix coccinelle warning/errors
2025-01-11 9:55 [PATCH v6 0/6] media: Fix coccinelle warning/errors Ricardo Ribalda
` (5 preceding siblings ...)
2025-01-11 9:55 ` [PATCH v6 6/6] media: venus: vdec: " Ricardo Ribalda
@ 2025-06-12 9:22 ` Bryan O'Donoghue
2025-06-16 11:34 ` Vikash Garodia
7 siblings, 0 replies; 18+ messages in thread
From: Bryan O'Donoghue @ 2025-06-12 9:22 UTC (permalink / raw)
To: Ricardo Ribalda, Mauro Carvalho Chehab, Stanimir Varbanov,
Vikash Garodia, Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm, Kosta Stefanov
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 0/6] media: Fix coccinelle warning/errors
2025-01-11 9:55 [PATCH v6 0/6] media: Fix coccinelle warning/errors Ricardo Ribalda
` (6 preceding siblings ...)
2025-06-12 9:22 ` [PATCH v6 0/6] media: Fix coccinelle warning/errors Bryan O'Donoghue
@ 2025-06-16 11:34 ` Vikash Garodia
7 siblings, 0 replies; 18+ messages in thread
From: Vikash Garodia @ 2025-06-16 11:34 UTC (permalink / raw)
To: Ricardo Ribalda, Mauro Carvalho Chehab, Stanimir Varbanov,
Bryan O'Donoghue, Hans Verkuil
Cc: linux-media, linux-kernel, linux-arm-msm, Kosta Stefanov
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
^ permalink raw reply [flat|nested] 18+ messages in thread