Hi Hugues,
thanks very much for testing
On Thu, Apr 07, 2022 at 06:25:25PM +0200, Hugues FRUCHET - FOSS wrote:
> Hi Jacopo,
>
> On 2/24/22 10:43 AM, Jacopo Mondi wrote:
> > In MIPI mode the frame rate control is performed by adjusting the
> > frame blankings and the s_frame_interval function is not used anymore.
> >
> > Only check for the per-mode supported frame rate in DVP mode and do not
> > restrict MIPI mode.
> >
> > Disallow enum/s/g_frame_interval if the chip is used in MIPI mode.
>
> This is breaking userspace which set framerate through media-ctl:
> media-ctl -d /dev/media0 --set-v4l2 "'ov5640
> 1-003c':0[fmt:JPEG_1X8/640x480@1/15 field:none]"
> because of unsupported framerate, all the rest is ignored (resolution and
> format).
>
> I can understand use of vblank to tune framerate but I would expect that
> compatibility with frame interval setting is kept, it's far more simple for
> an application to set the frame interval versus finding the right vblank to
> apply (not straightforward...)
I understand it might seem easier to state what FPS you want instead
of going through calculations, but I think the frame_interval ioctls
are actually mis-leading and should be discouraged for sensor drivers
(and consequentially for userspace).
frame_interval encourages driver developers to fix on a usually limited
set of supported modes, which limits the actual sensor capabilities to
a few pre-defined modes.
Drivers that support frame rate handling through frame_interval
usually do not expose configurable blankings, which has a direct
impact on the maximum achievable exposure time and should be in
control of userspace.
That said, I think it's maintainer's call to decide when moving to a
different API is considered a user-space breakage or not :)
>
> On my side I have reverted this patch and added support of both, see patch
> proposal in reply to [PATCH v5 16/27] media: ov5640: Add VBLANK control.
>
>
> >
> > While at it re-indent one function which whose parameters were wrongly
> > aligned.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/ov5640.c | 18 ++++++++++++++++--
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index baf368a39e0f..6b955163eb4d 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -2005,9 +2005,14 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
> > (mode->width != width || mode->height != height)))
> > return NULL;
> > - /* Check to see if the current mode exceeds the max frame rate */
> > + /*
> > + * Check to see if the current mode exceeds the max frame rate.
> > + * Only DVP mode uses the frame rate set by s_frame_interval, MIPI
> > + * mode controls framerate by setting blankings.
> > + */
> > timings = &mode->dvp_timings;
> > - if (ov5640_framerates[fr] > ov5640_framerates[timings->max_fps])
> > + if (!ov5640_is_csi2(sensor) &&
> > + ov5640_framerates[fr] > ov5640_framerates[timings->max_fps])
> > return NULL;
> > return mode;
> > @@ -3439,6 +3444,9 @@ static int ov5640_enum_frame_interval(
> > struct v4l2_fract tpf;
> > int ret;
> > + if (ov5640_is_csi2(sensor))
> > + return -EINVAL;
> > +
> > if (fie->pad != 0)
> > return -EINVAL;
> > if (fie->index >= OV5640_NUM_FRAMERATES)
> > @@ -3461,6 +3469,9 @@ static int ov5640_g_frame_interval(struct v4l2_subdev *sd,
> > {
> > struct ov5640_dev *sensor = to_ov5640_dev(sd);
> > + if (ov5640_is_csi2(sensor))
> > + return -EINVAL;
> > +
> > mutex_lock(&sensor->lock);
> > fi->interval = sensor->frame_interval;
> > mutex_unlock(&sensor->lock);
> > @@ -3475,6 +3486,9 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
> > const struct ov5640_mode_info *mode;
> > int frame_rate, ret = 0;
> > + if (ov5640_is_csi2(sensor))
> > + return -EINVAL;
> > +
> > if (fi->pad != 0)
> > return -EINVAL;
> >
