| 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: <CAFoNnrxonvqzszYKEh5Sh3xx1dnMrqYA==378vbFLLniSBjQ+w@mail.gmail.com>
Date: Tue, 12 Aug 2025 21:20:24 -0700
From: Will Whang <will@...lwhang.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>,
Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, imx@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 4/4] media: docs: Add userspace-API guide for the
IMX585 driver
Hi Laurent,
Reply inline.
Thanks,
Will Whang
On Tue, Aug 12, 2025 at 4:28 AM Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Will,
>
> On Mon, Aug 11, 2025 at 07:31:13PM -0700, Will Whang wrote:
> > On Mon, Aug 11, 2025 at 7:24 AM Dave Stevenson wrote:
> > > On Sun, 10 Aug 2025 at 23:11, Will Whang wrote:
> > > >
> > > > The new IMX585 V4L2 sub-device driver introduces several
> > > > driver-specific controls for configuring Clear-HDR blending,
> > > > gradation compression thresholds, and HCG enabling. This patch adds
> > > > an rst document under Documentation/userspace-api/media/drivers/
> > > > that details each control, allowed values, and their effects.
> > > >
> > > > Signed-off-by: Will Whang <will@...lwhang.com>
> > > > ---
> > > > .../userspace-api/media/drivers/imx585.rst | 122 ++++++++++++++++++
> > > > .../userspace-api/media/drivers/index.rst | 1 +
> > > > MAINTAINERS | 1 +
> > > > 3 files changed, 124 insertions(+)
> > > > create mode 100644 Documentation/userspace-api/media/drivers/imx585.rst
> > > >
> > > > diff --git a/Documentation/userspace-api/media/drivers/imx585.rst b/Documentation/userspace-api/media/drivers/imx585.rst
> > > > new file mode 100644
> > > > index 000000000..9f7c16f30
> > > > --- /dev/null
> > > > +++ b/Documentation/userspace-api/media/drivers/imx585.rst
> > > > @@ -0,0 +1,122 @@
> > > > +.. SPDX-License-Identifier: GPL-2.0-only
> > > > +
> > > > +Sony IMX585 driver
> > > > +==================
> > > > +
> > > > +The IMX585 image-sensor driver provides the following *driver-specific*
> > > > +V4L2 controls. They are visible only when the IMX585 driver is loaded
> > > > +and sit in the sensor-private control class.
> > > > +
> > > > +HDR data blending
> > > > +-----------------
> > > > +
> > > > +``V4L2_CID_IMX585_HDR_DATASEL_TH`` (``U16[2]``)
> > > > + Lower/upper **thresholds** (0 – 4095) that decide which exposure is
> > > > + chosen—or blended—for each pixel in Clear-HDR mode.
> > > > +
> > > > +``V4L2_CID_IMX585_HDR_DATASEL_BK`` (menu)
> > > > + **Blending ratio** between the long-gain (LG) and
> > > > + high-gain (HG) read-outs.
> > > > +
> > > > + .. flat-table::
> > > > + :stub-columns: 0
> > > > + :widths: 1 5
> > > > +
> > > > + * - ``0``
> > > > + - HG ½, LG ½
> > > > + * - ``1``
> > > > + - HG ¾, LG ¼
> > > > + * - ``2`` # duplicate ratio present in the datasheet
> > > > + - HG ½, LG ½
> > > > + * - ``3``
> > > > + - HG ⅞, LG ⅛
> > > > + * - ``4``
> > > > + - HG 15⁄16, LG 1⁄16
> > > > + * - ``5`` # second 50/50 entry as documented
> > > > + - **2ⁿᵈ** HG ½, LG ½
> > > > + * - ``6``
> > > > + - HG 1⁄16, LG 15⁄16
> > > > + * - ``7``
> > > > + - HG ⅛, LG ⅞
> > > > + * - ``8``
> > > > + - HG ¼, LG ¾
> > > > +
> > > > +Gradation compression
> > > > +---------------------
> > > > +
> > > > +``V4L2_CID_IMX585_HDR_GRAD_TH`` (``U32[2]``)
> > > > + 17-bit **break-points** (0 – 0x1ffff) that shape the 16-bit
> > > > + gradation-compression curve.
> > > > +
> > > > +``V4L2_CID_IMX585_HDR_GRAD_COMP_L`` (menu)
> > > > + See V4L2_CID_IMX585_HDR_GRAD_COMP_H
> > > > +
> > > > +``V4L2_CID_IMX585_HDR_GRAD_COMP_H`` (menu)
> > > > + **Compression ratios** below the first break-point and between the
> > > > + two break-points, respectively.
> > > > +
> > > > + .. flat-table::
> > > > + :stub-columns: 0
> > > > + :widths: 1 4
> > > > +
> > > > + * - ``0``
> > > > + - 1 : 1
> > > > + * - ``1``
> > > > + - 1 : 2
> > > > + * - ``2``
> > > > + - 1 : 4 *(default for COMP_L)*
> > > > + * - ``3``
> > > > + - 1 : 8
> > > > + * - ``4``
> > > > + - 1 : 16
> > > > + * - ``5``
> > > > + - 1 : 32
> > > > + * - ``6``
> > > > + - 1 : 64 *(default for COMP_H)*
> > > > + * - ``7``
> > > > + - 1 : 128
> > > > + * - ``8``
> > > > + - 1 : 256
> > > > + * - ``9``
> > > > + - 1 : 512
> > > > + * - ``10``
> > > > + - 1 : 1024
> > > > + * - ``11``
> > > > + - 1 : 2048
> > > > +
> > > > +Gain settings
> > > > +-------------
> > > > +
> > > > +``V4L2_CID_IMX585_HDR_GAIN`` (menu)
> > > > + **Additional gain** (in dB) applied to the high-gain path when
> > > > + Clear-HDR is active.
> > > > +
> > > > + .. flat-table::
> > > > + :stub-columns: 0
> > > > + :widths: 1 3
> > > > +
> > > > + * - ``0``
> > > > + - +0 dB
> > > > + * - ``1``
> > > > + - +6 dB
> > > > + * - ``2``
> > > > + - +12 dB *(default)*
> > > > + * - ``3``
> > > > + - +18 dB
> > > > + * - ``4``
> > > > + - +24 dB
> > > > + * - ``5``
> > > > + - +29.1 dB
> > > > +
> > > > +``V4L2_CID_IMX585_HCG_GAIN`` (boolean)
> > >
> > > HCG stands for High Conversion Gain, so we've got Gain repeated in the name.
> > >
> > > Spell it out as V4L2_CID_IMX585_HIGH_CONV_GAIN, or call it
> > > CONVERSION_GAIN and use an enum control?
> > >
> > > > + Toggle **High-Conversion-Gain** mode.
> > > > +
> > > > + *0 = LCG (default), 1 = HCG.*
> > >
> > > An HCG / LCG control would also be applicable for IMX290 [1], so it
> > > would be nice if this could be a generic control instead of imx585
> > > specific.
> > >
> > > I never got a good description as to the benefit HCG was meant to
> > > give. The datasheet for IMX290 says the conversion efficiency ratio
> > > between HCG and LCG is 2, but not why that is any better than adding
> > > 6dB of analog gain.
> >
> > What I've learned is that HCG is actually a 2nd stage amplifier, so
> > instead of using one for high gain, you can split it into two lower
> > gain stages that lower the noise.
>
> DCG as a term is a bit confusing. It was initially used to describe an
> inter-frame HDR technique, where two different conversion gains are
> applied to separate frames by switching on and off an extra capacitor on
> the pixel's floating diffusion node. I don't know how it evolved from
> there, but just adding a second amplifier doesn't seem to be a very
> effective way to lower noise.
>
> If anyone does some research on the topic and finds clear information,
> please share them.
>
>From my understanding, DOL-HDR (Digital Overlapping), multi-frame HDR
or stagger HDR are all using different exposure times to separate
high/low brightness image frames. DCG with multiframe HDR is kinda
weird because the main reason to have DCG is that you can have two
gains with one exposure/charge so both high/low gain images are
exactly from the same sample and not time delayed. If multi-frame HDR
is acceptable then using two exposure times is way easier to implement
then DCG just for multi-frame HDR.
Now that I think about it and see the register naming, I think HCG
control is for a DCG enabled sensor to select between high/low
conversion gain channels to use for the SDR result.
> > This is also why ClearHDR and HCG have the conflict, because it is
> > basically sampling after the 1st gain stage and 2nd gain stage as
> > High/Low gain images.
> >
> > I have thought about making it more generic because IMX662 and IMX678
> > are going to be the next patch once this one passes that also has this
> > function (the two are basically stripped down versions from IMX585
> > that the driver can be adapted to easily). I intended for the upcoming
> > IMX662/IMX678 driver to use IMX585's V4L2 HCG control also.
> >
> > But given that I'm new to Linux kernel developments, or more
> > specifically, V4L2, I really don't have an idea how to do so in a way
> > the patch will be accepted.
> > Or I can make this more generic name to replace IMX585 for STARVIS2,
> > for example: V4L2_CID_STARVIS2_HIGH_CONV_GAIN
>
> We should really try to standardize V4L2 controls for HDR support in
> sensors. See https://lore.kernel.org/linux-media/20250710220544.89066-1-mirela.rabulea@nxp.com
> for an attempt at doing so (for some of the controls at least). Could
> you share your feedback in that mail thread ?
>
My counter argument will be that HDR controls are way too diverse
vendor to vendor and even sensor to sensor that having a sensor-series
based HDR controls would suit better.
I think it might be better to focus on how to handle multiple image
channels from one sensor because what makes this (IMX585) sensor
unique (and thus, the implementations will be somewhat unique) is the
capability to merge the image on the sensor directly and output as a
12bit compressed or 16bit linear image. Without the ability to handle
multiple MIPI virtual channels for far more common DOL-HDR or
multi-frame based HDR sensors, there isn't a lot of need to
standardize V4L2 HDR controls.
> The proposal doesn't address the sensor-side blending controls. For
> those, I recommend considering how they should be handled from
> userspace. We don't want to have per-sensor code there if we can avoid
> it, and that should drive the API design.
>
Can you recommend a way to handle the blending controls from userspace?
I will be glad to move these controls to module parameters then V4L2
controls if we don't want per-sensor code. In the worst case I'll
remove the ClearHDR functions and HCG controls for this driver so the
patches go through.
> As for what you call gradation above, that's not strictly speaking
> limited to HDR, right ? If my understanding is right, this is about
> applying a compression curve to lower the bandwidth by lowering the
> number of bits per pixel while preserving more dynamic range in the
> lower and middle parts of the pixel value range. Is that correct ? If
> so, the term "companding" is also often used for this feature. I think
> we have three needs:
>
> - Enable/disable companding (which includes configuring the bit depth of
> the output format)
>
> - Obtaining the companding curve applied by the sensor (as the host will
> have to apply the inverse expansion curve)
>
> - Optionally, modifying the companing curve.
>
> I'd like standard controls for those.
>
Let me describe the data flow here:
+-----------+ +-----------+
| High gain | | Low gain |
+-----------+ +-----------+
[12b] | [12b] |
v v
+-----------------------------------------------+
| Linear merging stage |
| ( DATA_SEL, DATA_BK) |
+-----------------------------------------------+
|
(16bit output)
|
v
+-----------------------------------------------+
| Gradient Compression stage |
| (GRAD_TH, GRAD_COMP) |
+-----------------------------------------------+
|
(12bit output)
|
v
So both 12 bit compressed and 16 bit linear are all based on the HDR
result and even though this is exactly the same as companding, I still
think we need sensor dependent control here.
>From what I can see from the public datasheets, OX03C10 has a similar
data compression algorithm called piecewise linear (PWL), which acts
on 24bit(!) 3-frame HDR results with 32(!) curves.
On the Sony side I don't see this capability on any sensor other than IMX585.
> > > Sony's website [2] states
> > > "Sony’s Super High Conversion Gain technology is designed to amplify
> > > electrical signals immediately after the conversion from photons, when
> > > the noise levels are relatively low. In this way, it reduces the
> > > overall noise after amplification. As a result, lower-noise images,
> > > compared to conventional technology, can be captured even in a
> > > low-illuminance environment. Lower noise levels in images also help to
> > > enhance the accuracy in visual or AI-assisted image recognition."
> > > From that one would presume you'd always want it on (lower noise =
> > > good), unless needing the minimum exposure time and the image was
> > > already over-exposed.
> > > I'm guessing you have no additional information based on your description text.
> > >
> > > Dave
> > >
> > > [1] Also IMX327, IMX462, and IMX662 which are in the same family,
> > > IMX678 (ratio of 2.6), and quite probably most of the Sony Starvis or
> > > Starvis 2 ranges.
> > > [2] https://www.sony-semicon.com/en/technology/security/index.html
> >
> > Yeah I think Starvis 2 series all have this capability.
> >
> > > > +
> > > > +Notes
> > > > +-----
> > > > +
> > > > +* Controls are writable while streaming; changes take effect from the
> > > > + next frame.
> > > > +* HDR-specific controls are hidden when HDR is disabled.
> > > > +* Inter-control dependencies are enforced by the driver.
> > > > diff --git a/Documentation/userspace-api/media/drivers/index.rst b/Documentation/userspace-api/media/drivers/index.rst
> > > > index d706cb47b..87912acfb 100644
> > > > --- a/Documentation/userspace-api/media/drivers/index.rst
> > > > +++ b/Documentation/userspace-api/media/drivers/index.rst
> > > > @@ -32,6 +32,7 @@ For more details see the file COPYING in the source distribution of Linux.
> > > > cx2341x-uapi
> > > > dw100
> > > > imx-uapi
> > > > + imx585
> > > > max2175
> > > > npcm-video
> > > > omap3isp-uapi
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 175f5236a..42e32b6ba 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -23183,6 +23183,7 @@ M: Will Whang <will@...lwhang.com>
> > > > L: linux-media@...r.kernel.org
> > > > S: Maintained
> > > > F: Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
> > > > +F: Documentation/userspace-api/media/drivers/imx585.rst
> > > > F: drivers/media/i2c/imx585.c
> > > > F: include/uapi/linux/imx585.h
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
Powered by blists - more mailing lists