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

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

Hi Sakari,

Quoting Stefan Klug (2026-01-15 22:06:11)
> 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.

Jacopo told me that this wasn't completely clear and rereading it, he is
right. So to clarify: The idea is to skip the introduction of
TGT_CROP_VISIBLE and just include the processing margin in
TGT_CROP_DEFAULT. The less rectangles, the better.

> 
> 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
> >
>