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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
> /*
> * 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
