Re: [PATCH v11 39/66] media: Documentation: Add subdev configuration models, raw sensor model

[Stefan Klug <stefan.klug@ideasonboard.com>]

Hi Sakari,

Quoting Sakari Ailus (2026-01-15 13:04:28)
> Hi Kieran, Stefan, Jacopo,
> 
> On Fri, Oct 10, 2025 at 11:56:37AM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2025-10-10 10:45:49)
> > > Hi Sakari, hi Jacopo,
> > > 
> > > Quoting Jacopo Mondi (2025-10-07 16:01:11)
> > > > Hi again Sakari,
> > > >    cc Kieran and Stefan
> > > > 
> > > > Stefan and Kieran brought to my attention a use case I would like to
> > > > discuss with you
> > > > 
> > > > On Fri, Oct 03, 2025 at 03:25:19PM +0200, Jacopo Mondi wrote:
> > > > > Hi Sakari,
> > > > >
> > > > > On Fri, Oct 03, 2025 at 04:15:34PM +0300, Sakari Ailus wrote:
> > > > > > Hi Jacopo,
> > > > >
> > > > > [snip]
> > > > >
> > > > > > > >
> > > > > > >
> > > > > > > A recent discussion on libcamera made me wonder a few things
> > > > > > >
> > > > > > > https://patchwork.libcamera.org/patch/24547/
> > > > > > >
> > > > > > > In the current world (pre-RAW sensor model) the situation can be
> > > > > > > summarized as
> > > > > > >
> > > > > > > TGT_NATIVE_SIZE = full pixel array (readable and non readable)
> > > > > > > TGT_CROP_BOUNDS = readable pixel array (visible and non visibile pixels)
> > > > > >
> > > > > > Crop bounds is generally the same as native size.
> > > > > >
> > > > >
> > > > > I checked two sensors datasheet for this yesterday and both of them
> > > > > had parts of the pixel array that cannot be read out
> > > > >
> > > > > > > TGT_CROP_DEFAULT = visible pixels
> > > > > >
> > > > > > The default could exclude not-so-great pixels, too.
> > > > > >
> > > > >
> > > > > ok, suggested pixel array area used for image capture purposes then
> > > > >
> > > > > > > TGT_CROP = analgoue crop
> > > > > >
> > > > > > This could include digital crop as well.
> > > > > >
> > > > >
> > > > > Yes it might, not all sensor drivers behaves the same indeed
> > > > >
> > > > > > >
> > > > > > > where:
> > > > > > > - visibile = pixels used for image capture purpose
> > > > > > > - non-visible = optically black, dummies etc
> > > > > > >
> > > > > > > With the RAW sensor model:
> > > > > > >
> > > > > > > format(1/0) = readable pixel array (visible and non visible)
> > > > > > > TGT_CROP_DEFAULT(1/0) = visible pixel area
> > > > > > > TGT_CROP(1/0) = analogue crop
> > > > > > > TGT_COMPOSE(1/0) = binning/skipping
> > > > > > >
> > > > > > > Have we lost the ability to report the full pixel array size (readable
> > > > > > > and not readable) ? Is this intentional ? As if pixels cannot be read
> > > > > > > out they basically do no exist, and the information on the actual
> > > > > > > number of pixels (including non readable ones) should be kept
> > > > > > > somewhere else (like the libcamera sensor properties database) ?
> > > > > >
> > > > > > I'd keep this information in the user space if needed. There's little
> > > > > > software could presumably do with this information.
> > > > > >
> > > > >
> > > > > Agreed, there is no value I can think of in having this information in
> > > > > drivers
> > > > >
> > > > 
> > > > So, Kieran and Stefan are working with a sensor whose driver was
> > > > initially upstreamed with a wrong "readable pixel array"
> > > > (TGT_CROP_BOUNDS). The developer later realized there was more of the
> > > > pixel array to read and there was a use for the non-image pixels like
> > > > OB ones.
> > > > 
> > > > With the current model this is fine (sort of), as all rectangles are
> > > > expressed with the TGT_NATIVE size reference. TGT_BOUNDS might
> > > > increases but TGT_CROP_DEFAULT and TGT_CROP are still valid both in the
> > > > driver but also in userspace, which might have encoded some known
> > > > "tested" configurations.
> > > > 
> > > > With the new model we lose the information reported by TGT_NATIVE and
> > > > all rectangles will be expressed with the format on 1/0 as reference.
> > > > If the format changes because we later find out there were portions of
> > > > the pixel array that could have been read out, all other rectangles
> > > > will have to change as well, both in the driver (which is ok-ish) but
> > > > also in userspace, which we have no control on.
> > > > 
> > > > Stefan and Kieran could elaborate more on this, but basically, the
> > > > physical array is the only fixed reference we could actually count on.
> > > > Other rectangles, are subject to the driver developer understanding of
> > > > how the device work, which as we know very well, can change over time.
> > > > 
> > > > Now, if you agree this is something to be concerned on, I presume the
> > > > fix is quite easy
> > > > 
> > > >          format(1/0) = physical pixel array size
> > > >          TGT_CROP_BOUNDS(1/0) = readable pixel array (visible and non visible)
> > > >          TGT_CROP_DEFAULT(1/0) = visible pixel area
> > > >          TGT_CROP(1/0) = analogue crop
> > > >          TGT_COMPOSE(1/0) = binning/skipping
> > > > 
> > > > which basically only require re-introducing the use of CROP_BOUNDS in
> > > > the RAW camera model specification.
> > > 
> > > Thanks Jacopo for writing that up. Maybe a little addition on that
> > > matter. To our (especially Kierans) experience all the rectangles tend
> > > to be unexpectedly difficult to handle when you try to configure the
> > > sensors in a pixel perfect manner (having binned and non binned modes
> > > cover exactly the same area in all possible flipping configurations).
> > > The datasheets I'm aware of use the physical pixel array as common
> > > coordinate system to describe the geometry. Adding the readable pixel
> > > array as "artificial" coordinate system makes it difficult to match the
> > > values reported by a v4l driver with the datasheets at hand.
> > > 
> > > Another time where this comes into play is lens shading correction where
> > > you would want to describe the LSC against one reference coordinate
> > > system that ideally never ever changes.
> > > 
> > > To add to the confusion I'd love to have another rectangle added to the
> > > list. I don't have a proper name for it. The intent would be to
> > > distinguish between the "readable pixel array" and the "light exposed
> > > pixel array". So the list would become:
> > > 
> > >     format(1/0) = physical pixel array size
> > >     TGT_CROP_BOUNDS(1/0) = readable pixel array (visible and non visible)
> > >     TGT_CROP_VISIBLE(1/0) = visible pixel area including "flesh" for ISP
> 
> Carnivorous ISPs?

Yes, beware! :-)

> 
> > >     TGT_CROP_DEFAULT(1/0) = Recommended "good" pixels
> 
> How often is this known? Or would you rely on what the vendor tells?

We had a brief discussion about that internally. Some datasheets contain
that information by surrounding the "Recording area" with a margin
labeled as "Effective margin for color processing". I don't have numbers
how often that is the case though. As it doesn't add much technical
value to the kernel we could set TGT_CROP_DEFAULT to the above mentioned
TGT_CROP_VISIBLE and keep the information about the color processing
margin outside the kernel, e.g. in libcamera.

The only downside I see there is that (to my knowledge) it will be
difficult to guarantee that really all the pixels inside
TGT_CROP_VISIBLE are good. That's ok for libraries like libcamera that
will default to cutting off some margin after processing, but it might
be a problem for simple applications that just try capture the
CROP_DEFAULT. But maybe these applications do not exist at all :-)

> 
> > >     TGT_CROP(1/0) = analogue crop
> > >     TGT_COMPOSE(1/0) = binning/skipping
> > > 
> > > The idea is to be able to capture a larger image from the sensor for ISP
> > > processing and then cut it down to CROP_DEFAULT. This way we can prevent
> > > interpolation seams at the edges. Maybe the naming is bad and we should
> > > make CROP_BOUNDS the recommended area and add CROP_READABLE to denote
> > > the readable pixels...
> 
> CROP_BOUNDS really needs to be the bounding rectangle for everything.
> CROP_DEFAULT and NATIVE_SIZE are still up to definition, to a degree.
> 
> Have you tried capturing the optical black pixels on different sensors?

Yes, we did at least on the imx335 and imx283. Kieran is more in the
details there...

> 
> CCS specifies optical black pixel capture separately from the visible
> pixels and support also different VC, DT, depending on sensor capabilities;
> cropping results in capturing optical black pixel on the rows and columns
> specified for the crop area. In other words, the selection API isn't
> necessarily how optical black pixel capture is configured, at least for
> CCS.

Yes, that is a bit of a pain. Being able to select the VC or DT doesn't
make things easier. Some SoCs (e.g. imx8mp) are not able to capture
separate VCs or DTs. So to get to the information the only way is to go
through the image stream. Also for some use-cases like extra long
exposures in astro-photography it might be useful to have the OB in the
image data for postprocessing.  (The last case is a bit made up as I
don't know much about astro-photography tbh). 

So I believe we should keep the option to capture OB over the image
stream.

> 
> Let's assume the case of the sensor where you find, after the driver has
> been upstreamed, that there are extra columns and rows of pixels beyond the
> left and top of the image. How would you modify the selection rectangles
> and the format to allow capturing image data from that area? Can you avoid
> affecting the existing userspace?

That would mean that the TGT_CROP_BOUNDS and or TGT_CROP_DEFAULT
changes. The assumption is that the native coordinate system (the old
TGT_CROP_NATIVE_SIZE) stays in place so the application can still use
all the tuning that was done relative to the native coordinate system.
As the TGT_CROP_BOUNDS from the first version has top/left coordinates >
0 there is room to enlarge the TGT_CROP_BOUNDS in that direction.

Sure, this does not work if the initial driver didn't specify
TGT_CROP_BOUNDS with the correct top/left offset. It also does not guard
against really wrong datasheets or arbitrary bugs. But allows to
transparently improve in situations where the first version implements
the "default FullHD setup from the vendor" and later a improved version
of the driver allows access to the additional margins around.

Best regards,
Stefan

> 
> > 
> > I think this is important - and in IMX283 - it seems to be exactly what
> > the datasheet is ultimately recommending.
> > 
> > > > What do you think ?
> > > 
> > > Best regards,
> > > Stefan
> > > 
> > > > 
> > > > > > >
> > > > > > > All the discussion about readable/non-readable, visible/non-visibile
> > > > > > > and active and inactive areas make me think we would benefit from
> > > > > > > presenting a small glossary at the beginning of the "Sensor pixel
> > > > > > > array size, cropping and binning" paragraph ?
> > 
> > Oh yes, some sort of nicely interpretable description of "this rectangle
> > is expected to be this equivalent set of pixels" would be helpful. I've
> > found it so hard to identify which the right rectangle is here :D
> > 
> > 
> > I'm also weary that we might need to find a way to convey the
> > relationship between binning mode restrictions/offsets too.
> > 
> > The IMX283 2x2 and 3x3 binning modes produce offset outputs:
> > 
> > vwinpos is the coordinate programmed to the sensor to get a position -
> > and we have tooling in camshark that lets us determine the exact pixels
> > we capture from any mode to see what was really produced by the sensor:
> > 
> > 
> > ```
> > 'Sensor Native' ?
> > pixel (row)   -8 -7 -6 -5 -4 -3 -2 -1  0  1  2  3  4  5  6  7  8  9  10 11 12 13 14 15 16
> > native pixels  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |
> > 
> > 
> > vwinpos       -4    -3    -2    -1     0     1     2     3     4     5     6     7     8
> > no-binning     |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |  |
> > 
> > 
> > vwinpos             -3          -2          -1           0           1           2      
> > /2 binning     |     |     |     |     |     |     |     |     |     |     |     |     |
> > 
> > 
> > vwinpos        0                 1                 2                 3                 4
> > /3 binning     |        |        |        |        |        |        |        |        |
> > ```
> > 
> > 
> > To fix this - I'm moving the current '0' position for IMX283 to -2 in
> > the non binned modes.
> > 
> > That gives me a coordinate system where at least I can define an
> > alignment for 2x2 and 3x3 binning every 12 pixels (as the lowest common
> > multiplier).
> > 
> > But - also murkies the water from the above rectanges as this sensor
> > turns out to have 48 lines of pixels (24 bayer pair lines) 'less than
> > zero' ... or at least those are the ones that are visible in light. I
> > can read further below zero but only for black pixels.
> > 
> > So in otherwords - I can't even make a corresponding coordinate
> > rectangle that exactly matches the 'all readable' pixels or 'all
> > illuminated pixels' ... Ayeeeeee...
> > 
> > My lesson/take on this: Don't trust the datasheets. They lie :D
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
>