Hi Nicolas
On Mon, 16 Jun 2025 at 21:37, Nicolas Dufresne <nicolas@xxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > ---
> > 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
