| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPY8ntC6LPOM_RQSvSbButsh59S8t8ic16jAgNN3q7w9SycpBw@mail.gmail.com>
Date: Mon, 7 Oct 2024 18:29:52 +0100
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: André <git@...tzsch.eu>
Cc: Ricardo Ribalda Delgado <ribalda@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, ~postmarketos/upstreaming@...ts.sr.ht,
phone-devel@...r.kernel.org, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 08/13] media: i2c: imx214: Add vblank and hblank controls
Hi Andre
On Sun, 6 Oct 2024 at 22:41, André <git@...tzsch.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@...pberrypi.com> wrote:
> > >
> > > Hi André & Ricardo
> > >
> > > On Thu, 12 Sept 2024 at 14:41, Ricardo Ribalda Delgado
> > > <ribalda@...nel.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@...nel.org> wrote:
> > > > >
> > > > > From: André Apitzsch <git@...tzsch.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@...tzsch.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
> > > > >
> > > > >
> > > >
>
Powered by blists - more mailing lists