[08/13] media: i2c: imx214: Add vblank and hblank controls

Message ID	20240902-imx214-v1-8-c96cba989315@apitzsch.eu (mailing list archive)
State	New
Headers	show Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DB8C418734B; Mon, 2 Sep 2024 21:53:51 +0000 (UTC) From: =?utf-8?q?Andr=C3=A9_Apitzsch_via_B4_Relay?= <devnull+git.apitzsch.eu@kernel.org> Date: Mon, 02 Sep 2024 23:54:35 +0200 Subject: [PATCH 08/13] media: i2c: imx214: Add vblank and hblank controls Precedence: bulk MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <20240902-imx214-v1-8-c96cba989315@apitzsch.eu> References: <20240902-imx214-v1-0-c96cba989315@apitzsch.eu> In-Reply-To: <20240902-imx214-v1-0-c96cba989315@apitzsch.eu> To: Ricardo Ribalda <ribalda@kernel.org>, Sakari Ailus <sakari.ailus@linux.intel.com>, Mauro Carvalho Chehab <mchehab@kernel.org> Cc: ~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, =?utf-8?q?Andr?= =?utf-8?q?=C3=A9_Apitzsch?= <git@apitzsch.eu> Reply-To: git@apitzsch.eu
Series	media: i2c: imx214: Miscellaneous cleanups and improvements \| expand [00/13] media: i2c: imx214: Miscellaneous cleanups and improvements [01/13] media: i2c: imx214: Use subdev active state [02/13] media: i2c: imx214: Remove unneeded goto [03/13] media: i2c: imx214: Simplify with dev_err_probe() [04/13] media: i2c: imx214: Convert to CCI register access helpers [05/13] media: i2c: imx214: Replace register addresses with macros [06/13] media: i2c: imx214: Drop IMX214_REG_EXPOSURE from mode reg arrays [07/13] media: i2c: imx214: Use number of lanes from device tree [08/13] media: i2c: imx214: Add vblank and hblank controls [09/13] media: i2c: imx214: Extract format and crop settings [10/13] media: i2c: imx214: Implement vflip/hflip controls [11/13] media: i2c: imx214: Add analogue/digital gain control [12/13] media: i2c: imx214: Verify chip ID [13/13] media: i2c: imx214: Add test pattern control

André Apitzsch via B4 Relay Sept. 2, 2024, 9:54 p.m. UTC

From: André Apitzsch <git@apitzsch.eu>

Add vblank control to allow changing the framerate /
higher exposure values.

The vblank and hblank controls are needed for libcamera support.

While at it, fix the minimal exposure time according to the datasheet.

Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
 drivers/media/i2c/imx214.c | 112 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 91 insertions(+), 21 deletions(-)

Ricardo Ribalda Delgado Sept. 12, 2024, 1:40 p.m. UTC | #1

Hi

Arent you missing some chage in enum_frame_interval?

On Mon, Sep 2, 2024 at 11:53 PM André Apitzsch via B4 Relay
<devnull+git.apitzsch.eu@kernel.org> wrote:
>
> From: André Apitzsch <git@apitzsch.eu>
>
> Add vblank control to allow changing the framerate /
> higher exposure values.
>
> The vblank and hblank controls are needed for libcamera support.
>
> While at it, fix the minimal exposure time according to the datasheet.
>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
>  drivers/media/i2c/imx214.c | 112 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 91 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 3b422cddbdce..9f5a57aebb86 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -34,11 +34,18 @@
>
>  /* V-TIMING internal */
>  #define IMX214_REG_FRM_LENGTH_LINES    CCI_REG16(0x0340)
> +#define IMX214_VTS_MAX                 0xffff
> +
> +#define IMX214_VBLANK_MIN              4
> +
> +/* HBLANK control - read only */
> +#define IMX214_PPL_DEFAULT             5008
>
>  /* Exposure control */
>  #define IMX214_REG_EXPOSURE            CCI_REG16(0x0202)
> -#define IMX214_EXPOSURE_MIN            0
> -#define IMX214_EXPOSURE_MAX            3184
> +#define IMX214_EXPOSURE_OFFSET         10
> +#define IMX214_EXPOSURE_MIN            1
> +#define IMX214_EXPOSURE_MAX            (IMX214_VTS_MAX - IMX214_EXPOSURE_OFFSET)
>  #define IMX214_EXPOSURE_STEP           1
>  #define IMX214_EXPOSURE_DEFAULT                3184
>  #define IMX214_REG_EXPOSURE_RATIO      CCI_REG8(0x0222)
> @@ -189,6 +196,8 @@ struct imx214 {
>         struct v4l2_ctrl_handler ctrls;
>         struct v4l2_ctrl *pixel_rate;
>         struct v4l2_ctrl *link_freq;
> +       struct v4l2_ctrl *vblank;
> +       struct v4l2_ctrl *hblank;
>         struct v4l2_ctrl *exposure;
>         struct v4l2_ctrl *unit_size;
>
> @@ -205,8 +214,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
>         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
>         { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
>         { IMX214_REG_EXPOSURE_RATIO, 1 },
> -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
>         { IMX214_REG_X_ADD_STA, 56 },
>         { IMX214_REG_Y_ADD_STA, 408 },
>         { IMX214_REG_X_ADD_END, 4151 },
> @@ -277,8 +284,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = {
>         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
>         { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
>         { IMX214_REG_EXPOSURE_RATIO, 1 },
> -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
>         { IMX214_REG_X_ADD_STA, 1144 },
>         { IMX214_REG_Y_ADD_STA, 1020 },
>         { IMX214_REG_X_ADD_END, 3063 },
> @@ -362,6 +367,7 @@ static const struct cci_reg_sequence mode_table_common[] = {
>         { IMX214_REG_ORIENTATION, 0 },
>         { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
>         { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> +       { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
>         { CCI_REG8(0x4550), 0x02 },
>         { CCI_REG8(0x4601), 0x00 },
>         { CCI_REG8(0x4642), 0x05 },
> @@ -465,18 +471,24 @@ static const struct cci_reg_sequence mode_table_common[] = {
>  static const struct imx214_mode {
>         u32 width;
>         u32 height;
> +
> +       /* V-timing */
> +       unsigned int vts_def;
> +
>         unsigned int num_of_regs;
>         const struct cci_reg_sequence *reg_table;
>  } imx214_modes[] = {
>         {
>                 .width = 4096,
>                 .height = 2304,
> +               .vts_def = 3194,
>                 .num_of_regs = ARRAY_SIZE(mode_4096x2304),
>                 .reg_table = mode_4096x2304,
>         },
>         {
>                 .width = 1920,
>                 .height = 1080,
> +               .vts_def = 3194,
>                 .num_of_regs = ARRAY_SIZE(mode_1920x1080),
>                 .reg_table = mode_1920x1080,
>         },
> @@ -629,6 +641,37 @@ static int imx214_set_format(struct v4l2_subdev *sd,
>         __crop->width = mode->width;
>         __crop->height = mode->height;
>
> +       if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> +               int exposure_max;
> +               int exposure_def;
> +               int hblank;
> +
> +               /* Update limits and set FPS to default */
> +               __v4l2_ctrl_modify_range(imx214->vblank, IMX214_VBLANK_MIN,
> +                                        IMX214_VTS_MAX - mode->height, 1,
> +                                        mode->vts_def - mode->height);
> +               __v4l2_ctrl_s_ctrl(imx214->vblank,
> +                                  mode->vts_def - mode->height);
> +
> +               /* Update max exposure while meeting expected vblanking */
> +               exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> +               exposure_def = (exposure_max < IMX214_EXPOSURE_DEFAULT) ?
> +                       exposure_max : IMX214_EXPOSURE_DEFAULT;
> +               __v4l2_ctrl_modify_range(imx214->exposure,
> +                                        imx214->exposure->minimum,
> +                                        exposure_max, imx214->exposure->step,
> +                                        exposure_def);
> +
> +               /*
> +                * Currently PPL is fixed to IMX214_PPL_DEFAULT, so hblank
> +                * depends on mode->width only, and is not changeble in any
> +                * way other than changing the mode.
> +                */
> +               hblank = IMX214_PPL_DEFAULT - mode->width;
> +               __v4l2_ctrl_modify_range(imx214->hblank, hblank, hblank, 1,
> +                                        hblank);
> +       }
> +
>         return 0;
>  }
>
> @@ -678,8 +721,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
>  {
>         struct imx214 *imx214 = container_of(ctrl->handler,
>                                              struct imx214, ctrls);
> +       const struct v4l2_mbus_framefmt *format;
> +       struct v4l2_subdev_state *state;
>         int ret;
>
> +       state = v4l2_subdev_get_locked_active_state(&imx214->sd);
> +       format = v4l2_subdev_state_get_format(state, 0);
> +
> +       if (ctrl->id == V4L2_CID_VBLANK) {
> +               int exposure_max, exposure_def;
> +
> +               /* Update max exposure while meeting expected vblanking */
> +               exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET;
> +               exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> +               __v4l2_ctrl_modify_range(imx214->exposure,
> +                                        imx214->exposure->minimum,
> +                                        exposure_max, imx214->exposure->step,
> +                                        exposure_def);
> +       }
> +
>         /*
>          * Applying V4L2 control value only happens
>          * when power is up for streaming
> @@ -691,7 +751,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
>         case V4L2_CID_EXPOSURE:
>                 cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret);
>                 break;
> -
> +       case V4L2_CID_VBLANK:
> +               cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES,
> +                         format->height + ctrl->val, &ret);
> +               break;
>         default:
>                 ret = -EINVAL;
>         }
> @@ -714,8 +777,11 @@ static int imx214_ctrls_init(struct imx214 *imx214)
>                 .width = 1120,
>                 .height = 1120,
>         };
> +       const struct imx214_mode *mode = &imx214_modes[0];
>         struct v4l2_fwnode_device_properties props;
>         struct v4l2_ctrl_handler *ctrl_hdlr;
> +       int exposure_max, exposure_def;
> +       int hblank;
>         int ret;
>
>         ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> @@ -723,7 +789,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
>                 return ret;
>
>         ctrl_hdlr = &imx214->ctrls;
> -       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> +       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
>         if (ret)
>                 return ret;
>
> @@ -739,22 +805,26 @@ static int imx214_ctrls_init(struct imx214 *imx214)
>         if (imx214->link_freq)
>                 imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> -       /*
> -        * WARNING!
> -        * Values obtained reverse engineering blobs and/or devices.
> -        * Ranges and functionality might be wrong.
> -        *
> -        * Sony, please release some register set documentation for the
> -        * device.
> -        *
> -        * Yours sincerely, Ricardo.
> -        */

I would like to keep this comment until there is a public document available.



> +       /* Initial vblank/hblank/exposure parameters based on current mode */
> +       imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> +                                          V4L2_CID_VBLANK, IMX214_VBLANK_MIN,
> +                                          IMX214_VTS_MAX - mode->height, 1,
> +                                          mode->vts_def - mode->height);
> +
> +       hblank = IMX214_PPL_DEFAULT - mode->width;
> +       imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> +                                          V4L2_CID_HBLANK, hblank, hblank,
> +                                          1, hblank);
> +       if (imx214->hblank)
> +               imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +       exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> +       exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
>         imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
>                                              V4L2_CID_EXPOSURE,
> -                                            IMX214_EXPOSURE_MIN,
> -                                            IMX214_EXPOSURE_MAX,
> +                                            IMX214_EXPOSURE_MIN, exposure_max,
>                                              IMX214_EXPOSURE_STEP,
> -                                            IMX214_EXPOSURE_DEFAULT);
> +                                            exposure_def);
>
>         imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
>                                 NULL,
>
> --
> 2.46.0
>
>

Dave Stevenson Sept. 12, 2024, 2:51 p.m. UTC | #2

Hi André & Ricardo

On Thu, 12 Sept 2024 at 14:41, Ricardo Ribalda Delgado
<ribalda@kernel.org> wrote:
>
> Hi
>
> Arent you missing some chage in enum_frame_interval?

Raw sensors shouldn't be using [enum|set|get]_frame_interval at all
https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#frame-interval-configuration

The question now is how to handle the backwards compatibility for any
userspace app that might be using this driver and expecting to use the
frame_interval calls.
Seeing as it only ever allowed a fixed value of 30fps, leaving it as
is with a comment as to why it is there would be reasonable in my
view.

> On Mon, Sep 2, 2024 at 11:53 PM André Apitzsch via B4 Relay
> <devnull+git.apitzsch.eu@kernel.org> wrote:
> >
> > From: André Apitzsch <git@apitzsch.eu>
> >
> > Add vblank control to allow changing the framerate /
> > higher exposure values.
> >
> > The vblank and hblank controls are needed for libcamera support.
> >
> > While at it, fix the minimal exposure time according to the datasheet.
> >
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > ---
> >  drivers/media/i2c/imx214.c | 112 ++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 91 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 3b422cddbdce..9f5a57aebb86 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -34,11 +34,18 @@
> >
> >  /* V-TIMING internal */
> >  #define IMX214_REG_FRM_LENGTH_LINES    CCI_REG16(0x0340)
> > +#define IMX214_VTS_MAX                 0xffff
> > +
> > +#define IMX214_VBLANK_MIN              4
> > +
> > +/* HBLANK control - read only */
> > +#define IMX214_PPL_DEFAULT             5008
> >
> >  /* Exposure control */
> >  #define IMX214_REG_EXPOSURE            CCI_REG16(0x0202)
> > -#define IMX214_EXPOSURE_MIN            0
> > -#define IMX214_EXPOSURE_MAX            3184
> > +#define IMX214_EXPOSURE_OFFSET         10
> > +#define IMX214_EXPOSURE_MIN            1
> > +#define IMX214_EXPOSURE_MAX            (IMX214_VTS_MAX - IMX214_EXPOSURE_OFFSET)
> >  #define IMX214_EXPOSURE_STEP           1
> >  #define IMX214_EXPOSURE_DEFAULT                3184
> >  #define IMX214_REG_EXPOSURE_RATIO      CCI_REG8(0x0222)
> > @@ -189,6 +196,8 @@ struct imx214 {
> >         struct v4l2_ctrl_handler ctrls;
> >         struct v4l2_ctrl *pixel_rate;
> >         struct v4l2_ctrl *link_freq;
> > +       struct v4l2_ctrl *vblank;
> > +       struct v4l2_ctrl *hblank;
> >         struct v4l2_ctrl *exposure;
> >         struct v4l2_ctrl *unit_size;
> >
> > @@ -205,8 +214,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
> >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> >         { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> >         { IMX214_REG_X_ADD_STA, 56 },
> >         { IMX214_REG_Y_ADD_STA, 408 },
> >         { IMX214_REG_X_ADD_END, 4151 },
> > @@ -277,8 +284,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = {
> >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> >         { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> >         { IMX214_REG_X_ADD_STA, 1144 },
> >         { IMX214_REG_Y_ADD_STA, 1020 },
> >         { IMX214_REG_X_ADD_END, 3063 },
> > @@ -362,6 +367,7 @@ static const struct cci_reg_sequence mode_table_common[] = {
> >         { IMX214_REG_ORIENTATION, 0 },
> >         { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
> >         { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> > +       { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> >         { CCI_REG8(0x4550), 0x02 },
> >         { CCI_REG8(0x4601), 0x00 },
> >         { CCI_REG8(0x4642), 0x05 },
> > @@ -465,18 +471,24 @@ static const struct cci_reg_sequence mode_table_common[] = {
> >  static const struct imx214_mode {
> >         u32 width;
> >         u32 height;
> > +
> > +       /* V-timing */
> > +       unsigned int vts_def;
> > +
> >         unsigned int num_of_regs;
> >         const struct cci_reg_sequence *reg_table;
> >  } imx214_modes[] = {
> >         {
> >                 .width = 4096,
> >                 .height = 2304,
> > +               .vts_def = 3194,
> >                 .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> >                 .reg_table = mode_4096x2304,
> >         },
> >         {
> >                 .width = 1920,
> >                 .height = 1080,
> > +               .vts_def = 3194,
> >                 .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> >                 .reg_table = mode_1920x1080,
> >         },
> > @@ -629,6 +641,37 @@ static int imx214_set_format(struct v4l2_subdev *sd,
> >         __crop->width = mode->width;
> >         __crop->height = mode->height;
> >
> > +       if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > +               int exposure_max;
> > +               int exposure_def;
> > +               int hblank;
> > +
> > +               /* Update limits and set FPS to default */
> > +               __v4l2_ctrl_modify_range(imx214->vblank, IMX214_VBLANK_MIN,
> > +                                        IMX214_VTS_MAX - mode->height, 1,
> > +                                        mode->vts_def - mode->height);
> > +               __v4l2_ctrl_s_ctrl(imx214->vblank,
> > +                                  mode->vts_def - mode->height);
> > +
> > +               /* Update max exposure while meeting expected vblanking */
> > +               exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > +               exposure_def = (exposure_max < IMX214_EXPOSURE_DEFAULT) ?
> > +                       exposure_max : IMX214_EXPOSURE_DEFAULT;
> > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > +                                        imx214->exposure->minimum,
> > +                                        exposure_max, imx214->exposure->step,
> > +                                        exposure_def);
> > +
> > +               /*
> > +                * Currently PPL is fixed to IMX214_PPL_DEFAULT, so hblank
> > +                * depends on mode->width only, and is not changeble in any
> > +                * way other than changing the mode.
> > +                */
> > +               hblank = IMX214_PPL_DEFAULT - mode->width;
> > +               __v4l2_ctrl_modify_range(imx214->hblank, hblank, hblank, 1,
> > +                                        hblank);
> > +       }
> > +
> >         return 0;
> >  }
> >
> > @@ -678,8 +721,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> >         struct imx214 *imx214 = container_of(ctrl->handler,
> >                                              struct imx214, ctrls);
> > +       const struct v4l2_mbus_framefmt *format;
> > +       struct v4l2_subdev_state *state;
> >         int ret;
> >
> > +       state = v4l2_subdev_get_locked_active_state(&imx214->sd);
> > +       format = v4l2_subdev_state_get_format(state, 0);
> > +
> > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > +               int exposure_max, exposure_def;
> > +
> > +               /* Update max exposure while meeting expected vblanking */
> > +               exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET;
> > +               exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > +                                        imx214->exposure->minimum,
> > +                                        exposure_max, imx214->exposure->step,
> > +                                        exposure_def);
> > +       }
> > +
> >         /*
> >          * Applying V4L2 control value only happens
> >          * when power is up for streaming
> > @@ -691,7 +751,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
> >         case V4L2_CID_EXPOSURE:
> >                 cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret);
> >                 break;
> > -
> > +       case V4L2_CID_VBLANK:
> > +               cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES,
> > +                         format->height + ctrl->val, &ret);

My datasheet says this register is "set up in multiples of 2".
(LINE_LENGTH_PCK for HBLANK is "set in multiples of 8")

I don't have one of these modules, but is that saying only multiples
of 2 are permitted (in which case the step size for the control needs
to reflect that), or that setting a value of N is interpreted by the
hardware as 2N?
Do all the numbers with PIXEL_RATE work out correctly in the frame rate calcs?

Reading the spec sheet that 30fps is the max frame rate at full res
(4096x2304), and the driver was setting a value of 3194 to this
register, I don't see it being interpreted as 2N. Then again having
VBLANK at 890 seems pretty high.

> > +               break;
> >         default:
> >                 ret = -EINVAL;
> >         }
> > @@ -714,8 +777,11 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> >                 .width = 1120,
> >                 .height = 1120,
> >         };
> > +       const struct imx214_mode *mode = &imx214_modes[0];
> >         struct v4l2_fwnode_device_properties props;
> >         struct v4l2_ctrl_handler *ctrl_hdlr;
> > +       int exposure_max, exposure_def;
> > +       int hblank;
> >         int ret;
> >
> >         ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> > @@ -723,7 +789,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> >                 return ret;
> >
> >         ctrl_hdlr = &imx214->ctrls;
> > -       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> > +       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> >         if (ret)
> >                 return ret;
> >
> > @@ -739,22 +805,26 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> >         if (imx214->link_freq)
> >                 imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > -       /*
> > -        * WARNING!
> > -        * Values obtained reverse engineering blobs and/or devices.
> > -        * Ranges and functionality might be wrong.
> > -        *
> > -        * Sony, please release some register set documentation for the
> > -        * device.
> > -        *
> > -        * Yours sincerely, Ricardo.
> > -        */
>
> I would like to keep this comment until there is a public document available.

I suspect you'll be waiting forever for a document to be officially released.

I have a datasheet for IMX214, and I believe Kieran and Jacopo do too.
Which specific values do you wish to be verified?

>
> > +       /* Initial vblank/hblank/exposure parameters based on current mode */
> > +       imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> > +                                          V4L2_CID_VBLANK, IMX214_VBLANK_MIN,

IMX214_VBLANK_MIN being 4 feels plausible, but pretty low.
I read the datasheet to say there are 4 embedded data lines per image.
Seeing as you have STATS data output enabled as well that makes 5
lines of non-image data per frame, but you're only adding blanking
time for 4 lines.

As noted above, I think you've also increased the max frame rate
significantly above that quoted by Sony. Has that been actually
exercised and confirmed to function correctly?

  Dave

> > +                                          IMX214_VTS_MAX - mode->height, 1,
> > +                                          mode->vts_def - mode->height);
> > +
> > +       hblank = IMX214_PPL_DEFAULT - mode->width;
> > +       imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> > +                                          V4L2_CID_HBLANK, hblank, hblank,
> > +                                          1, hblank);
> > +       if (imx214->hblank)
> > +               imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +       exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > +       exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> >         imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> >                                              V4L2_CID_EXPOSURE,
> > -                                            IMX214_EXPOSURE_MIN,
> > -                                            IMX214_EXPOSURE_MAX,
> > +                                            IMX214_EXPOSURE_MIN, exposure_max,
> >                                              IMX214_EXPOSURE_STEP,
> > -                                            IMX214_EXPOSURE_DEFAULT);
> > +                                            exposure_def);
> >
> >         imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
> >                                 NULL,
> >
> > --
> > 2.46.0
> >
> >
>

Dave Stevenson Sept. 13, 2024, 5:40 p.m. UTC | #3

On Thu, 12 Sept 2024 at 15:51, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi André & Ricardo
>
> On Thu, 12 Sept 2024 at 14:41, Ricardo Ribalda Delgado
> <ribalda@kernel.org> wrote:
> >
> > Hi
> >
> > Arent you missing some chage in enum_frame_interval?
>
> Raw sensors shouldn't be using [enum|set|get]_frame_interval at all
> https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#frame-interval-configuration
>
> The question now is how to handle the backwards compatibility for any
> userspace app that might be using this driver and expecting to use the
> frame_interval calls.
> Seeing as it only ever allowed a fixed value of 30fps, leaving it as
> is with a comment as to why it is there would be reasonable in my
> view.
>
> > On Mon, Sep 2, 2024 at 11:53 PM André Apitzsch via B4 Relay
> > <devnull+git.apitzsch.eu@kernel.org> wrote:
> > >
> > > From: André Apitzsch <git@apitzsch.eu>
> > >
> > > Add vblank control to allow changing the framerate /
> > > higher exposure values.
> > >
> > > The vblank and hblank controls are needed for libcamera support.
> > >
> > > While at it, fix the minimal exposure time according to the datasheet.
> > >
> > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > ---
> > >  drivers/media/i2c/imx214.c | 112 ++++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 91 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > index 3b422cddbdce..9f5a57aebb86 100644
> > > --- a/drivers/media/i2c/imx214.c
> > > +++ b/drivers/media/i2c/imx214.c
> > > @@ -34,11 +34,18 @@
> > >
> > >  /* V-TIMING internal */
> > >  #define IMX214_REG_FRM_LENGTH_LINES    CCI_REG16(0x0340)
> > > +#define IMX214_VTS_MAX                 0xffff
> > > +
> > > +#define IMX214_VBLANK_MIN              4
> > > +
> > > +/* HBLANK control - read only */
> > > +#define IMX214_PPL_DEFAULT             5008
> > >
> > >  /* Exposure control */
> > >  #define IMX214_REG_EXPOSURE            CCI_REG16(0x0202)
> > > -#define IMX214_EXPOSURE_MIN            0
> > > -#define IMX214_EXPOSURE_MAX            3184
> > > +#define IMX214_EXPOSURE_OFFSET         10
> > > +#define IMX214_EXPOSURE_MIN            1
> > > +#define IMX214_EXPOSURE_MAX            (IMX214_VTS_MAX - IMX214_EXPOSURE_OFFSET)
> > >  #define IMX214_EXPOSURE_STEP           1
> > >  #define IMX214_EXPOSURE_DEFAULT                3184
> > >  #define IMX214_REG_EXPOSURE_RATIO      CCI_REG8(0x0222)
> > > @@ -189,6 +196,8 @@ struct imx214 {
> > >         struct v4l2_ctrl_handler ctrls;
> > >         struct v4l2_ctrl *pixel_rate;
> > >         struct v4l2_ctrl *link_freq;
> > > +       struct v4l2_ctrl *vblank;
> > > +       struct v4l2_ctrl *hblank;
> > >         struct v4l2_ctrl *exposure;
> > >         struct v4l2_ctrl *unit_size;
> > >
> > > @@ -205,8 +214,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
> > >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > >         { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> > >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > >         { IMX214_REG_X_ADD_STA, 56 },
> > >         { IMX214_REG_Y_ADD_STA, 408 },
> > >         { IMX214_REG_X_ADD_END, 4151 },
> > > @@ -277,8 +284,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = {
> > >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > >         { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH },
> > >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > >         { IMX214_REG_X_ADD_STA, 1144 },
> > >         { IMX214_REG_Y_ADD_STA, 1020 },
> > >         { IMX214_REG_X_ADD_END, 3063 },
> > > @@ -362,6 +367,7 @@ static const struct cci_reg_sequence mode_table_common[] = {
> > >         { IMX214_REG_ORIENTATION, 0 },
> > >         { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
> > >         { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> > > +       { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> > >         { CCI_REG8(0x4550), 0x02 },
> > >         { CCI_REG8(0x4601), 0x00 },
> > >         { CCI_REG8(0x4642), 0x05 },
> > > @@ -465,18 +471,24 @@ static const struct cci_reg_sequence mode_table_common[] = {
> > >  static const struct imx214_mode {
> > >         u32 width;
> > >         u32 height;
> > > +
> > > +       /* V-timing */
> > > +       unsigned int vts_def;
> > > +
> > >         unsigned int num_of_regs;
> > >         const struct cci_reg_sequence *reg_table;
> > >  } imx214_modes[] = {
> > >         {
> > >                 .width = 4096,
> > >                 .height = 2304,
> > > +               .vts_def = 3194,
> > >                 .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> > >                 .reg_table = mode_4096x2304,
> > >         },
> > >         {
> > >                 .width = 1920,
> > >                 .height = 1080,
> > > +               .vts_def = 3194,
> > >                 .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> > >                 .reg_table = mode_1920x1080,
> > >         },
> > > @@ -629,6 +641,37 @@ static int imx214_set_format(struct v4l2_subdev *sd,
> > >         __crop->width = mode->width;
> > >         __crop->height = mode->height;
> > >
> > > +       if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > +               int exposure_max;
> > > +               int exposure_def;
> > > +               int hblank;
> > > +
> > > +               /* Update limits and set FPS to default */
> > > +               __v4l2_ctrl_modify_range(imx214->vblank, IMX214_VBLANK_MIN,
> > > +                                        IMX214_VTS_MAX - mode->height, 1,
> > > +                                        mode->vts_def - mode->height);
> > > +               __v4l2_ctrl_s_ctrl(imx214->vblank,
> > > +                                  mode->vts_def - mode->height);
> > > +
> > > +               /* Update max exposure while meeting expected vblanking */
> > > +               exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > > +               exposure_def = (exposure_max < IMX214_EXPOSURE_DEFAULT) ?
> > > +                       exposure_max : IMX214_EXPOSURE_DEFAULT;
> > > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > > +                                        imx214->exposure->minimum,
> > > +                                        exposure_max, imx214->exposure->step,
> > > +                                        exposure_def);
> > > +
> > > +               /*
> > > +                * Currently PPL is fixed to IMX214_PPL_DEFAULT, so hblank
> > > +                * depends on mode->width only, and is not changeble in any
> > > +                * way other than changing the mode.
> > > +                */
> > > +               hblank = IMX214_PPL_DEFAULT - mode->width;
> > > +               __v4l2_ctrl_modify_range(imx214->hblank, hblank, hblank, 1,
> > > +                                        hblank);
> > > +       }
> > > +
> > >         return 0;
> > >  }
> > >
> > > @@ -678,8 +721,25 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  {
> > >         struct imx214 *imx214 = container_of(ctrl->handler,
> > >                                              struct imx214, ctrls);
> > > +       const struct v4l2_mbus_framefmt *format;
> > > +       struct v4l2_subdev_state *state;
> > >         int ret;
> > >
> > > +       state = v4l2_subdev_get_locked_active_state(&imx214->sd);
> > > +       format = v4l2_subdev_state_get_format(state, 0);
> > > +
> > > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > > +               int exposure_max, exposure_def;
> > > +
> > > +               /* Update max exposure while meeting expected vblanking */
> > > +               exposure_max = format->height + ctrl->val - IMX214_EXPOSURE_OFFSET;
> > > +               exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> > > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > > +                                        imx214->exposure->minimum,
> > > +                                        exposure_max, imx214->exposure->step,
> > > +                                        exposure_def);
> > > +       }
> > > +
> > >         /*
> > >          * Applying V4L2 control value only happens
> > >          * when power is up for streaming
> > > @@ -691,7 +751,10 @@ static int imx214_set_ctrl(struct v4l2_ctrl *ctrl)
> > >         case V4L2_CID_EXPOSURE:
> > >                 cci_write(imx214->regmap, IMX214_REG_EXPOSURE, ctrl->val, &ret);
> > >                 break;
> > > -
> > > +       case V4L2_CID_VBLANK:
> > > +               cci_write(imx214->regmap, IMX214_REG_FRM_LENGTH_LINES,
> > > +                         format->height + ctrl->val, &ret);
>
> My datasheet says this register is "set up in multiples of 2".
> (LINE_LENGTH_PCK for HBLANK is "set in multiples of 8")
>
> I don't have one of these modules, but is that saying only multiples
> of 2 are permitted (in which case the step size for the control needs
> to reflect that), or that setting a value of N is interpreted by the
> hardware as 2N?
> Do all the numbers with PIXEL_RATE work out correctly in the frame rate calcs?

Answering my own question, PIXEL_RATE looks dubious.

The original code had LINE_LENGTH_PCK as 5008, and FRAME_LENGTH_LINES
as 3194. If enum_frame_intervals is to be believed, then it delivered
30fps.
5008 * 3194 * 30 = 479,866,560 Pix/s.

The driver defines IMX214_DEFAULT_LINK_FREQ 480000000, and then
IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10),
which works out as 384MPix/s. (Presumably the 8 is 4 lanes and DDR,
but it's not obvious)

Parsing the PLL registers with the defined 24MHz input. We're in
single PLL mode, so MIPI frequency is directly linked to pixel rate.
VTCK ends up being 1200MHz, and VTPXCK and OPPXCK both are 120MHz.
Section 5.3 "Frame rate calculation formula" says "Pixel rate
[pixels/s] = VTPXCK [MHz] * 4", so 120 * 4 = 480MPix/s, which
basically agrees with my number above.

3.1.4. MIPI global timing setting says "Output bitrate = OPPXCK * reg
0x113[7:0]", so 120MHz * 10, or 1200Mbit/s. That would be a link
frequency of 600MHz due to DDR.
That also matches to 480MPix/s * 10bpp / 4 lanes / 2 for DDR

Registers 0x0820-0823 are meant to define the target CSI2 bitrate, and
are set to 0x12c00000, or 314,572,800 decimal. I can't get that to
square with the above.


So my conclusion is that both PIXEL_RATE and LINK_FREQUENCY are wrong,
however the relationship used to define them looks to be correct.
Correct IMX214_DEFAULT_LINK_FREQ to 600MHz, and all should be good.
However you almost certainly want to set IMX214_VBLANK_MIN to
something significantly greater than 4.

  Dave

> Reading the spec sheet that 30fps is the max frame rate at full res
> (4096x2304), and the driver was setting a value of 3194 to this
> register, I don't see it being interpreted as 2N. Then again having
> VBLANK at 890 seems pretty high.
>
> > > +               break;
> > >         default:
> > >                 ret = -EINVAL;
> > >         }
> > > @@ -714,8 +777,11 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > >                 .width = 1120,
> > >                 .height = 1120,
> > >         };
> > > +       const struct imx214_mode *mode = &imx214_modes[0];
> > >         struct v4l2_fwnode_device_properties props;
> > >         struct v4l2_ctrl_handler *ctrl_hdlr;
> > > +       int exposure_max, exposure_def;
> > > +       int hblank;
> > >         int ret;
> > >
> > >         ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> > > @@ -723,7 +789,7 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > >                 return ret;
> > >
> > >         ctrl_hdlr = &imx214->ctrls;
> > > -       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> > > +       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> > >         if (ret)
> > >                 return ret;
> > >
> > > @@ -739,22 +805,26 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > >         if (imx214->link_freq)
> > >                 imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > >
> > > -       /*
> > > -        * WARNING!
> > > -        * Values obtained reverse engineering blobs and/or devices.
> > > -        * Ranges and functionality might be wrong.
> > > -        *
> > > -        * Sony, please release some register set documentation for the
> > > -        * device.
> > > -        *
> > > -        * Yours sincerely, Ricardo.
> > > -        */
> >
> > I would like to keep this comment until there is a public document available.
>
> I suspect you'll be waiting forever for a document to be officially released.
>
> I have a datasheet for IMX214, and I believe Kieran and Jacopo do too.
> Which specific values do you wish to be verified?
>
> >
> > > +       /* Initial vblank/hblank/exposure parameters based on current mode */
> > > +       imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> > > +                                          V4L2_CID_VBLANK, IMX214_VBLANK_MIN,
>
> IMX214_VBLANK_MIN being 4 feels plausible, but pretty low.
> I read the datasheet to say there are 4 embedded data lines per image.
> Seeing as you have STATS data output enabled as well that makes 5
> lines of non-image data per frame, but you're only adding blanking
> time for 4 lines.
>
> As noted above, I think you've also increased the max frame rate
> significantly above that quoted by Sony. Has that been actually
> exercised and confirmed to function correctly?
>
>   Dave
>
> > > +                                          IMX214_VTS_MAX - mode->height, 1,
> > > +                                          mode->vts_def - mode->height);
> > > +
> > > +       hblank = IMX214_PPL_DEFAULT - mode->width;
> > > +       imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> > > +                                          V4L2_CID_HBLANK, hblank, hblank,
> > > +                                          1, hblank);
> > > +       if (imx214->hblank)
> > > +               imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > > +       exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > > +       exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> > >         imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> > >                                              V4L2_CID_EXPOSURE,
> > > -                                            IMX214_EXPOSURE_MIN,
> > > -                                            IMX214_EXPOSURE_MAX,
> > > +                                            IMX214_EXPOSURE_MIN, exposure_max,
> > >                                              IMX214_EXPOSURE_STEP,
> > > -                                            IMX214_EXPOSURE_DEFAULT);
> > > +                                            exposure_def);
> > >
> > >         imx214->unit_size = v4l2_ctrl_new_std_compound(ctrl_hdlr,
> > >                                 NULL,
> > >
> > > --
> > > 2.46.0
> > >
> > >
> >

André Apitzsch Oct. 6, 2024, 9:41 p.m. UTC | #4

Hi Dave,

Am Freitag, dem 13.09.2024 um 18:40 +0100 schrieb Dave Stevenson:
> On Thu, 12 Sept 2024 at 15:51, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> > 
> > Hi André & Ricardo
> > 
> > On Thu, 12 Sept 2024 at 14:41, Ricardo Ribalda Delgado
> > <ribalda@kernel.org> wrote:
> > > 
> > > Hi
> > > 
> > > Arent you missing some chage in enum_frame_interval?
> > 
> > Raw sensors shouldn't be using [enum|set|get]_frame_interval at all
> > https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#frame-interval-configuration
> > 
> > The question now is how to handle the backwards compatibility for
> > any userspace app that might be using this driver and expecting to
> > use the frame_interval calls.
> > Seeing as it only ever allowed a fixed value of 30fps, leaving it
> > as is with a comment as to why it is there would be reasonable in
> > my view.

Should the comment be added in this commit?
And what exactly should the comment say?

> > 
> > > On Mon, Sep 2, 2024 at 11:53 PM André Apitzsch via B4 Relay
> > > <devnull+git.apitzsch.eu@kernel.org> wrote:
> > > > 
> > > > From: André Apitzsch <git@apitzsch.eu>
> > > > 
> > > > Add vblank control to allow changing the framerate /
> > > > higher exposure values.
> > > > 
> > > > The vblank and hblank controls are needed for libcamera
> > > > support.
> > > > 
> > > > While at it, fix the minimal exposure time according to the
> > > > datasheet.
> > > > 
> > > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > > ---
> > > >  drivers/media/i2c/imx214.c | 112
> > > > ++++++++++++++++++++++++++++++++++++---------
> > > >  1 file changed, 91 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/imx214.c
> > > > b/drivers/media/i2c/imx214.c
> > > > index 3b422cddbdce..9f5a57aebb86 100644
> > > > --- a/drivers/media/i2c/imx214.c
> > > > +++ b/drivers/media/i2c/imx214.c
> > > > @@ -34,11 +34,18 @@
> > > > 
> > > >  /* V-TIMING internal */
> > > >  #define IMX214_REG_FRM_LENGTH_LINES    CCI_REG16(0x0340)
> > > > +#define IMX214_VTS_MAX                 0xffff
> > > > +
> > > > +#define IMX214_VBLANK_MIN              4
> > > > +
> > > > +/* HBLANK control - read only */
> > > > +#define IMX214_PPL_DEFAULT             5008
> > > > 
> > > >  /* Exposure control */
> > > >  #define IMX214_REG_EXPOSURE            CCI_REG16(0x0202)
> > > > -#define IMX214_EXPOSURE_MIN            0
> > > > -#define IMX214_EXPOSURE_MAX            3184
> > > > +#define IMX214_EXPOSURE_OFFSET         10
> > > > +#define IMX214_EXPOSURE_MIN            1
> > > > +#define IMX214_EXPOSURE_MAX            (IMX214_VTS_MAX -
> > > > IMX214_EXPOSURE_OFFSET)
> > > >  #define IMX214_EXPOSURE_STEP           1
> > > >  #define IMX214_EXPOSURE_DEFAULT                3184
> > > >  #define IMX214_REG_EXPOSURE_RATIO      CCI_REG8(0x0222)
> > > > @@ -189,6 +196,8 @@ struct imx214 {
> > > >         struct v4l2_ctrl_handler ctrls;
> > > >         struct v4l2_ctrl *pixel_rate;
> > > >         struct v4l2_ctrl *link_freq;
> > > > +       struct v4l2_ctrl *vblank;
> > > > +       struct v4l2_ctrl *hblank;
> > > >         struct v4l2_ctrl *exposure;
> > > >         struct v4l2_ctrl *unit_size;
> > > > 
> > > > @@ -205,8 +214,6 @@ static const struct cci_reg_sequence
> > > > mode_4096x2304[] = {
> > > >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > > >         { IMX214_REG_HDR_RES_REDUCTION,
> > > > IMX214_HDR_RES_REDU_THROUGH },
> > > >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > > >         { IMX214_REG_X_ADD_STA, 56 },
> > > >         { IMX214_REG_Y_ADD_STA, 408 },
> > > >         { IMX214_REG_X_ADD_END, 4151 },
> > > > @@ -277,8 +284,6 @@ static const struct cci_reg_sequence
> > > > mode_1920x1080[] = {
> > > >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > > >         { IMX214_REG_HDR_RES_REDUCTION,
> > > > IMX214_HDR_RES_REDU_THROUGH },
> > > >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > > >         { IMX214_REG_X_ADD_STA, 1144 },
> > > >         { IMX214_REG_Y_ADD_STA, 1020 },
> > > >         { IMX214_REG_X_ADD_END, 3063 },
> > > > @@ -362,6 +367,7 @@ static const struct cci_reg_sequence
> > > > mode_table_common[] = {
> > > >         { IMX214_REG_ORIENTATION, 0 },
> > > >         { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK
> > > > },
> > > >         { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> > > > +       { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> > > >         { CCI_REG8(0x4550), 0x02 },
> > > >         { CCI_REG8(0x4601), 0x00 },
> > > >         { CCI_REG8(0x4642), 0x05 },
> > > > @@ -465,18 +471,24 @@ static const struct cci_reg_sequence
> > > > mode_table_common[] = {
> > > >  static const struct imx214_mode {
> > > >         u32 width;
> > > >         u32 height;
> > > > +
> > > > +       /* V-timing */
> > > > +       unsigned int vts_def;
> > > > +
> > > >         unsigned int num_of_regs;
> > > >         const struct cci_reg_sequence *reg_table;
> > > >  } imx214_modes[] = {
> > > >         {
> > > >                 .width = 4096,
> > > >                 .height = 2304,
> > > > +               .vts_def = 3194,
> > > >                 .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> > > >                 .reg_table = mode_4096x2304,
> > > >         },
> > > >         {
> > > >                 .width = 1920,
> > > >                 .height = 1080,
> > > > +               .vts_def = 3194,
> > > >                 .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> > > >                 .reg_table = mode_1920x1080,
> > > >         },
> > > > @@ -629,6 +641,37 @@ static int imx214_set_format(struct
> > > > v4l2_subdev *sd,
> > > >         __crop->width = mode->width;
> > > >         __crop->height = mode->height;
> > > > 
> > > > +       if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > > +               int exposure_max;
> > > > +               int exposure_def;
> > > > +               int hblank;
> > > > +
> > > > +               /* Update limits and set FPS to default */
> > > > +               __v4l2_ctrl_modify_range(imx214->vblank,
> > > > IMX214_VBLANK_MIN,
> > > > +                                        IMX214_VTS_MAX - mode-
> > > > >height, 1,
> > > > +                                        mode->vts_def - mode-
> > > > >height);
> > > > +               __v4l2_ctrl_s_ctrl(imx214->vblank,
> > > > +                                  mode->vts_def - mode-
> > > > >height);
> > > > +
> > > > +               /* Update max exposure while meeting expected
> > > > vblanking */
> > > > +               exposure_max = mode->vts_def -
> > > > IMX214_EXPOSURE_OFFSET;
> > > > +               exposure_def = (exposure_max <
> > > > IMX214_EXPOSURE_DEFAULT) ?
> > > > +                       exposure_max : IMX214_EXPOSURE_DEFAULT;
> > > > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > > > +                                        imx214->exposure-
> > > > >minimum,
> > > > +                                        exposure_max, imx214-
> > > > >exposure->step,
> > > > +                                        exposure_def);
> > > > +
> > > > +               /*
> > > > +                * Currently PPL is fixed to
> > > > IMX214_PPL_DEFAULT, so hblank
> > > > +                * depends on mode->width only, and is not
> > > > changeble in any
> > > > +                * way other than changing the mode.
> > > > +                */
> > > > +               hblank = IMX214_PPL_DEFAULT - mode->width;
> > > > +               __v4l2_ctrl_modify_range(imx214->hblank,
> > > > hblank, hblank, 1,
> > > > +                                        hblank);
> > > > +       }
> > > > +
> > > >         return 0;
> > > >  }
> > > > 
> > > > @@ -678,8 +721,25 @@ static int imx214_set_ctrl(struct
> > > > v4l2_ctrl *ctrl)
> > > >  {
> > > >         struct imx214 *imx214 = container_of(ctrl->handler,
> > > >                                              struct imx214,
> > > > ctrls);
> > > > +       const struct v4l2_mbus_framefmt *format;
> > > > +       struct v4l2_subdev_state *state;
> > > >         int ret;
> > > > 
> > > > +       state = v4l2_subdev_get_locked_active_state(&imx214-
> > > > >sd);
> > > > +       format = v4l2_subdev_state_get_format(state, 0);
> > > > +
> > > > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > > > +               int exposure_max, exposure_def;
> > > > +
> > > > +               /* Update max exposure while meeting expected
> > > > vblanking */
> > > > +               exposure_max = format->height + ctrl->val -
> > > > IMX214_EXPOSURE_OFFSET;
> > > > +               exposure_def = min(exposure_max,
> > > > IMX214_EXPOSURE_DEFAULT);
> > > > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > > > +                                        imx214->exposure-
> > > > >minimum,
> > > > +                                        exposure_max, imx214-
> > > > >exposure->step,
> > > > +                                        exposure_def);
> > > > +       }
> > > > +
> > > >         /*
> > > >          * Applying V4L2 control value only happens
> > > >          * when power is up for streaming
> > > > @@ -691,7 +751,10 @@ static int imx214_set_ctrl(struct
> > > > v4l2_ctrl *ctrl)
> > > >         case V4L2_CID_EXPOSURE:
> > > >                 cci_write(imx214->regmap, IMX214_REG_EXPOSURE,
> > > > ctrl->val, &ret);
> > > >                 break;
> > > > -
> > > > +       case V4L2_CID_VBLANK:
> > > > +               cci_write(imx214->regmap,
> > > > IMX214_REG_FRM_LENGTH_LINES,
> > > > +                         format->height + ctrl->val, &ret);
> > 
> > My datasheet says this register is "set up in multiples of 2".
> > (LINE_LENGTH_PCK for HBLANK is "set in multiples of 8")
> > 
> > I don't have one of these modules, but is that saying only
> > multiples
> > of 2 are permitted (in which case the step size for the control
> > needs
> > to reflect that), or that setting a value of N is interpreted by
> > the
> > hardware as 2N?
> > Do all the numbers with PIXEL_RATE work out correctly in the frame
> > rate calcs?
> 
> Answering my own question, PIXEL_RATE looks dubious.
> 
> The original code had LINE_LENGTH_PCK as 5008, and FRAME_LENGTH_LINES
> as 3194. If enum_frame_intervals is to be believed, then it delivered
> 30fps.
> 5008 * 3194 * 30 = 479,866,560 Pix/s.
> 
> The driver defines IMX214_DEFAULT_LINK_FREQ 480000000, and then
> IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10),
> which works out as 384MPix/s. (Presumably the 8 is 4 lanes and DDR,
> but it's not obvious)
> 
> Parsing the PLL registers with the defined 24MHz input. We're in
> single PLL mode, so MIPI frequency is directly linked to pixel rate.
> VTCK ends up being 1200MHz, and VTPXCK and OPPXCK both are 120MHz.
> Section 5.3 "Frame rate calculation formula" says "Pixel rate
> [pixels/s] = VTPXCK [MHz] * 4", so 120 * 4 = 480MPix/s, which
> basically agrees with my number above.
> 
> 3.1.4. MIPI global timing setting says "Output bitrate = OPPXCK * reg
> 0x113[7:0]", so 120MHz * 10, or 1200Mbit/s. That would be a link
> frequency of 600MHz due to DDR.
> That also matches to 480MPix/s * 10bpp / 4 lanes / 2 for DDR
> 
> Registers 0x0820-0823 are meant to define the target CSI2 bitrate,
> and are set to 0x12c00000, or 314,572,800 decimal. I can't get that
> to square with the above.
> 
> 
> So my conclusion is that both PIXEL_RATE and LINK_FREQUENCY are
> wrong, however the relationship used to define them looks to be
> correct.
> Correct IMX214_DEFAULT_LINK_FREQ to 600MHz, and all should be good.

Will do that. Should it be done in this commit or in a separat one?

> However you almost certainly want to set IMX214_VBLANK_MIN to
> something significantly greater than 4.

Any recommendation for the new IMX214_VBLANK_MIN value?

Best regards,
André

> 
>   Dave
> 
> > Reading the spec sheet that 30fps is the max frame rate at full res
> > (4096x2304), and the driver was setting a value of 3194 to this
> > register, I don't see it being interpreted as 2N. Then again having
> > VBLANK at 890 seems pretty high.
> > 
> > > > +               break;
> > > >         default:
> > > >                 ret = -EINVAL;
> > > >         }
> > > > @@ -714,8 +777,11 @@ static int imx214_ctrls_init(struct imx214
> > > > *imx214)
> > > >                 .width = 1120,
> > > >                 .height = 1120,
> > > >         };
> > > > +       const struct imx214_mode *mode = &imx214_modes[0];
> > > >         struct v4l2_fwnode_device_properties props;
> > > >         struct v4l2_ctrl_handler *ctrl_hdlr;
> > > > +       int exposure_max, exposure_def;
> > > > +       int hblank;
> > > >         int ret;
> > > > 
> > > >         ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> > > > @@ -723,7 +789,7 @@ static int imx214_ctrls_init(struct imx214
> > > > *imx214)
> > > >                 return ret;
> > > > 
> > > >         ctrl_hdlr = &imx214->ctrls;
> > > > -       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> > > > +       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> > > >         if (ret)
> > > >                 return ret;
> > > > 
> > > > @@ -739,22 +805,26 @@ static int imx214_ctrls_init(struct
> > > > imx214 *imx214)
> > > >         if (imx214->link_freq)
> > > >                 imx214->link_freq->flags |=
> > > > V4L2_CTRL_FLAG_READ_ONLY;
> > > > 
> > > > -       /*
> > > > -        * WARNING!
> > > > -        * Values obtained reverse engineering blobs and/or
> > > > devices.
> > > > -        * Ranges and functionality might be wrong.
> > > > -        *
> > > > -        * Sony, please release some register set documentation
> > > > for the
> > > > -        * device.
> > > > -        *
> > > > -        * Yours sincerely, Ricardo.
> > > > -        */
> > > 
> > > I would like to keep this comment until there is a public
> > > document available.
> > 
> > I suspect you'll be waiting forever for a document to be officially
> > released.
> > 
> > I have a datasheet for IMX214, and I believe Kieran and Jacopo do
> > too.
> > Which specific values do you wish to be verified?
> > 
> > > 
> > > > +       /* Initial vblank/hblank/exposure parameters based on
> > > > current mode */
> > > > +       imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > &imx214_ctrl_ops,
> > > > +                                          V4L2_CID_VBLANK,
> > > > IMX214_VBLANK_MIN,
> > 
> > IMX214_VBLANK_MIN being 4 feels plausible, but pretty low.
> > I read the datasheet to say there are 4 embedded data lines per
> > image.
> > Seeing as you have STATS data output enabled as well that makes 5
> > lines of non-image data per frame, but you're only adding blanking
> > time for 4 lines.
> > 
> > As noted above, I think you've also increased the max frame rate
> > significantly above that quoted by Sony. Has that been actually
> > exercised and confirmed to function correctly?
> > 
> >   Dave
> > 
> > > > +                                          IMX214_VTS_MAX -
> > > > mode->height, 1,
> > > > +                                          mode->vts_def -
> > > > mode->height);
> > > > +
> > > > +       hblank = IMX214_PPL_DEFAULT - mode->width;
> > > > +       imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > &imx214_ctrl_ops,
> > > > +                                          V4L2_CID_HBLANK,
> > > > hblank, hblank,
> > > > +                                          1, hblank);
> > > > +       if (imx214->hblank)
> > > > +               imx214->hblank->flags |=
> > > > V4L2_CTRL_FLAG_READ_ONLY;
> > > > +
> > > > +       exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > > > +       exposure_def = min(exposure_max,
> > > > IMX214_EXPOSURE_DEFAULT);
> > > >         imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > &imx214_ctrl_ops,
> > > >                                              V4L2_CID_EXPOSURE,
> > > > -                                           
> > > > IMX214_EXPOSURE_MIN,
> > > > -                                           
> > > > IMX214_EXPOSURE_MAX,
> > > > +                                           
> > > > IMX214_EXPOSURE_MIN, exposure_max,
> > > >                                             
> > > > IMX214_EXPOSURE_STEP,
> > > > -                                           
> > > > IMX214_EXPOSURE_DEFAULT);
> > > > +                                            exposure_def);
> > > > 
> > > >         imx214->unit_size =
> > > > v4l2_ctrl_new_std_compound(ctrl_hdlr,
> > > >                                 NULL,
> > > > 
> > > > --
> > > > 2.46.0
> > > > 
> > > > 
> > >

Dave Stevenson Oct. 7, 2024, 5:29 p.m. UTC | #5

Hi Andre

On Sun, 6 Oct 2024 at 22:41, André <git@apitzsch.eu> wrote:
>
> Hi Dave,
>
> Am Freitag, dem 13.09.2024 um 18:40 +0100 schrieb Dave Stevenson:
> > On Thu, 12 Sept 2024 at 15:51, Dave Stevenson
> > <dave.stevenson@raspberrypi.com> wrote:
> > >
> > > Hi André & Ricardo
> > >
> > > On Thu, 12 Sept 2024 at 14:41, Ricardo Ribalda Delgado
> > > <ribalda@kernel.org> wrote:
> > > >
> > > > Hi
> > > >
> > > > Arent you missing some chage in enum_frame_interval?
> > >
> > > Raw sensors shouldn't be using [enum|set|get]_frame_interval at all
> > > https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#frame-interval-configuration
> > >
> > > The question now is how to handle the backwards compatibility for
> > > any userspace app that might be using this driver and expecting to
> > > use the frame_interval calls.
> > > Seeing as it only ever allowed a fixed value of 30fps, leaving it
> > > as is with a comment as to why it is there would be reasonable in
> > > my view.
>
> Should the comment be added in this commit?
> And what exactly should the comment say?

Add it to imx214_enum_frame_interval.
Something along the lines of:
Raw sensors should be using the VBLANK and HBLANK controls to
determine the frame rate. However this driver was initially added
using the [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of 30fps.
Retain the frame_interval ops for backwards compatibility, but they do nothing.

> > >
> > > > On Mon, Sep 2, 2024 at 11:53 PM André Apitzsch via B4 Relay
> > > > <devnull+git.apitzsch.eu@kernel.org> wrote:
> > > > >
> > > > > From: André Apitzsch <git@apitzsch.eu>
> > > > >
> > > > > Add vblank control to allow changing the framerate /
> > > > > higher exposure values.
> > > > >
> > > > > The vblank and hblank controls are needed for libcamera
> > > > > support.
> > > > >
> > > > > While at it, fix the minimal exposure time according to the
> > > > > datasheet.
> > > > >
> > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > > > ---
> > > > >  drivers/media/i2c/imx214.c | 112
> > > > > ++++++++++++++++++++++++++++++++++++---------
> > > > >  1 file changed, 91 insertions(+), 21 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx214.c
> > > > > b/drivers/media/i2c/imx214.c
> > > > > index 3b422cddbdce..9f5a57aebb86 100644
> > > > > --- a/drivers/media/i2c/imx214.c
> > > > > +++ b/drivers/media/i2c/imx214.c
> > > > > @@ -34,11 +34,18 @@
> > > > >
> > > > >  /* V-TIMING internal */
> > > > >  #define IMX214_REG_FRM_LENGTH_LINES    CCI_REG16(0x0340)
> > > > > +#define IMX214_VTS_MAX                 0xffff
> > > > > +
> > > > > +#define IMX214_VBLANK_MIN              4
> > > > > +
> > > > > +/* HBLANK control - read only */
> > > > > +#define IMX214_PPL_DEFAULT             5008
> > > > >
> > > > >  /* Exposure control */
> > > > >  #define IMX214_REG_EXPOSURE            CCI_REG16(0x0202)
> > > > > -#define IMX214_EXPOSURE_MIN            0
> > > > > -#define IMX214_EXPOSURE_MAX            3184
> > > > > +#define IMX214_EXPOSURE_OFFSET         10
> > > > > +#define IMX214_EXPOSURE_MIN            1
> > > > > +#define IMX214_EXPOSURE_MAX            (IMX214_VTS_MAX -
> > > > > IMX214_EXPOSURE_OFFSET)
> > > > >  #define IMX214_EXPOSURE_STEP           1
> > > > >  #define IMX214_EXPOSURE_DEFAULT                3184
> > > > >  #define IMX214_REG_EXPOSURE_RATIO      CCI_REG8(0x0222)
> > > > > @@ -189,6 +196,8 @@ struct imx214 {
> > > > >         struct v4l2_ctrl_handler ctrls;
> > > > >         struct v4l2_ctrl *pixel_rate;
> > > > >         struct v4l2_ctrl *link_freq;
> > > > > +       struct v4l2_ctrl *vblank;
> > > > > +       struct v4l2_ctrl *hblank;
> > > > >         struct v4l2_ctrl *exposure;
> > > > >         struct v4l2_ctrl *unit_size;
> > > > >
> > > > > @@ -205,8 +214,6 @@ static const struct cci_reg_sequence
> > > > > mode_4096x2304[] = {
> > > > >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > > > >         { IMX214_REG_HDR_RES_REDUCTION,
> > > > > IMX214_HDR_RES_REDU_THROUGH },
> > > > >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > > > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > > > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > > > >         { IMX214_REG_X_ADD_STA, 56 },
> > > > >         { IMX214_REG_Y_ADD_STA, 408 },
> > > > >         { IMX214_REG_X_ADD_END, 4151 },
> > > > > @@ -277,8 +284,6 @@ static const struct cci_reg_sequence
> > > > > mode_1920x1080[] = {
> > > > >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > > > >         { IMX214_REG_HDR_RES_REDUCTION,
> > > > > IMX214_HDR_RES_REDU_THROUGH },
> > > > >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > > > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > > > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > > > >         { IMX214_REG_X_ADD_STA, 1144 },
> > > > >         { IMX214_REG_Y_ADD_STA, 1020 },
> > > > >         { IMX214_REG_X_ADD_END, 3063 },
> > > > > @@ -362,6 +367,7 @@ static const struct cci_reg_sequence
> > > > > mode_table_common[] = {
> > > > >         { IMX214_REG_ORIENTATION, 0 },
> > > > >         { IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK
> > > > > },
> > > > >         { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> > > > > +       { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> > > > >         { CCI_REG8(0x4550), 0x02 },
> > > > >         { CCI_REG8(0x4601), 0x00 },
> > > > >         { CCI_REG8(0x4642), 0x05 },
> > > > > @@ -465,18 +471,24 @@ static const struct cci_reg_sequence
> > > > > mode_table_common[] = {
> > > > >  static const struct imx214_mode {
> > > > >         u32 width;
> > > > >         u32 height;
> > > > > +
> > > > > +       /* V-timing */
> > > > > +       unsigned int vts_def;
> > > > > +
> > > > >         unsigned int num_of_regs;
> > > > >         const struct cci_reg_sequence *reg_table;
> > > > >  } imx214_modes[] = {
> > > > >         {
> > > > >                 .width = 4096,
> > > > >                 .height = 2304,
> > > > > +               .vts_def = 3194,
> > > > >                 .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> > > > >                 .reg_table = mode_4096x2304,
> > > > >         },
> > > > >         {
> > > > >                 .width = 1920,
> > > > >                 .height = 1080,
> > > > > +               .vts_def = 3194,
> > > > >                 .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> > > > >                 .reg_table = mode_1920x1080,
> > > > >         },
> > > > > @@ -629,6 +641,37 @@ static int imx214_set_format(struct
> > > > > v4l2_subdev *sd,
> > > > >         __crop->width = mode->width;
> > > > >         __crop->height = mode->height;
> > > > >
> > > > > +       if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > > > +               int exposure_max;
> > > > > +               int exposure_def;
> > > > > +               int hblank;
> > > > > +
> > > > > +               /* Update limits and set FPS to default */
> > > > > +               __v4l2_ctrl_modify_range(imx214->vblank,
> > > > > IMX214_VBLANK_MIN,
> > > > > +                                        IMX214_VTS_MAX - mode-
> > > > > >height, 1,
> > > > > +                                        mode->vts_def - mode-
> > > > > >height);
> > > > > +               __v4l2_ctrl_s_ctrl(imx214->vblank,
> > > > > +                                  mode->vts_def - mode-
> > > > > >height);
> > > > > +
> > > > > +               /* Update max exposure while meeting expected
> > > > > vblanking */
> > > > > +               exposure_max = mode->vts_def -
> > > > > IMX214_EXPOSURE_OFFSET;
> > > > > +               exposure_def = (exposure_max <
> > > > > IMX214_EXPOSURE_DEFAULT) ?
> > > > > +                       exposure_max : IMX214_EXPOSURE_DEFAULT;
> > > > > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > > > > +                                        imx214->exposure-
> > > > > >minimum,
> > > > > +                                        exposure_max, imx214-
> > > > > >exposure->step,
> > > > > +                                        exposure_def);
> > > > > +
> > > > > +               /*
> > > > > +                * Currently PPL is fixed to
> > > > > IMX214_PPL_DEFAULT, so hblank
> > > > > +                * depends on mode->width only, and is not
> > > > > changeble in any
> > > > > +                * way other than changing the mode.
> > > > > +                */
> > > > > +               hblank = IMX214_PPL_DEFAULT - mode->width;
> > > > > +               __v4l2_ctrl_modify_range(imx214->hblank,
> > > > > hblank, hblank, 1,
> > > > > +                                        hblank);
> > > > > +       }
> > > > > +
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > @@ -678,8 +721,25 @@ static int imx214_set_ctrl(struct
> > > > > v4l2_ctrl *ctrl)
> > > > >  {
> > > > >         struct imx214 *imx214 = container_of(ctrl->handler,
> > > > >                                              struct imx214,
> > > > > ctrls);
> > > > > +       const struct v4l2_mbus_framefmt *format;
> > > > > +       struct v4l2_subdev_state *state;
> > > > >         int ret;
> > > > >
> > > > > +       state = v4l2_subdev_get_locked_active_state(&imx214-
> > > > > >sd);
> > > > > +       format = v4l2_subdev_state_get_format(state, 0);
> > > > > +
> > > > > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > > > > +               int exposure_max, exposure_def;
> > > > > +
> > > > > +               /* Update max exposure while meeting expected
> > > > > vblanking */
> > > > > +               exposure_max = format->height + ctrl->val -
> > > > > IMX214_EXPOSURE_OFFSET;
> > > > > +               exposure_def = min(exposure_max,
> > > > > IMX214_EXPOSURE_DEFAULT);
> > > > > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > > > > +                                        imx214->exposure-
> > > > > >minimum,
> > > > > +                                        exposure_max, imx214-
> > > > > >exposure->step,
> > > > > +                                        exposure_def);
> > > > > +       }
> > > > > +
> > > > >         /*
> > > > >          * Applying V4L2 control value only happens
> > > > >          * when power is up for streaming
> > > > > @@ -691,7 +751,10 @@ static int imx214_set_ctrl(struct
> > > > > v4l2_ctrl *ctrl)
> > > > >         case V4L2_CID_EXPOSURE:
> > > > >                 cci_write(imx214->regmap, IMX214_REG_EXPOSURE,
> > > > > ctrl->val, &ret);
> > > > >                 break;
> > > > > -
> > > > > +       case V4L2_CID_VBLANK:
> > > > > +               cci_write(imx214->regmap,
> > > > > IMX214_REG_FRM_LENGTH_LINES,
> > > > > +                         format->height + ctrl->val, &ret);
> > >
> > > My datasheet says this register is "set up in multiples of 2".
> > > (LINE_LENGTH_PCK for HBLANK is "set in multiples of 8")
> > >
> > > I don't have one of these modules, but is that saying only
> > > multiples
> > > of 2 are permitted (in which case the step size for the control
> > > needs
> > > to reflect that), or that setting a value of N is interpreted by
> > > the
> > > hardware as 2N?
> > > Do all the numbers with PIXEL_RATE work out correctly in the frame
> > > rate calcs?
> >
> > Answering my own question, PIXEL_RATE looks dubious.
> >
> > The original code had LINE_LENGTH_PCK as 5008, and FRAME_LENGTH_LINES
> > as 3194. If enum_frame_intervals is to be believed, then it delivered
> > 30fps.
> > 5008 * 3194 * 30 = 479,866,560 Pix/s.
> >
> > The driver defines IMX214_DEFAULT_LINK_FREQ 480000000, and then
> > IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10),
> > which works out as 384MPix/s. (Presumably the 8 is 4 lanes and DDR,
> > but it's not obvious)
> >
> > Parsing the PLL registers with the defined 24MHz input. We're in
> > single PLL mode, so MIPI frequency is directly linked to pixel rate.
> > VTCK ends up being 1200MHz, and VTPXCK and OPPXCK both are 120MHz.
> > Section 5.3 "Frame rate calculation formula" says "Pixel rate
> > [pixels/s] = VTPXCK [MHz] * 4", so 120 * 4 = 480MPix/s, which
> > basically agrees with my number above.
> >
> > 3.1.4. MIPI global timing setting says "Output bitrate = OPPXCK * reg
> > 0x113[7:0]", so 120MHz * 10, or 1200Mbit/s. That would be a link
> > frequency of 600MHz due to DDR.
> > That also matches to 480MPix/s * 10bpp / 4 lanes / 2 for DDR
> >
> > Registers 0x0820-0823 are meant to define the target CSI2 bitrate,
> > and are set to 0x12c00000, or 314,572,800 decimal. I can't get that
> > to square with the above.
> >
> >
> > So my conclusion is that both PIXEL_RATE and LINK_FREQUENCY are
> > wrong, however the relationship used to define them looks to be
> > correct.
> > Correct IMX214_DEFAULT_LINK_FREQ to 600MHz, and all should be good.
>
> Will do that. Should it be done in this commit or in a separat one?

A separate one as it is independent of adding VBLANK and HBLANK
controls. Ideally it should come before this patch as then hopefully
everything actually works correctly when you add them.

> > However you almost certainly want to set IMX214_VBLANK_MIN to
> > something significantly greater than 4.
>
> Any recommendation for the new IMX214_VBLANK_MIN value?

As my comment below, the datasheet lists 30fps as the max frame rate
for full res, therefore IMX214_VBLANK_MIN being correct for that seems
reasonable. 890 appears to be the value for that, but I haven't
verified that all the numbers do then work out.

Out of interest, what platform are you testing this on, and where are
you getting the sensor from? I'm always happy to tinker with sensors
that I can get working on a Pi, and I think the Arducam DepthAI Oak
module should work, but haven't picked one up yet.

  Dave

> Best regards,
> André
>
> >
> >   Dave
> >
> > > Reading the spec sheet that 30fps is the max frame rate at full res
> > > (4096x2304), and the driver was setting a value of 3194 to this
> > > register, I don't see it being interpreted as 2N. Then again having
> > > VBLANK at 890 seems pretty high.
> > >
> > > > > +               break;
> > > > >         default:
> > > > >                 ret = -EINVAL;
> > > > >         }
> > > > > @@ -714,8 +777,11 @@ static int imx214_ctrls_init(struct imx214
> > > > > *imx214)
> > > > >                 .width = 1120,
> > > > >                 .height = 1120,
> > > > >         };
> > > > > +       const struct imx214_mode *mode = &imx214_modes[0];
> > > > >         struct v4l2_fwnode_device_properties props;
> > > > >         struct v4l2_ctrl_handler *ctrl_hdlr;
> > > > > +       int exposure_max, exposure_def;
> > > > > +       int hblank;
> > > > >         int ret;
> > > > >
> > > > >         ret = v4l2_fwnode_device_parse(imx214->dev, &props);
> > > > > @@ -723,7 +789,7 @@ static int imx214_ctrls_init(struct imx214
> > > > > *imx214)
> > > > >                 return ret;
> > > > >
> > > > >         ctrl_hdlr = &imx214->ctrls;
> > > > > -       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> > > > > +       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> > > > >         if (ret)
> > > > >                 return ret;
> > > > >
> > > > > @@ -739,22 +805,26 @@ static int imx214_ctrls_init(struct
> > > > > imx214 *imx214)
> > > > >         if (imx214->link_freq)
> > > > >                 imx214->link_freq->flags |=
> > > > > V4L2_CTRL_FLAG_READ_ONLY;
> > > > >
> > > > > -       /*
> > > > > -        * WARNING!
> > > > > -        * Values obtained reverse engineering blobs and/or
> > > > > devices.
> > > > > -        * Ranges and functionality might be wrong.
> > > > > -        *
> > > > > -        * Sony, please release some register set documentation
> > > > > for the
> > > > > -        * device.
> > > > > -        *
> > > > > -        * Yours sincerely, Ricardo.
> > > > > -        */
> > > >
> > > > I would like to keep this comment until there is a public
> > > > document available.
> > >
> > > I suspect you'll be waiting forever for a document to be officially
> > > released.
> > >
> > > I have a datasheet for IMX214, and I believe Kieran and Jacopo do
> > > too.
> > > Which specific values do you wish to be verified?
> > >
> > > >
> > > > > +       /* Initial vblank/hblank/exposure parameters based on
> > > > > current mode */
> > > > > +       imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > &imx214_ctrl_ops,
> > > > > +                                          V4L2_CID_VBLANK,
> > > > > IMX214_VBLANK_MIN,
> > >
> > > IMX214_VBLANK_MIN being 4 feels plausible, but pretty low.
> > > I read the datasheet to say there are 4 embedded data lines per
> > > image.
> > > Seeing as you have STATS data output enabled as well that makes 5
> > > lines of non-image data per frame, but you're only adding blanking
> > > time for 4 lines.
> > >
> > > As noted above, I think you've also increased the max frame rate
> > > significantly above that quoted by Sony. Has that been actually
> > > exercised and confirmed to function correctly?
> > >
> > >   Dave
> > >
> > > > > +                                          IMX214_VTS_MAX -
> > > > > mode->height, 1,
> > > > > +                                          mode->vts_def -
> > > > > mode->height);
> > > > > +
> > > > > +       hblank = IMX214_PPL_DEFAULT - mode->width;
> > > > > +       imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > &imx214_ctrl_ops,
> > > > > +                                          V4L2_CID_HBLANK,
> > > > > hblank, hblank,
> > > > > +                                          1, hblank);
> > > > > +       if (imx214->hblank)
> > > > > +               imx214->hblank->flags |=
> > > > > V4L2_CTRL_FLAG_READ_ONLY;
> > > > > +
> > > > > +       exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> > > > > +       exposure_def = min(exposure_max,
> > > > > IMX214_EXPOSURE_DEFAULT);
> > > > >         imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > &imx214_ctrl_ops,
> > > > >                                              V4L2_CID_EXPOSURE,
> > > > > -
> > > > > IMX214_EXPOSURE_MIN,
> > > > > -
> > > > > IMX214_EXPOSURE_MAX,
> > > > > +
> > > > > IMX214_EXPOSURE_MIN, exposure_max,
> > > > >
> > > > > IMX214_EXPOSURE_STEP,
> > > > > -
> > > > > IMX214_EXPOSURE_DEFAULT);
> > > > > +                                            exposure_def);
> > > > >
> > > > >         imx214->unit_size =
> > > > > v4l2_ctrl_new_std_compound(ctrl_hdlr,
> > > > >                                 NULL,
> > > > >
> > > > > --
> > > > > 2.46.0
> > > > >
> > > > >
> > > >
>

André Apitzsch Oct. 12, 2024, 9:45 p.m. UTC | #6

Hi Dave,

Am Montag, dem 07.10.2024 um 18:29 +0100 schrieb Dave Stevenson:
> Hi Andre
> 
> On Sun, 6 Oct 2024 at 22:41, André <git@apitzsch.eu> wrote:
> > 
> > Hi Dave,
> > 
> > Am Freitag, dem 13.09.2024 um 18:40 +0100 schrieb Dave Stevenson:
> > > On Thu, 12 Sept 2024 at 15:51, Dave Stevenson
> > > <dave.stevenson@raspberrypi.com> wrote:
> > > > 
> > > > Hi André & Ricardo
> > > > 
> > > > On Thu, 12 Sept 2024 at 14:41, Ricardo Ribalda Delgado
> > > > <ribalda@kernel.org> wrote:
> > > > > 
> > > > > Hi
> > > > > 
> > > > > Arent you missing some chage in enum_frame_interval?
> > > > 
> > > > Raw sensors shouldn't be using [enum|set|get]_frame_interval at
> > > > all
> > > > https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#frame-interval-configuration
> > > > 
> > > > The question now is how to handle the backwards compatibility
> > > > for
> > > > any userspace app that might be using this driver and expecting
> > > > to
> > > > use the frame_interval calls.
> > > > Seeing as it only ever allowed a fixed value of 30fps, leaving
> > > > it
> > > > as is with a comment as to why it is there would be reasonable
> > > > in
> > > > my view.
> > 
> > Should the comment be added in this commit?
> > And what exactly should the comment say?
> 
> Add it to imx214_enum_frame_interval.
> Something along the lines of:
> Raw sensors should be using the VBLANK and HBLANK controls to
> determine the frame rate. However this driver was initially added
> using the [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of
> 30fps.
> Retain the frame_interval ops for backwards compatibility, but they
> do nothing.
> 
> > > > 
> > > > > On Mon, Sep 2, 2024 at 11:53 PM André Apitzsch via B4 Relay
> > > > > <devnull+git.apitzsch.eu@kernel.org> wrote:
> > > > > > 
> > > > > > From: André Apitzsch <git@apitzsch.eu>
> > > > > > 
> > > > > > Add vblank control to allow changing the framerate /
> > > > > > higher exposure values.
> > > > > > 
> > > > > > The vblank and hblank controls are needed for libcamera
> > > > > > support.
> > > > > > 
> > > > > > While at it, fix the minimal exposure time according to the
> > > > > > datasheet.
> > > > > > 
> > > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > > > > ---
> > > > > >  drivers/media/i2c/imx214.c | 112
> > > > > > ++++++++++++++++++++++++++++++++++++---------
> > > > > >  1 file changed, 91 insertions(+), 21 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/i2c/imx214.c
> > > > > > b/drivers/media/i2c/imx214.c
> > > > > > index 3b422cddbdce..9f5a57aebb86 100644
> > > > > > --- a/drivers/media/i2c/imx214.c
> > > > > > +++ b/drivers/media/i2c/imx214.c
> > > > > > @@ -34,11 +34,18 @@
> > > > > > 
> > > > > >  /* V-TIMING internal */
> > > > > >  #define IMX214_REG_FRM_LENGTH_LINES    CCI_REG16(0x0340)
> > > > > > +#define IMX214_VTS_MAX                 0xffff
> > > > > > +
> > > > > > +#define IMX214_VBLANK_MIN              4
> > > > > > +
> > > > > > +/* HBLANK control - read only */
> > > > > > +#define IMX214_PPL_DEFAULT             5008
> > > > > > 
> > > > > >  /* Exposure control */
> > > > > >  #define IMX214_REG_EXPOSURE            CCI_REG16(0x0202)
> > > > > > -#define IMX214_EXPOSURE_MIN            0
> > > > > > -#define IMX214_EXPOSURE_MAX            3184
> > > > > > +#define IMX214_EXPOSURE_OFFSET         10
> > > > > > +#define IMX214_EXPOSURE_MIN            1
> > > > > > +#define IMX214_EXPOSURE_MAX            (IMX214_VTS_MAX -
> > > > > > IMX214_EXPOSURE_OFFSET)
> > > > > >  #define IMX214_EXPOSURE_STEP           1
> > > > > >  #define IMX214_EXPOSURE_DEFAULT                3184
> > > > > >  #define IMX214_REG_EXPOSURE_RATIO      CCI_REG8(0x0222)
> > > > > > @@ -189,6 +196,8 @@ struct imx214 {
> > > > > >         struct v4l2_ctrl_handler ctrls;
> > > > > >         struct v4l2_ctrl *pixel_rate;
> > > > > >         struct v4l2_ctrl *link_freq;
> > > > > > +       struct v4l2_ctrl *vblank;
> > > > > > +       struct v4l2_ctrl *hblank;
> > > > > >         struct v4l2_ctrl *exposure;
> > > > > >         struct v4l2_ctrl *unit_size;
> > > > > > 
> > > > > > @@ -205,8 +214,6 @@ static const struct cci_reg_sequence
> > > > > > mode_4096x2304[] = {
> > > > > >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > > > > >         { IMX214_REG_HDR_RES_REDUCTION,
> > > > > > IMX214_HDR_RES_REDU_THROUGH },
> > > > > >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > > > > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > > > > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > > > > >         { IMX214_REG_X_ADD_STA, 56 },
> > > > > >         { IMX214_REG_Y_ADD_STA, 408 },
> > > > > >         { IMX214_REG_X_ADD_END, 4151 },
> > > > > > @@ -277,8 +284,6 @@ static const struct cci_reg_sequence
> > > > > > mode_1920x1080[] = {
> > > > > >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > > > > >         { IMX214_REG_HDR_RES_REDUCTION,
> > > > > > IMX214_HDR_RES_REDU_THROUGH },
> > > > > >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > > > > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > > > > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > > > > >         { IMX214_REG_X_ADD_STA, 1144 },
> > > > > >         { IMX214_REG_Y_ADD_STA, 1020 },
> > > > > >         { IMX214_REG_X_ADD_END, 3063 },
> > > > > > @@ -362,6 +367,7 @@ static const struct cci_reg_sequence
> > > > > > mode_table_common[] = {
> > > > > >         { IMX214_REG_ORIENTATION, 0 },
> > > > > >         { IMX214_REG_MASK_CORR_FRAMES,
> > > > > > IMX214_CORR_FRAMES_MASK
> > > > > > },
> > > > > >         { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> > > > > > +       { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> > > > > >         { CCI_REG8(0x4550), 0x02 },
> > > > > >         { CCI_REG8(0x4601), 0x00 },
> > > > > >         { CCI_REG8(0x4642), 0x05 },
> > > > > > @@ -465,18 +471,24 @@ static const struct cci_reg_sequence
> > > > > > mode_table_common[] = {
> > > > > >  static const struct imx214_mode {
> > > > > >         u32 width;
> > > > > >         u32 height;
> > > > > > +
> > > > > > +       /* V-timing */
> > > > > > +       unsigned int vts_def;
> > > > > > +
> > > > > >         unsigned int num_of_regs;
> > > > > >         const struct cci_reg_sequence *reg_table;
> > > > > >  } imx214_modes[] = {
> > > > > >         {
> > > > > >                 .width = 4096,
> > > > > >                 .height = 2304,
> > > > > > +               .vts_def = 3194,
> > > > > >                 .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> > > > > >                 .reg_table = mode_4096x2304,
> > > > > >         },
> > > > > >         {
> > > > > >                 .width = 1920,
> > > > > >                 .height = 1080,
> > > > > > +               .vts_def = 3194,
> > > > > >                 .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> > > > > >                 .reg_table = mode_1920x1080,
> > > > > >         },
> > > > > > @@ -629,6 +641,37 @@ static int imx214_set_format(struct
> > > > > > v4l2_subdev *sd,
> > > > > >         __crop->width = mode->width;
> > > > > >         __crop->height = mode->height;
> > > > > > 
> > > > > > +       if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > > > > +               int exposure_max;
> > > > > > +               int exposure_def;
> > > > > > +               int hblank;
> > > > > > +
> > > > > > +               /* Update limits and set FPS to default */
> > > > > > +               __v4l2_ctrl_modify_range(imx214->vblank,
> > > > > > IMX214_VBLANK_MIN,
> > > > > > +                                        IMX214_VTS_MAX -
> > > > > > mode-
> > > > > > > height, 1,
> > > > > > +                                        mode->vts_def -
> > > > > > mode-
> > > > > > > height);
> > > > > > +               __v4l2_ctrl_s_ctrl(imx214->vblank,
> > > > > > +                                  mode->vts_def - mode-
> > > > > > > height);
> > > > > > +
> > > > > > +               /* Update max exposure while meeting
> > > > > > expected
> > > > > > vblanking */
> > > > > > +               exposure_max = mode->vts_def -
> > > > > > IMX214_EXPOSURE_OFFSET;
> > > > > > +               exposure_def = (exposure_max <
> > > > > > IMX214_EXPOSURE_DEFAULT) ?
> > > > > > +                       exposure_max :
> > > > > > IMX214_EXPOSURE_DEFAULT;
> > > > > > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > > > > > +                                        imx214->exposure-
> > > > > > > minimum,
> > > > > > +                                        exposure_max,
> > > > > > imx214-
> > > > > > > exposure->step,
> > > > > > +                                        exposure_def);
> > > > > > +
> > > > > > +               /*
> > > > > > +                * Currently PPL is fixed to
> > > > > > IMX214_PPL_DEFAULT, so hblank
> > > > > > +                * depends on mode->width only, and is not
> > > > > > changeble in any
> > > > > > +                * way other than changing the mode.
> > > > > > +                */
> > > > > > +               hblank = IMX214_PPL_DEFAULT - mode->width;
> > > > > > +               __v4l2_ctrl_modify_range(imx214->hblank,
> > > > > > hblank, hblank, 1,
> > > > > > +                                        hblank);
> > > > > > +       }
> > > > > > +
> > > > > >         return 0;
> > > > > >  }
> > > > > > 
> > > > > > @@ -678,8 +721,25 @@ static int imx214_set_ctrl(struct
> > > > > > v4l2_ctrl *ctrl)
> > > > > >  {
> > > > > >         struct imx214 *imx214 = container_of(ctrl->handler,
> > > > > >                                              struct imx214,
> > > > > > ctrls);
> > > > > > +       const struct v4l2_mbus_framefmt *format;
> > > > > > +       struct v4l2_subdev_state *state;
> > > > > >         int ret;
> > > > > > 
> > > > > > +       state =
> > > > > > v4l2_subdev_get_locked_active_state(&imx214-
> > > > > > > sd);
> > > > > > +       format = v4l2_subdev_state_get_format(state, 0);
> > > > > > +
> > > > > > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > > > > > +               int exposure_max, exposure_def;
> > > > > > +
> > > > > > +               /* Update max exposure while meeting
> > > > > > expected
> > > > > > vblanking */
> > > > > > +               exposure_max = format->height + ctrl->val -
> > > > > > IMX214_EXPOSURE_OFFSET;
> > > > > > +               exposure_def = min(exposure_max,
> > > > > > IMX214_EXPOSURE_DEFAULT);
> > > > > > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > > > > > +                                        imx214->exposure-
> > > > > > > minimum,
> > > > > > +                                        exposure_max,
> > > > > > imx214-
> > > > > > > exposure->step,
> > > > > > +                                        exposure_def);
> > > > > > +       }
> > > > > > +
> > > > > >         /*
> > > > > >          * Applying V4L2 control value only happens
> > > > > >          * when power is up for streaming
> > > > > > @@ -691,7 +751,10 @@ static int imx214_set_ctrl(struct
> > > > > > v4l2_ctrl *ctrl)
> > > > > >         case V4L2_CID_EXPOSURE:
> > > > > >                 cci_write(imx214->regmap,
> > > > > > IMX214_REG_EXPOSURE,
> > > > > > ctrl->val, &ret);
> > > > > >                 break;
> > > > > > -
> > > > > > +       case V4L2_CID_VBLANK:
> > > > > > +               cci_write(imx214->regmap,
> > > > > > IMX214_REG_FRM_LENGTH_LINES,
> > > > > > +                         format->height + ctrl->val,
> > > > > > &ret);
> > > > 
> > > > My datasheet says this register is "set up in multiples of 2".
> > > > (LINE_LENGTH_PCK for HBLANK is "set in multiples of 8")
> > > > 
> > > > I don't have one of these modules, but is that saying only
> > > > multiples
> > > > of 2 are permitted (in which case the step size for the control
> > > > needs
> > > > to reflect that), or that setting a value of N is interpreted
> > > > by
> > > > the
> > > > hardware as 2N?
> > > > Do all the numbers with PIXEL_RATE work out correctly in the
> > > > frame
> > > > rate calcs?
> > > 
> > > Answering my own question, PIXEL_RATE looks dubious.
> > > 
> > > The original code had LINE_LENGTH_PCK as 5008, and
> > > FRAME_LENGTH_LINES as 3194. If enum_frame_intervals is to be
> > > believed, then it delivered 30fps.
> > > 5008 * 3194 * 30 = 479,866,560 Pix/s.
> > > 
> > > The driver defines IMX214_DEFAULT_LINK_FREQ 480000000, and then
> > > IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) /
> > > 10), which works out as 384MPix/s. (Presumably the 8 is 4 lanes
> > > and DDR, but it's not obvious)
> > > 
> > > Parsing the PLL registers with the defined 24MHz input. We're in
> > > single PLL mode, so MIPI frequency is directly linked to pixel
> > > rate.
> > > VTCK ends up being 1200MHz, and VTPXCK and OPPXCK both are
> > > 120MHz.
> > > Section 5.3 "Frame rate calculation formula" says "Pixel rate
> > > [pixels/s] = VTPXCK [MHz] * 4", so 120 * 4 = 480MPix/s, which
> > > basically agrees with my number above.
> > > 
> > > 3.1.4. MIPI global timing setting says "Output bitrate = OPPXCK *
> > > reg  0x113[7:0]", so 120MHz * 10, or 1200Mbit/s. That would be a
> > > link frequency of 600MHz due to DDR.
> > > That also matches to 480MPix/s * 10bpp / 4 lanes / 2 for DDR

Is it okay, if I use the three sections above for the commit message of
the fix-link-frequency commit?

> > > 
> > > Registers 0x0820-0823 are meant to define the target CSI2
> > > bitrate, and are set to 0x12c00000, or 314,572,800 decimal. I
> > > can't get that to square with the above.
> > > 

According the manual, the registers 0x0820-0823 represent an unsigned
32-bit real, where the first 16 bit seem to represent the integer part
and the last 16 bit the fractional part. This means 0x12c00000 is
4800.0 Mbit/s, which is equal to 480MPix/s * 10bpp.

> > > 
> > > So my conclusion is that both PIXEL_RATE and LINK_FREQUENCY are
> > > wrong, however the relationship used to define them looks to be
> > > correct.
> > > Correct IMX214_DEFAULT_LINK_FREQ to 600MHz, and all should be
> > > good.
> > 
> > Will do that. Should it be done in this commit or in a separat one?
> 
> A separate one as it is independent of adding VBLANK and HBLANK
> controls. Ideally it should come before this patch as then hopefully
> everything actually works correctly when you add them.
> 
> > > However you almost certainly want to set IMX214_VBLANK_MIN to
> > > something significantly greater than 4.
> > 
> > Any recommendation for the new IMX214_VBLANK_MIN value?
> 
> As my comment below, the datasheet lists 30fps as the max frame rate
> for full res, therefore IMX214_VBLANK_MIN being correct for that
> seems reasonable. 890 appears to be the value for that, but I haven't
> verified that all the numbers do then work out.
> 
> Out of interest, what platform are you testing this on, and where are
> you getting the sensor from? I'm always happy to tinker with sensors
> that I can get working on a Pi, and I think the Arducam DepthAI Oak
> module should work, but haven't picked one up yet.

The sensor is part of my mobile phone[1], which I use for testing.

By the way, do you (or anyone else) happen to have the documentation
for the Samsung s5k5e2?

André


[1] https://wiki.postmarketos.org/wiki/BQ_Aquaris_M5_(bq-piccolo)

> 
>   Dave
> 
> > Best regards,
> > André
> > 
> > > 
> > >   Dave
> > > 
> > > > Reading the spec sheet that 30fps is the max frame rate at full
> > > > res
> > > > (4096x2304), and the driver was setting a value of 3194 to this
> > > > register, I don't see it being interpreted as 2N. Then again
> > > > having
> > > > VBLANK at 890 seems pretty high.
> > > > 
> > > > > > +               break;
> > > > > >         default:
> > > > > >                 ret = -EINVAL;
> > > > > >         }
> > > > > > @@ -714,8 +777,11 @@ static int imx214_ctrls_init(struct
> > > > > > imx214
> > > > > > *imx214)
> > > > > >                 .width = 1120,
> > > > > >                 .height = 1120,
> > > > > >         };
> > > > > > +       const struct imx214_mode *mode = &imx214_modes[0];
> > > > > >         struct v4l2_fwnode_device_properties props;
> > > > > >         struct v4l2_ctrl_handler *ctrl_hdlr;
> > > > > > +       int exposure_max, exposure_def;
> > > > > > +       int hblank;
> > > > > >         int ret;
> > > > > > 
> > > > > >         ret = v4l2_fwnode_device_parse(imx214->dev,
> > > > > > &props);
> > > > > > @@ -723,7 +789,7 @@ static int imx214_ctrls_init(struct
> > > > > > imx214
> > > > > > *imx214)
> > > > > >                 return ret;
> > > > > > 
> > > > > >         ctrl_hdlr = &imx214->ctrls;
> > > > > > -       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> > > > > > +       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> > > > > >         if (ret)
> > > > > >                 return ret;
> > > > > > 
> > > > > > @@ -739,22 +805,26 @@ static int imx214_ctrls_init(struct
> > > > > > imx214 *imx214)
> > > > > >         if (imx214->link_freq)
> > > > > >                 imx214->link_freq->flags |=
> > > > > > V4L2_CTRL_FLAG_READ_ONLY;
> > > > > > 
> > > > > > -       /*
> > > > > > -        * WARNING!
> > > > > > -        * Values obtained reverse engineering blobs and/or
> > > > > > devices.
> > > > > > -        * Ranges and functionality might be wrong.
> > > > > > -        *
> > > > > > -        * Sony, please release some register set
> > > > > > documentation
> > > > > > for the
> > > > > > -        * device.
> > > > > > -        *
> > > > > > -        * Yours sincerely, Ricardo.
> > > > > > -        */
> > > > > 
> > > > > I would like to keep this comment until there is a public
> > > > > document available.
> > > > 
> > > > I suspect you'll be waiting forever for a document to be
> > > > officially
> > > > released.
> > > > 
> > > > I have a datasheet for IMX214, and I believe Kieran and Jacopo
> > > > do
> > > > too.
> > > > Which specific values do you wish to be verified?
> > > > 
> > > > > 
> > > > > > +       /* Initial vblank/hblank/exposure parameters based
> > > > > > on
> > > > > > current mode */
> > > > > > +       imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > > &imx214_ctrl_ops,
> > > > > > +                                          V4L2_CID_VBLANK,
> > > > > > IMX214_VBLANK_MIN,
> > > > 
> > > > IMX214_VBLANK_MIN being 4 feels plausible, but pretty low.
> > > > I read the datasheet to say there are 4 embedded data lines per
> > > > image.
> > > > Seeing as you have STATS data output enabled as well that makes
> > > > 5
> > > > lines of non-image data per frame, but you're only adding
> > > > blanking
> > > > time for 4 lines.
> > > > 
> > > > As noted above, I think you've also increased the max frame
> > > > rate
> > > > significantly above that quoted by Sony. Has that been actually
> > > > exercised and confirmed to function correctly?
> > > > 
> > > >   Dave
> > > > 
> > > > > > +                                          IMX214_VTS_MAX -
> > > > > > mode->height, 1,
> > > > > > +                                          mode->vts_def -
> > > > > > mode->height);
> > > > > > +
> > > > > > +       hblank = IMX214_PPL_DEFAULT - mode->width;
> > > > > > +       imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > > &imx214_ctrl_ops,
> > > > > > +                                          V4L2_CID_HBLANK,
> > > > > > hblank, hblank,
> > > > > > +                                          1, hblank);
> > > > > > +       if (imx214->hblank)
> > > > > > +               imx214->hblank->flags |=
> > > > > > V4L2_CTRL_FLAG_READ_ONLY;
> > > > > > +
> > > > > > +       exposure_max = mode->vts_def -
> > > > > > IMX214_EXPOSURE_OFFSET;
> > > > > > +       exposure_def = min(exposure_max,
> > > > > > IMX214_EXPOSURE_DEFAULT);
> > > > > >         imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > > &imx214_ctrl_ops,
> > > > > >                                             
> > > > > > V4L2_CID_EXPOSURE,
> > > > > > -
> > > > > > IMX214_EXPOSURE_MIN,
> > > > > > -
> > > > > > IMX214_EXPOSURE_MAX,
> > > > > > +
> > > > > > IMX214_EXPOSURE_MIN, exposure_max,
> > > > > > 
> > > > > > IMX214_EXPOSURE_STEP,
> > > > > > -
> > > > > > IMX214_EXPOSURE_DEFAULT);
> > > > > > +                                            exposure_def);
> > > > > > 
> > > > > >         imx214->unit_size =
> > > > > > v4l2_ctrl_new_std_compound(ctrl_hdlr,
> > > > > >                                 NULL,
> > > > > > 
> > > > > > --
> > > > > > 2.46.0
> > > > > > 
> > > > > > 
> > > > > 
> >

Dave Stevenson Oct. 14, 2024, 12:17 p.m. UTC | #7

Hi André

On Sat, 12 Oct 2024 at 22:46, André Apitzsch <git@apitzsch.eu> wrote:
>
> Hi Dave,
>
> Am Montag, dem 07.10.2024 um 18:29 +0100 schrieb Dave Stevenson:
> > Hi Andre
> >
> > On Sun, 6 Oct 2024 at 22:41, André <git@apitzsch.eu> wrote:
> > >
> > > Hi Dave,
> > >
> > > Am Freitag, dem 13.09.2024 um 18:40 +0100 schrieb Dave Stevenson:
> > > > On Thu, 12 Sept 2024 at 15:51, Dave Stevenson
> > > > <dave.stevenson@raspberrypi.com> wrote:
> > > > >
> > > > > Hi André & Ricardo
> > > > >
> > > > > On Thu, 12 Sept 2024 at 14:41, Ricardo Ribalda Delgado
> > > > > <ribalda@kernel.org> wrote:
> > > > > >
> > > > > > Hi
> > > > > >
> > > > > > Arent you missing some chage in enum_frame_interval?
> > > > >
> > > > > Raw sensors shouldn't be using [enum|set|get]_frame_interval at
> > > > > all
> > > > > https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#frame-interval-configuration
> > > > >
> > > > > The question now is how to handle the backwards compatibility
> > > > > for
> > > > > any userspace app that might be using this driver and expecting
> > > > > to
> > > > > use the frame_interval calls.
> > > > > Seeing as it only ever allowed a fixed value of 30fps, leaving
> > > > > it
> > > > > as is with a comment as to why it is there would be reasonable
> > > > > in
> > > > > my view.
> > >
> > > Should the comment be added in this commit?
> > > And what exactly should the comment say?
> >
> > Add it to imx214_enum_frame_interval.
> > Something along the lines of:
> > Raw sensors should be using the VBLANK and HBLANK controls to
> > determine the frame rate. However this driver was initially added
> > using the [S|G|ENUM]_FRAME_INTERVAL ioctls with a fixed rate of
> > 30fps.
> > Retain the frame_interval ops for backwards compatibility, but they
> > do nothing.
> >
> > > > >
> > > > > > On Mon, Sep 2, 2024 at 11:53 PM André Apitzsch via B4 Relay
> > > > > > <devnull+git.apitzsch.eu@kernel.org> wrote:
> > > > > > >
> > > > > > > From: André Apitzsch <git@apitzsch.eu>
> > > > > > >
> > > > > > > Add vblank control to allow changing the framerate /
> > > > > > > higher exposure values.
> > > > > > >
> > > > > > > The vblank and hblank controls are needed for libcamera
> > > > > > > support.
> > > > > > >
> > > > > > > While at it, fix the minimal exposure time according to the
> > > > > > > datasheet.
> > > > > > >
> > > > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > > > > > ---
> > > > > > >  drivers/media/i2c/imx214.c | 112
> > > > > > > ++++++++++++++++++++++++++++++++++++---------
> > > > > > >  1 file changed, 91 insertions(+), 21 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/imx214.c
> > > > > > > b/drivers/media/i2c/imx214.c
> > > > > > > index 3b422cddbdce..9f5a57aebb86 100644
> > > > > > > --- a/drivers/media/i2c/imx214.c
> > > > > > > +++ b/drivers/media/i2c/imx214.c
> > > > > > > @@ -34,11 +34,18 @@
> > > > > > >
> > > > > > >  /* V-TIMING internal */
> > > > > > >  #define IMX214_REG_FRM_LENGTH_LINES    CCI_REG16(0x0340)
> > > > > > > +#define IMX214_VTS_MAX                 0xffff
> > > > > > > +
> > > > > > > +#define IMX214_VBLANK_MIN              4
> > > > > > > +
> > > > > > > +/* HBLANK control - read only */
> > > > > > > +#define IMX214_PPL_DEFAULT             5008
> > > > > > >
> > > > > > >  /* Exposure control */
> > > > > > >  #define IMX214_REG_EXPOSURE            CCI_REG16(0x0202)
> > > > > > > -#define IMX214_EXPOSURE_MIN            0
> > > > > > > -#define IMX214_EXPOSURE_MAX            3184
> > > > > > > +#define IMX214_EXPOSURE_OFFSET         10
> > > > > > > +#define IMX214_EXPOSURE_MIN            1
> > > > > > > +#define IMX214_EXPOSURE_MAX            (IMX214_VTS_MAX -
> > > > > > > IMX214_EXPOSURE_OFFSET)
> > > > > > >  #define IMX214_EXPOSURE_STEP           1
> > > > > > >  #define IMX214_EXPOSURE_DEFAULT                3184
> > > > > > >  #define IMX214_REG_EXPOSURE_RATIO      CCI_REG8(0x0222)
> > > > > > > @@ -189,6 +196,8 @@ struct imx214 {
> > > > > > >         struct v4l2_ctrl_handler ctrls;
> > > > > > >         struct v4l2_ctrl *pixel_rate;
> > > > > > >         struct v4l2_ctrl *link_freq;
> > > > > > > +       struct v4l2_ctrl *vblank;
> > > > > > > +       struct v4l2_ctrl *hblank;
> > > > > > >         struct v4l2_ctrl *exposure;
> > > > > > >         struct v4l2_ctrl *unit_size;
> > > > > > >
> > > > > > > @@ -205,8 +214,6 @@ static const struct cci_reg_sequence
> > > > > > > mode_4096x2304[] = {
> > > > > > >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > > > > > >         { IMX214_REG_HDR_RES_REDUCTION,
> > > > > > > IMX214_HDR_RES_REDU_THROUGH },
> > > > > > >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > > > > > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > > > > > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > > > > > >         { IMX214_REG_X_ADD_STA, 56 },
> > > > > > >         { IMX214_REG_Y_ADD_STA, 408 },
> > > > > > >         { IMX214_REG_X_ADD_END, 4151 },
> > > > > > > @@ -277,8 +284,6 @@ static const struct cci_reg_sequence
> > > > > > > mode_1920x1080[] = {
> > > > > > >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> > > > > > >         { IMX214_REG_HDR_RES_REDUCTION,
> > > > > > > IMX214_HDR_RES_REDU_THROUGH },
> > > > > > >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > > > > > > -       { IMX214_REG_FRM_LENGTH_LINES, 3194 },
> > > > > > > -       { IMX214_REG_LINE_LENGTH_PCK, 5008 },
> > > > > > >         { IMX214_REG_X_ADD_STA, 1144 },
> > > > > > >         { IMX214_REG_Y_ADD_STA, 1020 },
> > > > > > >         { IMX214_REG_X_ADD_END, 3063 },
> > > > > > > @@ -362,6 +367,7 @@ static const struct cci_reg_sequence
> > > > > > > mode_table_common[] = {
> > > > > > >         { IMX214_REG_ORIENTATION, 0 },
> > > > > > >         { IMX214_REG_MASK_CORR_FRAMES,
> > > > > > > IMX214_CORR_FRAMES_MASK
> > > > > > > },
> > > > > > >         { IMX214_REG_FAST_STANDBY_CTRL, 1 },
> > > > > > > +       { IMX214_REG_LINE_LENGTH_PCK, IMX214_PPL_DEFAULT },
> > > > > > >         { CCI_REG8(0x4550), 0x02 },
> > > > > > >         { CCI_REG8(0x4601), 0x00 },
> > > > > > >         { CCI_REG8(0x4642), 0x05 },
> > > > > > > @@ -465,18 +471,24 @@ static const struct cci_reg_sequence
> > > > > > > mode_table_common[] = {
> > > > > > >  static const struct imx214_mode {
> > > > > > >         u32 width;
> > > > > > >         u32 height;
> > > > > > > +
> > > > > > > +       /* V-timing */
> > > > > > > +       unsigned int vts_def;
> > > > > > > +
> > > > > > >         unsigned int num_of_regs;
> > > > > > >         const struct cci_reg_sequence *reg_table;
> > > > > > >  } imx214_modes[] = {
> > > > > > >         {
> > > > > > >                 .width = 4096,
> > > > > > >                 .height = 2304,
> > > > > > > +               .vts_def = 3194,
> > > > > > >                 .num_of_regs = ARRAY_SIZE(mode_4096x2304),
> > > > > > >                 .reg_table = mode_4096x2304,
> > > > > > >         },
> > > > > > >         {
> > > > > > >                 .width = 1920,
> > > > > > >                 .height = 1080,
> > > > > > > +               .vts_def = 3194,
> > > > > > >                 .num_of_regs = ARRAY_SIZE(mode_1920x1080),
> > > > > > >                 .reg_table = mode_1920x1080,
> > > > > > >         },
> > > > > > > @@ -629,6 +641,37 @@ static int imx214_set_format(struct
> > > > > > > v4l2_subdev *sd,
> > > > > > >         __crop->width = mode->width;
> > > > > > >         __crop->height = mode->height;
> > > > > > >
> > > > > > > +       if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > > > > > +               int exposure_max;
> > > > > > > +               int exposure_def;
> > > > > > > +               int hblank;
> > > > > > > +
> > > > > > > +               /* Update limits and set FPS to default */
> > > > > > > +               __v4l2_ctrl_modify_range(imx214->vblank,
> > > > > > > IMX214_VBLANK_MIN,
> > > > > > > +                                        IMX214_VTS_MAX -
> > > > > > > mode-
> > > > > > > > height, 1,
> > > > > > > +                                        mode->vts_def -
> > > > > > > mode-
> > > > > > > > height);
> > > > > > > +               __v4l2_ctrl_s_ctrl(imx214->vblank,
> > > > > > > +                                  mode->vts_def - mode-
> > > > > > > > height);
> > > > > > > +
> > > > > > > +               /* Update max exposure while meeting
> > > > > > > expected
> > > > > > > vblanking */
> > > > > > > +               exposure_max = mode->vts_def -
> > > > > > > IMX214_EXPOSURE_OFFSET;
> > > > > > > +               exposure_def = (exposure_max <
> > > > > > > IMX214_EXPOSURE_DEFAULT) ?
> > > > > > > +                       exposure_max :
> > > > > > > IMX214_EXPOSURE_DEFAULT;
> > > > > > > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > > > > > > +                                        imx214->exposure-
> > > > > > > > minimum,
> > > > > > > +                                        exposure_max,
> > > > > > > imx214-
> > > > > > > > exposure->step,
> > > > > > > +                                        exposure_def);
> > > > > > > +
> > > > > > > +               /*
> > > > > > > +                * Currently PPL is fixed to
> > > > > > > IMX214_PPL_DEFAULT, so hblank
> > > > > > > +                * depends on mode->width only, and is not
> > > > > > > changeble in any
> > > > > > > +                * way other than changing the mode.
> > > > > > > +                */
> > > > > > > +               hblank = IMX214_PPL_DEFAULT - mode->width;
> > > > > > > +               __v4l2_ctrl_modify_range(imx214->hblank,
> > > > > > > hblank, hblank, 1,
> > > > > > > +                                        hblank);
> > > > > > > +       }
> > > > > > > +
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -678,8 +721,25 @@ static int imx214_set_ctrl(struct
> > > > > > > v4l2_ctrl *ctrl)
> > > > > > >  {
> > > > > > >         struct imx214 *imx214 = container_of(ctrl->handler,
> > > > > > >                                              struct imx214,
> > > > > > > ctrls);
> > > > > > > +       const struct v4l2_mbus_framefmt *format;
> > > > > > > +       struct v4l2_subdev_state *state;
> > > > > > >         int ret;
> > > > > > >
> > > > > > > +       state =
> > > > > > > v4l2_subdev_get_locked_active_state(&imx214-
> > > > > > > > sd);
> > > > > > > +       format = v4l2_subdev_state_get_format(state, 0);
> > > > > > > +
> > > > > > > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > > > > > > +               int exposure_max, exposure_def;
> > > > > > > +
> > > > > > > +               /* Update max exposure while meeting
> > > > > > > expected
> > > > > > > vblanking */
> > > > > > > +               exposure_max = format->height + ctrl->val -
> > > > > > > IMX214_EXPOSURE_OFFSET;
> > > > > > > +               exposure_def = min(exposure_max,
> > > > > > > IMX214_EXPOSURE_DEFAULT);
> > > > > > > +               __v4l2_ctrl_modify_range(imx214->exposure,
> > > > > > > +                                        imx214->exposure-
> > > > > > > > minimum,
> > > > > > > +                                        exposure_max,
> > > > > > > imx214-
> > > > > > > > exposure->step,
> > > > > > > +                                        exposure_def);
> > > > > > > +       }
> > > > > > > +
> > > > > > >         /*
> > > > > > >          * Applying V4L2 control value only happens
> > > > > > >          * when power is up for streaming
> > > > > > > @@ -691,7 +751,10 @@ static int imx214_set_ctrl(struct
> > > > > > > v4l2_ctrl *ctrl)
> > > > > > >         case V4L2_CID_EXPOSURE:
> > > > > > >                 cci_write(imx214->regmap,
> > > > > > > IMX214_REG_EXPOSURE,
> > > > > > > ctrl->val, &ret);
> > > > > > >                 break;
> > > > > > > -
> > > > > > > +       case V4L2_CID_VBLANK:
> > > > > > > +               cci_write(imx214->regmap,
> > > > > > > IMX214_REG_FRM_LENGTH_LINES,
> > > > > > > +                         format->height + ctrl->val,
> > > > > > > &ret);
> > > > >
> > > > > My datasheet says this register is "set up in multiples of 2".
> > > > > (LINE_LENGTH_PCK for HBLANK is "set in multiples of 8")
> > > > >
> > > > > I don't have one of these modules, but is that saying only
> > > > > multiples
> > > > > of 2 are permitted (in which case the step size for the control
> > > > > needs
> > > > > to reflect that), or that setting a value of N is interpreted
> > > > > by
> > > > > the
> > > > > hardware as 2N?
> > > > > Do all the numbers with PIXEL_RATE work out correctly in the
> > > > > frame
> > > > > rate calcs?
> > > >
> > > > Answering my own question, PIXEL_RATE looks dubious.
> > > >
> > > > The original code had LINE_LENGTH_PCK as 5008, and
> > > > FRAME_LENGTH_LINES as 3194. If enum_frame_intervals is to be
> > > > believed, then it delivered 30fps.
> > > > 5008 * 3194 * 30 = 479,866,560 Pix/s.
> > > >
> > > > The driver defines IMX214_DEFAULT_LINK_FREQ 480000000, and then
> > > > IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) /
> > > > 10), which works out as 384MPix/s. (Presumably the 8 is 4 lanes
> > > > and DDR, but it's not obvious)
> > > >
> > > > Parsing the PLL registers with the defined 24MHz input. We're in
> > > > single PLL mode, so MIPI frequency is directly linked to pixel
> > > > rate.
> > > > VTCK ends up being 1200MHz, and VTPXCK and OPPXCK both are
> > > > 120MHz.
> > > > Section 5.3 "Frame rate calculation formula" says "Pixel rate
> > > > [pixels/s] = VTPXCK [MHz] * 4", so 120 * 4 = 480MPix/s, which
> > > > basically agrees with my number above.
> > > >
> > > > 3.1.4. MIPI global timing setting says "Output bitrate = OPPXCK *
> > > > reg  0x113[7:0]", so 120MHz * 10, or 1200Mbit/s. That would be a
> > > > link frequency of 600MHz due to DDR.
> > > > That also matches to 480MPix/s * 10bpp / 4 lanes / 2 for DDR
>
> Is it okay, if I use the three sections above for the commit message of
> the fix-link-frequency commit?

Fine by me.

> > > >
> > > > Registers 0x0820-0823 are meant to define the target CSI2
> > > > bitrate, and are set to 0x12c00000, or 314,572,800 decimal. I
> > > > can't get that to square with the above.
> > > >
>
> According the manual, the registers 0x0820-0823 represent an unsigned
> 32-bit real, where the first 16 bit seem to represent the integer part
> and the last 16 bit the fractional part. This means 0x12c00000 is
> 4800.0 Mbit/s, which is equal to 480MPix/s * 10bpp.

My datasheet doesn't appear to have the clarification, just that it is
"Target frequency (32-bit unsigned Real)".
Your numbers work out perfectly though, which is always a good thing.

> > > >
> > > > So my conclusion is that both PIXEL_RATE and LINK_FREQUENCY are
> > > > wrong, however the relationship used to define them looks to be
> > > > correct.
> > > > Correct IMX214_DEFAULT_LINK_FREQ to 600MHz, and all should be
> > > > good.
> > >
> > > Will do that. Should it be done in this commit or in a separat one?
> >
> > A separate one as it is independent of adding VBLANK and HBLANK
> > controls. Ideally it should come before this patch as then hopefully
> > everything actually works correctly when you add them.
> >
> > > > However you almost certainly want to set IMX214_VBLANK_MIN to
> > > > something significantly greater than 4.
> > >
> > > Any recommendation for the new IMX214_VBLANK_MIN value?
> >
> > As my comment below, the datasheet lists 30fps as the max frame rate
> > for full res, therefore IMX214_VBLANK_MIN being correct for that
> > seems reasonable. 890 appears to be the value for that, but I haven't
> > verified that all the numbers do then work out.
> >
> > Out of interest, what platform are you testing this on, and where are
> > you getting the sensor from? I'm always happy to tinker with sensors
> > that I can get working on a Pi, and I think the Arducam DepthAI Oak
> > module should work, but haven't picked one up yet.
>
> The sensor is part of my mobile phone[1], which I use for testing.

Fair enough.

> By the way, do you (or anyone else) happen to have the documentation
> for the Samsung s5k5e2?

No, I rarely see any information on Samsung sensors these days.
I may be totally misremembering, but I seem to recall the ones I did
deal with 10 years ago followed the CCS spec, so it might be worth
dumping registers to see if they do follow.

  Dave

> André
>
>
> [1] https://wiki.postmarketos.org/wiki/BQ_Aquaris_M5_(bq-piccolo)
>
> >
> >   Dave
> >
> > > Best regards,
> > > André
> > >
> > > >
> > > >   Dave
> > > >
> > > > > Reading the spec sheet that 30fps is the max frame rate at full
> > > > > res
> > > > > (4096x2304), and the driver was setting a value of 3194 to this
> > > > > register, I don't see it being interpreted as 2N. Then again
> > > > > having
> > > > > VBLANK at 890 seems pretty high.
> > > > >
> > > > > > > +               break;
> > > > > > >         default:
> > > > > > >                 ret = -EINVAL;
> > > > > > >         }
> > > > > > > @@ -714,8 +777,11 @@ static int imx214_ctrls_init(struct
> > > > > > > imx214
> > > > > > > *imx214)
> > > > > > >                 .width = 1120,
> > > > > > >                 .height = 1120,
> > > > > > >         };
> > > > > > > +       const struct imx214_mode *mode = &imx214_modes[0];
> > > > > > >         struct v4l2_fwnode_device_properties props;
> > > > > > >         struct v4l2_ctrl_handler *ctrl_hdlr;
> > > > > > > +       int exposure_max, exposure_def;
> > > > > > > +       int hblank;
> > > > > > >         int ret;
> > > > > > >
> > > > > > >         ret = v4l2_fwnode_device_parse(imx214->dev,
> > > > > > > &props);
> > > > > > > @@ -723,7 +789,7 @@ static int imx214_ctrls_init(struct
> > > > > > > imx214
> > > > > > > *imx214)
> > > > > > >                 return ret;
> > > > > > >
> > > > > > >         ctrl_hdlr = &imx214->ctrls;
> > > > > > > -       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 6);
> > > > > > > +       ret = v4l2_ctrl_handler_init(&imx214->ctrls, 8);
> > > > > > >         if (ret)
> > > > > > >                 return ret;
> > > > > > >
> > > > > > > @@ -739,22 +805,26 @@ static int imx214_ctrls_init(struct
> > > > > > > imx214 *imx214)
> > > > > > >         if (imx214->link_freq)
> > > > > > >                 imx214->link_freq->flags |=
> > > > > > > V4L2_CTRL_FLAG_READ_ONLY;
> > > > > > >
> > > > > > > -       /*
> > > > > > > -        * WARNING!
> > > > > > > -        * Values obtained reverse engineering blobs and/or
> > > > > > > devices.
> > > > > > > -        * Ranges and functionality might be wrong.
> > > > > > > -        *
> > > > > > > -        * Sony, please release some register set
> > > > > > > documentation
> > > > > > > for the
> > > > > > > -        * device.
> > > > > > > -        *
> > > > > > > -        * Yours sincerely, Ricardo.
> > > > > > > -        */
> > > > > >
> > > > > > I would like to keep this comment until there is a public
> > > > > > document available.
> > > > >
> > > > > I suspect you'll be waiting forever for a document to be
> > > > > officially
> > > > > released.
> > > > >
> > > > > I have a datasheet for IMX214, and I believe Kieran and Jacopo
> > > > > do
> > > > > too.
> > > > > Which specific values do you wish to be verified?
> > > > >
> > > > > >
> > > > > > > +       /* Initial vblank/hblank/exposure parameters based
> > > > > > > on
> > > > > > > current mode */
> > > > > > > +       imx214->vblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > > > &imx214_ctrl_ops,
> > > > > > > +                                          V4L2_CID_VBLANK,
> > > > > > > IMX214_VBLANK_MIN,
> > > > >
> > > > > IMX214_VBLANK_MIN being 4 feels plausible, but pretty low.
> > > > > I read the datasheet to say there are 4 embedded data lines per
> > > > > image.
> > > > > Seeing as you have STATS data output enabled as well that makes
> > > > > 5
> > > > > lines of non-image data per frame, but you're only adding
> > > > > blanking
> > > > > time for 4 lines.
> > > > >
> > > > > As noted above, I think you've also increased the max frame
> > > > > rate
> > > > > significantly above that quoted by Sony. Has that been actually
> > > > > exercised and confirmed to function correctly?
> > > > >
> > > > >   Dave
> > > > >
> > > > > > > +                                          IMX214_VTS_MAX -
> > > > > > > mode->height, 1,
> > > > > > > +                                          mode->vts_def -
> > > > > > > mode->height);
> > > > > > > +
> > > > > > > +       hblank = IMX214_PPL_DEFAULT - mode->width;
> > > > > > > +       imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > > > &imx214_ctrl_ops,
> > > > > > > +                                          V4L2_CID_HBLANK,
> > > > > > > hblank, hblank,
> > > > > > > +                                          1, hblank);
> > > > > > > +       if (imx214->hblank)
> > > > > > > +               imx214->hblank->flags |=
> > > > > > > V4L2_CTRL_FLAG_READ_ONLY;
> > > > > > > +
> > > > > > > +       exposure_max = mode->vts_def -
> > > > > > > IMX214_EXPOSURE_OFFSET;
> > > > > > > +       exposure_def = min(exposure_max,
> > > > > > > IMX214_EXPOSURE_DEFAULT);
> > > > > > >         imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > > > &imx214_ctrl_ops,
> > > > > > >
> > > > > > > V4L2_CID_EXPOSURE,
> > > > > > > -
> > > > > > > IMX214_EXPOSURE_MIN,
> > > > > > > -
> > > > > > > IMX214_EXPOSURE_MAX,
> > > > > > > +
> > > > > > > IMX214_EXPOSURE_MIN, exposure_max,
> > > > > > >
> > > > > > > IMX214_EXPOSURE_STEP,
> > > > > > > -
> > > > > > > IMX214_EXPOSURE_DEFAULT);
> > > > > > > +                                            exposure_def);
> > > > > > >
> > > > > > >         imx214->unit_size =
> > > > > > > v4l2_ctrl_new_std_compound(ctrl_hdlr,
> > > > > > >                                 NULL,
> > > > > > >
> > > > > > > --
> > > > > > > 2.46.0
> > > > > > >
> > > > > > >
> > > > > >
> > >
>

[08/13] media: i2c: imx214: Add vblank and hblank controls

Commit Message

Comments

Patch