diff mbox series

[3/3] media: i2c: max9286: Support manual framesync operation

Message ID 20211216220946.20771-4-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series media: i2c: max9286: Small new features | expand

Commit Message

Laurent Pinchart Dec. 16, 2021, 10:09 p.m. UTC 
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

The MAX9286 can generate a framesync signal to synchronize the cameras,
using an internal timer. Support this mode of operation and configure it
through the .s_frameinterval() operation. If the frame interval is not
0, framesync is switched to manual mode with the specified interval,
otherwise automatic mode is used.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 3 deletions(-)

Comments

Jacopo Mondi Dec. 17, 2021, 11:14 a.m. UTC | #1 
Hi Laurent,

On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> The MAX9286 can generate a framesync signal to synchronize the cameras,
> using an internal timer. Support this mode of operation and configure it
> through the .s_frameinterval() operation. If the frame interval is not
> 0, framesync is switched to manual mode with the specified interval,
> otherwise automatic mode is used.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index fa2f6a823fe6..dce1146635d2 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -174,6 +174,7 @@ struct max9286_priv {
>  	struct v4l2_ctrl *pixelrate;
>
>  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> +	struct v4l2_fract interval;
>
>  	/* Protects controls and fmt structures */
>  	struct mutex mutex;
> @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
>  	return 0;
>  }
>
> +static void max9286_set_fsync_period(struct max9286_priv *priv)
> +{
> +	u32 fsync;
> +
> +	if (!priv->interval.numerator || !priv->interval.denominator) {
> +		/*
> +		 * Special case, a null interval enables automatic FRAMESYNC
> +		 * mode. FRAMESYNC is taken from the slowest link.
> +		 */
> +		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> +			      MAX9286_FSYNCMETH_AUTO);
> +		return;
> +	}
> +
> +	/*
> +	 * Manual FRAMESYNC
> +	 *
> +	 * The FRAMESYNC generator is configured with a period expressed as a
> +	 * number of PCLK periods, which runs at 75MHz.
> +	 */
> +	fsync = div_u64(75000000ULL * priv->interval.numerator,
> +			priv->interval.denominator);

Don't we calculate the pixel rate dynamically based on the number of
enabled sources ?

> +
> +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
> +		      MAX9286_FSYNCMETH_MANUAL);
> +
> +	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
> +	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
> +	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 Subdev
>   */
> @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret;
>
>  	if (enable) {
> +		max9286_set_fsync_period(priv);
> +
>  		/*
>  		 * The frame sync between cameras is transmitted across the
>  		 * reverse channel as GPIO. We must open all channels while
> @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	return 0;
>  }
>
> +static int max9286_g_frame_interval(struct v4l2_subdev *sd,
> +				    struct v4l2_subdev_frame_interval *interval)
> +{
> +	struct max9286_priv *priv = sd_to_max9286(sd);
> +
> +	if (interval->pad == MAX9286_SRC_PAD)

Ah!

I would have expected, as this setting applies to all cameras, and
consequentially to the output image stream frame rate, that the
operation should have been done on the single source pad... Why is
this not the case ?

Thanks
  j


> +		return -EINVAL;
> +
> +	interval->interval = priv->interval;
> +
> +	return 0;
> +}
> +
> +static int max9286_s_frame_interval(struct v4l2_subdev *sd,
> +				    struct v4l2_subdev_frame_interval *interval)
> +{
> +	struct max9286_priv *priv = sd_to_max9286(sd);
> +
> +	if (interval->pad == MAX9286_SRC_PAD)
> +		return -EINVAL;
> +
> +	priv->interval = interval->interval;
> +
> +	return 0;
> +}
> +
>  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_mbus_code_enum *code)
> @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>
>  static const struct v4l2_subdev_video_ops max9286_video_ops = {
>  	.s_stream	= max9286_s_stream,
> +	.g_frame_interval = max9286_g_frame_interval,
> +	.s_frame_interval = max9286_s_frame_interval,
>  };
>
>  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
>  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
>  		      MAX9286_DATATYPE_YUV422_8BIT);
>
> -	/* Automatic: FRAMESYNC taken from the slowest Link. */
> -	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> -		      MAX9286_FSYNCMETH_AUTO);
> +	max9286_set_fsync_period(priv);
>
>  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
>  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Dec. 17, 2021, 4:05 p.m. UTC | #2 
Hi Jacopo,

On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote:
> On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > The MAX9286 can generate a framesync signal to synchronize the cameras,
> > using an internal timer. Support this mode of operation and configure it
> > through the .s_frameinterval() operation. If the frame interval is not
> > 0, framesync is switched to manual mode with the specified interval,
> > otherwise automatic mode is used.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 63 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index fa2f6a823fe6..dce1146635d2 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -174,6 +174,7 @@ struct max9286_priv {
> >  	struct v4l2_ctrl *pixelrate;
> >
> >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > +	struct v4l2_fract interval;
> >
> >  	/* Protects controls and fmt structures */
> >  	struct mutex mutex;
> > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> >  	return 0;
> >  }
> >
> > +static void max9286_set_fsync_period(struct max9286_priv *priv)
> > +{
> > +	u32 fsync;
> > +
> > +	if (!priv->interval.numerator || !priv->interval.denominator) {
> > +		/*
> > +		 * Special case, a null interval enables automatic FRAMESYNC
> > +		 * mode. FRAMESYNC is taken from the slowest link.
> > +		 */
> > +		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > +			      MAX9286_FSYNCMETH_AUTO);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Manual FRAMESYNC
> > +	 *
> > +	 * The FRAMESYNC generator is configured with a period expressed as a
> > +	 * number of PCLK periods, which runs at 75MHz.
> > +	 */
> > +	fsync = div_u64(75000000ULL * priv->interval.numerator,
> > +			priv->interval.denominator);
> 
> Don't we calculate the pixel rate dynamically based on the number of
> enabled sources ?

That's a good point, this hardcodes operation for 4 cameras. I'll see
how I can fix that.

> > +
> > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
> > +		      MAX9286_FSYNCMETH_MANUAL);
> > +
> > +	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
> > +	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
> > +	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * V4L2 Subdev
> >   */
> > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  	int ret;
> >
> >  	if (enable) {
> > +		max9286_set_fsync_period(priv);
> > +
> >  		/*
> >  		 * The frame sync between cameras is transmitted across the
> >  		 * reverse channel as GPIO. We must open all channels while
> > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  	return 0;
> >  }
> >
> > +static int max9286_g_frame_interval(struct v4l2_subdev *sd,
> > +				    struct v4l2_subdev_frame_interval *interval)
> > +{
> > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > +
> > +	if (interval->pad == MAX9286_SRC_PAD)
> 
> Ah!
> 
> I would have expected, as this setting applies to all cameras, and
> consequentially to the output image stream frame rate, that the
> operation should have been done on the single source pad... Why is
> this not the case ?

I went back and forth between the two, I think it would work fine on the
source pad as well. I'll give it a try.

> > +		return -EINVAL;
> > +
> > +	interval->interval = priv->interval;
> > +
> > +	return 0;
> > +}
> > +
> > +static int max9286_s_frame_interval(struct v4l2_subdev *sd,
> > +				    struct v4l2_subdev_frame_interval *interval)
> > +{
> > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > +
> > +	if (interval->pad == MAX9286_SRC_PAD)
> > +		return -EINVAL;
> > +
> > +	priv->interval = interval->interval;
> > +
> > +	return 0;
> > +}
> > +
> >  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> >  				  struct v4l2_subdev_state *sd_state,
> >  				  struct v4l2_subdev_mbus_code_enum *code)
> > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> >
> >  static const struct v4l2_subdev_video_ops max9286_video_ops = {
> >  	.s_stream	= max9286_s_stream,
> > +	.g_frame_interval = max9286_g_frame_interval,
> > +	.s_frame_interval = max9286_s_frame_interval,
> >  };
> >
> >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
> >  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> >  		      MAX9286_DATATYPE_YUV422_8BIT);
> >
> > -	/* Automatic: FRAMESYNC taken from the slowest Link. */
> > -	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > -		      MAX9286_FSYNCMETH_AUTO);
> > +	max9286_set_fsync_period(priv);
> >
> >  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> >  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
Laurent Pinchart Dec. 17, 2021, 5:49 p.m. UTC | #3 
Hi Jacopo,

On Fri, Dec 17, 2021 at 06:05:02PM +0200, Laurent Pinchart wrote:
> On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote:
> > On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote:
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > The MAX9286 can generate a framesync signal to synchronize the cameras,
> > > using an internal timer. Support this mode of operation and configure it
> > > through the .s_frameinterval() operation. If the frame interval is not
> > > 0, framesync is switched to manual mode with the specified interval,
> > > otherwise automatic mode is used.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 63 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index fa2f6a823fe6..dce1146635d2 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -174,6 +174,7 @@ struct max9286_priv {
> > >  	struct v4l2_ctrl *pixelrate;
> > >
> > >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > > +	struct v4l2_fract interval;
> > >
> > >  	/* Protects controls and fmt structures */
> > >  	struct mutex mutex;
> > > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> > >  	return 0;
> > >  }
> > >
> > > +static void max9286_set_fsync_period(struct max9286_priv *priv)
> > > +{
> > > +	u32 fsync;
> > > +
> > > +	if (!priv->interval.numerator || !priv->interval.denominator) {
> > > +		/*
> > > +		 * Special case, a null interval enables automatic FRAMESYNC
> > > +		 * mode. FRAMESYNC is taken from the slowest link.
> > > +		 */
> > > +		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > +			      MAX9286_FSYNCMETH_AUTO);
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Manual FRAMESYNC
> > > +	 *
> > > +	 * The FRAMESYNC generator is configured with a period expressed as a
> > > +	 * number of PCLK periods, which runs at 75MHz.
> > > +	 */
> > > +	fsync = div_u64(75000000ULL * priv->interval.numerator,
> > > +			priv->interval.denominator);
> > 
> > Don't we calculate the pixel rate dynamically based on the number of
> > enabled sources ?
> 
> That's a good point, this hardcodes operation for 4 cameras. I'll see
> how I can fix that.
> 
> > > +
> > > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
> > > +		      MAX9286_FSYNCMETH_MANUAL);
> > > +
> > > +	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
> > > +	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
> > > +	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
> > > +}
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * V4L2 Subdev
> > >   */
> > > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > >  	int ret;
> > >
> > >  	if (enable) {
> > > +		max9286_set_fsync_period(priv);
> > > +
> > >  		/*
> > >  		 * The frame sync between cameras is transmitted across the
> > >  		 * reverse channel as GPIO. We must open all channels while
> > > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > >  	return 0;
> > >  }
> > >
> > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd,
> > > +				    struct v4l2_subdev_frame_interval *interval)
> > > +{
> > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > +
> > > +	if (interval->pad == MAX9286_SRC_PAD)
> > 
> > Ah!
> > 
> > I would have expected, as this setting applies to all cameras, and
> > consequentially to the output image stream frame rate, that the
> > operation should have been done on the single source pad... Why is
> > this not the case ?
> 
> I went back and forth between the two, I think it would work fine on the
> source pad as well. I'll give it a try.

Note that once we'll move to the muxed streams API, the frame interval
will be per stream, so setting it on the source pad won't make much of a
difference. What do you think ?

> > > +		return -EINVAL;
> > > +
> > > +	interval->interval = priv->interval;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int max9286_s_frame_interval(struct v4l2_subdev *sd,
> > > +				    struct v4l2_subdev_frame_interval *interval)
> > > +{
> > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > +
> > > +	if (interval->pad == MAX9286_SRC_PAD)
> > > +		return -EINVAL;
> > > +
> > > +	priv->interval = interval->interval;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> > >  				  struct v4l2_subdev_state *sd_state,
> > >  				  struct v4l2_subdev_mbus_code_enum *code)
> > > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> > >
> > >  static const struct v4l2_subdev_video_ops max9286_video_ops = {
> > >  	.s_stream	= max9286_s_stream,
> > > +	.g_frame_interval = max9286_g_frame_interval,
> > > +	.s_frame_interval = max9286_s_frame_interval,
> > >  };
> > >
> > >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
> > >  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> > >  		      MAX9286_DATATYPE_YUV422_8BIT);
> > >
> > > -	/* Automatic: FRAMESYNC taken from the slowest Link. */
> > > -	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > -		      MAX9286_FSYNCMETH_AUTO);
> > > +	max9286_set_fsync_period(priv);
> > >
> > >  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> > >  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
Jacopo Mondi Dec. 20, 2021, 8:43 a.m. UTC | #4 
Hi Laurent,

On Fri, Dec 17, 2021 at 07:49:07PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Dec 17, 2021 at 06:05:02PM +0200, Laurent Pinchart wrote:
> > On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote:
> > > On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote:
> > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >
> > > > The MAX9286 can generate a framesync signal to synchronize the cameras,
> > > > using an internal timer. Support this mode of operation and configure it
> > > > through the .s_frameinterval() operation. If the frame interval is not
> > > > 0, framesync is switched to manual mode with the specified interval,
> > > > otherwise automatic mode is used.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 63 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > > index fa2f6a823fe6..dce1146635d2 100644
> > > > --- a/drivers/media/i2c/max9286.c
> > > > +++ b/drivers/media/i2c/max9286.c
> > > > @@ -174,6 +174,7 @@ struct max9286_priv {
> > > >  	struct v4l2_ctrl *pixelrate;
> > > >
> > > >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > > > +	struct v4l2_fract interval;
> > > >
> > > >  	/* Protects controls and fmt structures */
> > > >  	struct mutex mutex;
> > > > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static void max9286_set_fsync_period(struct max9286_priv *priv)
> > > > +{
> > > > +	u32 fsync;
> > > > +
> > > > +	if (!priv->interval.numerator || !priv->interval.denominator) {
> > > > +		/*
> > > > +		 * Special case, a null interval enables automatic FRAMESYNC
> > > > +		 * mode. FRAMESYNC is taken from the slowest link.
> > > > +		 */
> > > > +		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > > +			      MAX9286_FSYNCMETH_AUTO);
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Manual FRAMESYNC
> > > > +	 *
> > > > +	 * The FRAMESYNC generator is configured with a period expressed as a
> > > > +	 * number of PCLK periods, which runs at 75MHz.
> > > > +	 */
> > > > +	fsync = div_u64(75000000ULL * priv->interval.numerator,
> > > > +			priv->interval.denominator);
> > >
> > > Don't we calculate the pixel rate dynamically based on the number of
> > > enabled sources ?
> >
> > That's a good point, this hardcodes operation for 4 cameras. I'll see
> > how I can fix that.
> >
> > > > +
> > > > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
> > > > +		      MAX9286_FSYNCMETH_MANUAL);
> > > > +
> > > > +	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
> > > > +	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
> > > > +	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
> > > > +}
> > > > +
> > > >  /* -----------------------------------------------------------------------------
> > > >   * V4L2 Subdev
> > > >   */
> > > > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > > >  	int ret;
> > > >
> > > >  	if (enable) {
> > > > +		max9286_set_fsync_period(priv);
> > > > +
> > > >  		/*
> > > >  		 * The frame sync between cameras is transmitted across the
> > > >  		 * reverse channel as GPIO. We must open all channels while
> > > > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd,
> > > > +				    struct v4l2_subdev_frame_interval *interval)
> > > > +{
> > > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > +
> > > > +	if (interval->pad == MAX9286_SRC_PAD)
> > >
> > > Ah!
> > >
> > > I would have expected, as this setting applies to all cameras, and
> > > consequentially to the output image stream frame rate, that the
> > > operation should have been done on the single source pad... Why is
> > > this not the case ?
> >
> > I went back and forth between the two, I think it would work fine on the
> > source pad as well. I'll give it a try.
>
> Note that once we'll move to the muxed streams API, the frame interval
> will be per stream, so setting it on the source pad won't make much of a
> difference. What do you think ?
>

Per-stream means you have it apply on all the enabled source pads, and
potentially configure cameras differently, which is not supported
because frame sync is a configuration parameter which applies to all
sources.

If I were you I would have used the single source pad, as from the
user perspective what is relevant is the max9286 produced frame rate,
but I understand the counter-argument, so up to you :)

Thanks
   j

> > > > +		return -EINVAL;
> > > > +
> > > > +	interval->interval = priv->interval;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9286_s_frame_interval(struct v4l2_subdev *sd,
> > > > +				    struct v4l2_subdev_frame_interval *interval)
> > > > +{
> > > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > +
> > > > +	if (interval->pad == MAX9286_SRC_PAD)
> > > > +		return -EINVAL;
> > > > +
> > > > +	priv->interval = interval->interval;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> > > >  				  struct v4l2_subdev_state *sd_state,
> > > >  				  struct v4l2_subdev_mbus_code_enum *code)
> > > > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> > > >
> > > >  static const struct v4l2_subdev_video_ops max9286_video_ops = {
> > > >  	.s_stream	= max9286_s_stream,
> > > > +	.g_frame_interval = max9286_g_frame_interval,
> > > > +	.s_frame_interval = max9286_s_frame_interval,
> > > >  };
> > > >
> > > >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > > > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
> > > >  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> > > >  		      MAX9286_DATATYPE_YUV422_8BIT);
> > > >
> > > > -	/* Automatic: FRAMESYNC taken from the slowest Link. */
> > > > -	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > > -		      MAX9286_FSYNCMETH_AUTO);
> > > > +	max9286_set_fsync_period(priv);
> > > >
> > > >  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> > > >  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Dec. 20, 2021, 8:45 a.m. UTC | #5 
Hi Jacopo,

On Mon, Dec 20, 2021 at 09:43:57AM +0100, Jacopo Mondi wrote:
> On Fri, Dec 17, 2021 at 07:49:07PM +0200, Laurent Pinchart wrote:
> > On Fri, Dec 17, 2021 at 06:05:02PM +0200, Laurent Pinchart wrote:
> > > On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote:
> > > > On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote:
> > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > >
> > > > > The MAX9286 can generate a framesync signal to synchronize the cameras,
> > > > > using an internal timer. Support this mode of operation and configure it
> > > > > through the .s_frameinterval() operation. If the frame interval is not
> > > > > 0, framesync is switched to manual mode with the specified interval,
> > > > > otherwise automatic mode is used.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > >  drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
> > > > >  1 file changed, 63 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > > > index fa2f6a823fe6..dce1146635d2 100644
> > > > > --- a/drivers/media/i2c/max9286.c
> > > > > +++ b/drivers/media/i2c/max9286.c
> > > > > @@ -174,6 +174,7 @@ struct max9286_priv {
> > > > >  	struct v4l2_ctrl *pixelrate;
> > > > >
> > > > >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > > > > +	struct v4l2_fract interval;
> > > > >
> > > > >  	/* Protects controls and fmt structures */
> > > > >  	struct mutex mutex;
> > > > > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > +static void max9286_set_fsync_period(struct max9286_priv *priv)
> > > > > +{
> > > > > +	u32 fsync;
> > > > > +
> > > > > +	if (!priv->interval.numerator || !priv->interval.denominator) {
> > > > > +		/*
> > > > > +		 * Special case, a null interval enables automatic FRAMESYNC
> > > > > +		 * mode. FRAMESYNC is taken from the slowest link.
> > > > > +		 */
> > > > > +		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > > > +			      MAX9286_FSYNCMETH_AUTO);
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	/*
> > > > > +	 * Manual FRAMESYNC
> > > > > +	 *
> > > > > +	 * The FRAMESYNC generator is configured with a period expressed as a
> > > > > +	 * number of PCLK periods, which runs at 75MHz.
> > > > > +	 */
> > > > > +	fsync = div_u64(75000000ULL * priv->interval.numerator,
> > > > > +			priv->interval.denominator);
> > > >
> > > > Don't we calculate the pixel rate dynamically based on the number of
> > > > enabled sources ?
> > >
> > > That's a good point, this hardcodes operation for 4 cameras. I'll see
> > > how I can fix that.
> > >
> > > > > +
> > > > > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
> > > > > +		      MAX9286_FSYNCMETH_MANUAL);
> > > > > +
> > > > > +	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
> > > > > +	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
> > > > > +	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
> > > > > +}
> > > > > +
> > > > >  /* -----------------------------------------------------------------------------
> > > > >   * V4L2 Subdev
> > > > >   */
> > > > > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > > > >  	int ret;
> > > > >
> > > > >  	if (enable) {
> > > > > +		max9286_set_fsync_period(priv);
> > > > > +
> > > > >  		/*
> > > > >  		 * The frame sync between cameras is transmitted across the
> > > > >  		 * reverse channel as GPIO. We must open all channels while
> > > > > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd,
> > > > > +				    struct v4l2_subdev_frame_interval *interval)
> > > > > +{
> > > > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > > +
> > > > > +	if (interval->pad == MAX9286_SRC_PAD)
> > > >
> > > > Ah!
> > > >
> > > > I would have expected, as this setting applies to all cameras, and
> > > > consequentially to the output image stream frame rate, that the
> > > > operation should have been done on the single source pad... Why is
> > > > this not the case ?
> > >
> > > I went back and forth between the two, I think it would work fine on the
> > > source pad as well. I'll give it a try.
> >
> > Note that once we'll move to the muxed streams API, the frame interval
> > will be per stream, so setting it on the source pad won't make much of a
> > difference. What do you think ?
> >
> 
> Per-stream means you have it apply on all the enabled source pads, and
> potentially configure cameras differently, which is not supported
> because frame sync is a configuration parameter which applies to all
> sources.
> 
> If I were you I would have used the single source pad, as from the
> user perspective what is relevant is the max9286 produced frame rate,
> but I understand the counter-argument, so up to you :)

The single source pad will have four streams, so it will still be
per-stream, even on a single pad.

> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	interval->interval = priv->interval;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int max9286_s_frame_interval(struct v4l2_subdev *sd,
> > > > > +				    struct v4l2_subdev_frame_interval *interval)
> > > > > +{
> > > > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > > +
> > > > > +	if (interval->pad == MAX9286_SRC_PAD)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	priv->interval = interval->interval;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> > > > >  				  struct v4l2_subdev_state *sd_state,
> > > > >  				  struct v4l2_subdev_mbus_code_enum *code)
> > > > > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> > > > >
> > > > >  static const struct v4l2_subdev_video_ops max9286_video_ops = {
> > > > >  	.s_stream	= max9286_s_stream,
> > > > > +	.g_frame_interval = max9286_g_frame_interval,
> > > > > +	.s_frame_interval = max9286_s_frame_interval,
> > > > >  };
> > > > >
> > > > >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > > > > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
> > > > >  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> > > > >  		      MAX9286_DATATYPE_YUV422_8BIT);
> > > > >
> > > > > -	/* Automatic: FRAMESYNC taken from the slowest Link. */
> > > > > -	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > > > -		      MAX9286_FSYNCMETH_AUTO);
> > > > > +	max9286_set_fsync_period(priv);
> > > > >
> > > > >  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> > > > >  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
diff mbox series

Patch

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index fa2f6a823fe6..dce1146635d2 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -174,6 +174,7 @@  struct max9286_priv {
 	struct v4l2_ctrl *pixelrate;
 
 	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
+	struct v4l2_fract interval;
 
 	/* Protects controls and fmt structures */
 	struct mutex mutex;
@@ -474,6 +475,37 @@  static int max9286_check_config_link(struct max9286_priv *priv,
 	return 0;
 }
 
+static void max9286_set_fsync_period(struct max9286_priv *priv)
+{
+	u32 fsync;
+
+	if (!priv->interval.numerator || !priv->interval.denominator) {
+		/*
+		 * Special case, a null interval enables automatic FRAMESYNC
+		 * mode. FRAMESYNC is taken from the slowest link.
+		 */
+		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
+			      MAX9286_FSYNCMETH_AUTO);
+		return;
+	}
+
+	/*
+	 * Manual FRAMESYNC
+	 *
+	 * The FRAMESYNC generator is configured with a period expressed as a
+	 * number of PCLK periods, which runs at 75MHz.
+	 */
+	fsync = div_u64(75000000ULL * priv->interval.numerator,
+			priv->interval.denominator);
+
+	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
+		      MAX9286_FSYNCMETH_MANUAL);
+
+	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
+	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
+	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
+}
+
 /* -----------------------------------------------------------------------------
  * V4L2 Subdev
  */
@@ -656,6 +688,8 @@  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	int ret;
 
 	if (enable) {
+		max9286_set_fsync_period(priv);
+
 		/*
 		 * The frame sync between cameras is transmitted across the
 		 * reverse channel as GPIO. We must open all channels while
@@ -715,6 +749,32 @@  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	return 0;
 }
 
+static int max9286_g_frame_interval(struct v4l2_subdev *sd,
+				    struct v4l2_subdev_frame_interval *interval)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+
+	if (interval->pad == MAX9286_SRC_PAD)
+		return -EINVAL;
+
+	interval->interval = priv->interval;
+
+	return 0;
+}
+
+static int max9286_s_frame_interval(struct v4l2_subdev *sd,
+				    struct v4l2_subdev_frame_interval *interval)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+
+	if (interval->pad == MAX9286_SRC_PAD)
+		return -EINVAL;
+
+	priv->interval = interval->interval;
+
+	return 0;
+}
+
 static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_mbus_code_enum *code)
@@ -806,6 +866,8 @@  static int max9286_get_fmt(struct v4l2_subdev *sd,
 
 static const struct v4l2_subdev_video_ops max9286_video_ops = {
 	.s_stream	= max9286_s_stream,
+	.g_frame_interval = max9286_g_frame_interval,
+	.s_frame_interval = max9286_s_frame_interval,
 };
 
 static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
@@ -998,9 +1060,7 @@  static int max9286_setup(struct max9286_priv *priv)
 		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
 		      MAX9286_DATATYPE_YUV422_8BIT);
 
-	/* Automatic: FRAMESYNC taken from the slowest Link. */
-	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
-		      MAX9286_FSYNCMETH_AUTO);
+	max9286_set_fsync_period(priv);
 
 	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
 	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |