| 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: <175550390975.1721288.3121861926484209664@ping.linuxembedded.co.uk>
Date: Mon, 18 Aug 2025 08:58:29 +0100
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, Will Whang <will@...lwhang.com>
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
Hi Will,
Thanks for working on getting this upstream! I'm always happy to see
more cameras supported for all platforms!
Please post the IMX585 camera sensor helpers to libcamera too - they're
eligible for merge when a driver is posted publicly to the kernel
mailing list.
I'm only tackling the black level in this review at the moment as I
noticied it so trimming out other parts for the review:
Quoting Will Whang (2025-08-16 06:54:32)
> Implements support for:
> * 4-lane / 2-lane CSI-2
> * 12-bit linear, 4K/FHD mode.
> * Monochrome variant.
> * Tested on Raspberry Pi 4/5 with 24 MHz clock.
>
> Signed-off-by: Will Whang <will@...lwhang.com>
> ---
<snip>
> +
> +/* Black level control */
> +#define IMX585_REG_BLKLEVEL CCI_REG16_LE(0x30dc)
> +#define IMX585_BLKLEVEL_DEFAULT 50
50 ... seems surprisingly low to me ... Is that a 10 bit value or a 12
bit value ? You only have 12 bit modes ...
Oh - fortunately I can see a datasheet on this one.
"""
When the output data length is 10-bit output, increasing the register
setting value by 1h increases the black level by 1 LSB. When the output
data length is 12-bit output, increasing the register setting value by
1h increases the black level by 4 LSB.
Use with values shown below is recommended.
10-bit output: 032h (50d in LSB units)
12-bit output: 032h (200d in LSB units)
"""
Ok - so the value written is always a 10 bit number regardless of the
mode, which gives 3200 as a 16 bit value which is more in the range I
would have expected, so now it makes sense.
> +
> +/* Digital Clamp */
> +#define IMX585_REG_DIGITAL_CLAMP CCI_REG8(0x3458)
> +
<snip>
> +/* --------------------------------------------------------------------------
> + * Controls
> + * --------------------------------------------------------------------------
> + */
> +
> +static int imx585_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler);
> + const struct imx585_mode *mode, *mode_list;
> + struct v4l2_subdev_state *state;
> + struct v4l2_mbus_framefmt *fmt;
> + unsigned int num_modes;
> + int ret = 0;
> +
> + state = v4l2_subdev_get_locked_active_state(&imx585->sd);
> + fmt = v4l2_subdev_state_get_format(state, 0);
> +
> + get_mode_table(imx585, fmt->code, &mode_list, &num_modes);
> + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
> + fmt->width, fmt->height);
> +
> + /* Apply control only when powered (runtime active). */
> + if (!pm_runtime_get_if_active(imx585->clientdev))
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_EXPOSURE: {
> + u32 shr = (imx585->vmax - ctrl->val) & ~1U; /* SHR always a multiple of 2 */
> +
> + dev_dbg(imx585->clientdev, "EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n",
> + ctrl->val, shr, imx585->vmax, imx585->hmax);
> +
> + ret = cci_write(imx585->regmap, IMX585_REG_SHR, shr, NULL);
> + if (ret)
> + dev_err_ratelimited(imx585->clientdev, "SHR write failed (%d)\n", ret);
> + break;
> + }
> + case V4L2_CID_ANALOGUE_GAIN:
> + dev_dbg(imx585->clientdev, "ANALOG_GAIN=%u\n", ctrl->val);
> + ret = cci_write(imx585->regmap, IMX585_REG_ANALOG_GAIN, ctrl->val, NULL);
> + if (ret)
> + dev_err_ratelimited(imx585->clientdev, "Gain write failed (%d)\n", ret);
> + break;
> + case V4L2_CID_VBLANK: {
> + u32 current_exposure = imx585->exposure->cur.val;
> +
> + imx585->vmax = (mode->height + ctrl->val) & ~1U;
> +
> + current_exposure = clamp_t(u32, current_exposure,
> + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN);
> + __v4l2_ctrl_modify_range(imx585->exposure,
> + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN, 1,
> + current_exposure);
> +
> + dev_dbg(imx585->clientdev, "VBLANK=%u -> VMAX=%u\n", ctrl->val, imx585->vmax);
> +
> + ret = cci_write(imx585->regmap, IMX585_REG_VMAX, imx585->vmax, NULL);
> + if (ret)
> + dev_err_ratelimited(imx585->clientdev, "VMAX write failed (%d)\n", ret);
> + break;
> + }
> + case V4L2_CID_HBLANK: {
> + u64 pixel_rate = (u64)mode->width * IMX585_PIXEL_RATE;
> + u64 hmax;
> +
> + do_div(pixel_rate, mode->min_hmax);
> + hmax = (u64)(mode->width + ctrl->val) * IMX585_PIXEL_RATE;
> + do_div(hmax, pixel_rate);
> + imx585->hmax = (u32)hmax;
> +
> + dev_dbg(imx585->clientdev, "HBLANK=%u -> HMAX=%u\n", ctrl->val, imx585->hmax);
> +
> + ret = cci_write(imx585->regmap, IMX585_REG_HMAX, imx585->hmax, NULL);
> + if (ret)
> + dev_err_ratelimited(imx585->clientdev, "HMAX write failed (%d)\n", ret);
> + break;
> + }
> + case V4L2_CID_HFLIP:
> + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEH, ctrl->val, NULL);
> + if (ret)
> + dev_err_ratelimited(imx585->clientdev, "HFLIP write failed (%d)\n", ret);
> + break;
> + case V4L2_CID_VFLIP:
> + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEV, ctrl->val, NULL);
> + if (ret)
> + dev_err_ratelimited(imx585->clientdev, "VFLIP write failed (%d)\n", ret);
> + break;
> + case V4L2_CID_BRIGHTNESS: {
This is the wrong control to set blacklevel.
Please use V4L2_CID_BLACK_LEVEL
> + u16 blacklevel = min_t(u32, ctrl->val, 4095);
do you know if the value is specific to which mode the sensor is in ?
I assume this is a 10 bit value if the sensor is in a 10 bit mode and a
12 bit mode if the sensor is in 12 bit mode for example.
Edit: Nope - now I've found a datasheet - this is /always/ a 10 bit
value. Can you document that with a comment please? Especially as this
driver only currently outputs 12 bit modes but uses a 10 bit black level
that could be confusing otherwise.
If we want to make that adjustable from libcamera we'll have to be
careful to always use the correct units.
> +
> + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, blacklevel, NULL);
> + if (ret)
> + dev_err_ratelimited(imx585->clientdev, "BLKLEVEL write failed (%d)\n", ret);
> + break;
> + }
> + default:
> + dev_dbg(imx585->clientdev, "Unhandled ctrl %s: id=0x%x, val=0x%x\n",
> + ctrl->name, ctrl->id, ctrl->val);
> + break;
> + }
> +
> + pm_runtime_put(imx585->clientdev);
> + return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops imx585_ctrl_ops = {
> + .s_ctrl = imx585_set_ctrl,
> +};
> +
> +static int imx585_init_controls(struct imx585 *imx585)
> +{
> + struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler;
> + struct v4l2_fwnode_device_properties props;
> + int ret;
> +
> + ret = v4l2_ctrl_handler_init(hdl, 16);
> +
> + /* Read-only, updated per mode */
> + imx585->pixel_rate = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
> + V4L2_CID_PIXEL_RATE,
> + 1, UINT_MAX, 1, 1);
> +
> + imx585->link_freq =
> + v4l2_ctrl_new_int_menu(hdl, &imx585_ctrl_ops, V4L2_CID_LINK_FREQ,
> + 0, 0, &link_freqs[imx585->link_freq_idx]);
> + if (imx585->link_freq)
> + imx585->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + imx585->vblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
> + V4L2_CID_VBLANK, 0, 0xFFFFF, 1, 0);
> + imx585->hblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
> + V4L2_CID_HBLANK, 0, 0xFFFF, 1, 0);
> + imx585->blacklevel = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
> + V4L2_CID_BRIGHTNESS, 0, 0xFFFF, 1,
You're setting max to 0xFFFF but in the code you clamp it to 4095
(0xFFF)?
I'd use readable decimal values for the black level range rather than
hex, as I think of black level in decimal values.
If you set this to
imx585->blacklevel = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
V4L2_CID_BLACK_LEVEL, 0, 4095, 1,
IMX585_BLKLEVEL_DEFAULT);
you can remove the clamp from the set controls call as v4l2 will
restrict the values.
Oh ... I just looked at the datasheet again:
Setting value
Initial value 032h
Setting value 000h to 3FFh
So the actual maximum is 1023. Maybe hex (0x3ff) or decimal is fine
either way depending on if you want to closely match the datasheet.
> + IMX585_BLKLEVEL_DEFAULT);
> +
> + imx585->exposure = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
> + V4L2_CID_EXPOSURE,
> + IMX585_EXPOSURE_MIN, IMX585_EXPOSURE_MAX,
> + IMX585_EXPOSURE_STEP, IMX585_EXPOSURE_DEFAULT);
> +
> + imx585->gain = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> + IMX585_ANA_GAIN_MIN, IMX585_ANA_GAIN_MAX,
> + IMX585_ANA_GAIN_STEP, IMX585_ANA_GAIN_DEFAULT);
> +
> + imx585->hflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> + imx585->vflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> +
> + if (hdl->error) {
> + ret = hdl->error;
> + dev_err(imx585->clientdev, "control init failed (%d)\n", ret);
> + goto err_free;
> + }
> +
> + ret = v4l2_fwnode_device_parse(imx585->clientdev, &props);
> + if (ret)
> + goto err_free;
> +
> + ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx585_ctrl_ops, &props);
> + if (ret)
> + goto err_free;
> +
> + imx585->sd.ctrl_handler = hdl;
> + return 0;
> +
> +err_free:
> + v4l2_ctrl_handler_free(hdl);
> + return ret;
> +}
> +
> +static void imx585_free_controls(struct imx585 *imx585)
> +{
> + v4l2_ctrl_handler_free(imx585->sd.ctrl_handler);
> +}
> +};
<snip>
Powered by blists - more mailing lists