Re: [PATCH 04/11] media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dave,

Thank you for the patch.

On Tue, Jan 31, 2023 at 07:20:09PM +0000, Dave Stevenson wrote:
> The datasheet lists the link frequency changes between
> 1080p and 720p modes. This is correct that the link frequency
> changes as measured on an oscilloscope.
> 
> Link frequency is not necessarily the same as pixel rate.

That's right. The model where

	htotal * vtotal * frame rate = pixel rate

applies to the pixel array. Things can be retimed when transmitted over
CSI-2 (or over anything, really), with pixels transmitted at a higher or
lower rate, with smaller or larger blanking, as long as it matches the
total horizontal line length in time units. I suppose this could in
theory be done vertically too, but that would require too much memory.

I hope we'll never have a need, on the receiver side, to know the active
line length in time units as sent over CSI-2 :-)

> The datasheet gives standard configurations for 1080p and 720p
> modes at a number of frame rates.
> Looking at the 1080p mode it gives:
> HMAX = 0x898 = 2200
> VMAX = 0x465 = 1125
> 2200 * 1125 * 60fps = 148.5MPix/s
> 
> Looking at the 720p mode it gives:
> HMAX = 0xce4 = 3300
> VMAX = 0x2ee = 750
> 3300 * 750 * 60fps = 148.5Mpix/s
> 
> This driver currently scales the pixel rate proportionally to the
> link frequency, however the above shows that this is not the
> correct thing to do, and currently all frame rate and exposure
> calculations give incorrect results.
> 
> Correctly report the pixel rate as being 148.5MPix/s under any
> mode.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx290.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 12946ca9d8d2..bd8729aed43c 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -107,6 +107,7 @@
>  
>  #define IMX290_VMAX_DEFAULT				1125
>  
> +#define IMX290_PIXEL_RATE				148500000

A blank line is missing here. With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  /*
>   * The IMX290 pixel array is organized as follows:
>   *
> @@ -190,7 +191,6 @@ struct imx290 {
>  
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *link_freq;
> -	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *vblank;
>  };
> @@ -649,15 +649,8 @@ static void imx290_ctrl_update(struct imx290 *imx290,
>  {
>  	unsigned int hblank = mode->hmax - mode->width;
>  	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> -	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
> -	u64 pixel_rate;
> -
> -	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> -	pixel_rate = link_freq * 2 * imx290->nlanes;
> -	do_div(pixel_rate, imx290_format_info(format->code)->bpp);
>  
>  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> -	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, pixel_rate);
>  
>  	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
>  	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
> @@ -707,9 +700,9 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  	if (imx290->link_freq)
>  		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
> -	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> -					       V4L2_CID_PIXEL_RATE,
> -					       1, INT_MAX, 1, 1);
> +	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, V4L2_CID_PIXEL_RATE,
> +			  IMX290_PIXEL_RATE, IMX290_PIXEL_RATE, 1,
> +			  IMX290_PIXEL_RATE);
>  
>  	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
>  				     V4L2_CID_TEST_PATTERN,

-- 
Regards,

Laurent Pinchart