diff mbox series

[v5,20/27] media: ov5640: Limit frame_interval to DVP mode only

Message ID 20220224094313.233347-21-jacopo@jmondi.org
State New
Headers show
Series media: ov5640: Rework the clock tree programming for MIPI | expand

Commit Message

Jacopo Mondi Feb. 24, 2022, 9:43 a.m. UTC
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.

While at it re-indent one function which whose parameters were wrongly
aligned.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov5640.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Hugues FRUCHET April 7, 2022, 4:25 p.m. UTC | #1
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...)

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@jmondi.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   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;
>   
>
Jacopo Mondi April 8, 2022, 4:08 p.m. UTC | #2
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@jmondi.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   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;
> >
diff mbox series

Patch

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;