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

From: Dave Stevenson
Date: Fri Sep 13 2024 - 13:41:21 EST

Next message: Jim Mattson: "Re: [PATCH v11 20/20] x86/cpu/amd: Do not print FW_BUG for Secure TSC"
Previous message: Yosry Ahmed: "Re: [regression] oops on heavy compilations ("kernel BUG at mm/zswap.c:1005!" and "Oops: invalid opcode: 0000")"
In reply to: Dave Stevenson: "Re: [PATCH 08/13] media: i2c: imx214: Add vblank and hblank controls"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 12 Sept 2024 at 15:51, Dave Stevenson
<dave.stevenson@xxxxxxxxxxxxxxx> wrote:
>
> Hi André & Ricardo
>
> On Thu, 12 Sept 2024 at 14:41, Ricardo Ribalda Delgado
> <ribalda@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > >
> > > From: André Apitzsch <git@xxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxx>
> > > ---
> > > 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
> > >
> > >
> >

Next message: Jim Mattson: "Re: [PATCH v11 20/20] x86/cpu/amd: Do not print FW_BUG for Secure TSC"
Previous message: Yosry Ahmed: "Re: [regression] oops on heavy compilations ("kernel BUG at mm/zswap.c:1005!" and "Oops: invalid opcode: 0000")"
In reply to: Dave Stevenson: "Re: [PATCH 08/13] media: i2c: imx214: Add vblank and hblank controls"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]