Message-ID: <174792044639.778642.7190019361640032369@calcite>
Date: Thu, 22 May 2025 22:27:26 +0900
From: Paul Elder <paul.elder@...asonboard.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Krzysztof Hałasa <khalasa@...p.pl>, Dafna Hirschfeld <dafna@...tmail.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, Heiko Stuebner <heiko@...ech.de>, linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, Jacopo Mondi <jacopo.mondi@...asonboard.com>, Ondrej Jirman <megi@....cz>, Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, stefan.klug@...asonboard.com
Subject: Re: [PATCH] RKISP1: correct histogram window size

Quoting Laurent Pinchart (2025-05-22 04:03:08)
> On Wed, May 21, 2025 at 07:21:57PM +0200, Paul Elder wrote:
> > Quoting Laurent Pinchart (2025-05-21 12:10:42)
> > > On Tue, May 20, 2025 at 03:26:58PM +0200, Krzysztof Hałasa wrote:
> > > > Paul Elder <paul.elder@...asonboard.com> writes:
> > > > 
> > > > >> Without the patch (i.MX8MP, all-white RGGB-12 full HD input from
> > > > >> the sensor, YUV NV12 output from ISP, full range, histogram Y mode).
> > > > >> HIST_STEPSIZE = 3 (lowest permitted):
> > > > >
> > > > > According to the datasheet, the histogram bins are 16-bit integer with a
> > > > > 4-bit fractional part. To prevent overflowing the 16-bit integer
> > > > > counter, the step size should be 10.
> > > 
> > > That would be for combined RGB mode, as every pixel is accounted for
> > > three times in that mode. In other modes, a step size of 8 should be
> > > fine.
> > 
> > Ah, right.
> > 
> > > > >
> > > > > Do you have any other information on this? Is it known that it's stable
> > > > > and consistent to use all 20 bits anyway?
> > > 
> > > The documentation states that the width of the bin counter registers is
> > > 20 bits wide including a 4-bit fractional part, and that the software
> > > should use only the upper 16 bits of the bin counters. The fractional
> > > part is caused by the weights. There's a corresponding todo comment in
> > > libcamera:
> > > 
> > >         ...
> > >          *
> > >          * \todo Take into account weights. That is, if the weights are low
> > >          * enough we can potentially reduce the predivider to increase
> > >          * precision. This needs some investigation however, as this hardware
> > >          * behavior is undocumented and is only an educated guess.
> > >          */
> > >         int count = mode == RKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED ? 3 : 1;
> > >         double factor = size.width * size.height * count / 65536.0;
> > >         double root = std::sqrt(factor);
> > >         uint8_t predivider = static_cast<uint8_t>(std::ceil(root));
> > > 
> > >         return std::clamp<uint8_t>(predivider, 3, 127);
> > > 
> > > libcamera sets the default weights to 1, and discards the 4 fractional
> > > bits. It seems that the 
> > 
> > (what did you mean to finish saying...?)
> 
> Oops. Ignore that, I split my reasoning to two paragraphs and forgot to
> delete that half line.
> 
> > > I expect that each pixel contributes to its bin by adding the weight
> > > value corresponding to its zone. Setting all weights to 1, I would
> > > expect that the 4 fractional bits could be used to increase the bin size
> > > to 1048575 pixels (20 bits), and therefore decrease the predivider from
> > > 10 to 3.
> > 
> > True. I suppose if all the weights are 1 then we can squeeze out more bit
> > precision then. But that's a todo for libcamera.
> > 
> > > > Interesting. I only have those mrv_*.h files which come with
> > > > isp-imx-4.2.2.* package(s). Here we have (among others):
> > > > 
> > > > /*! Register: isp_hist_prop: Histogram properties (0x00000000)*/
> > > > /*! Slice: stepsize:*/
> > > > /*! histogram predivider, process every (stepsize)th pixel, all other pixels are skipped */
> > > > /* 0,1,2: not allowed */
> > > > /* 3: process every third input pixel */
> > > > /* 4: process every fourth input pixel */
> > > > /* ...*/
> > > > /* 7FH: process every 127th pixel */
> > > > #define MRV_HIST_STEPSIZE_MASK 0x000003F8
> > > > #define MRV_HIST_STEPSIZE_SHIFT 3
> > > > 
> > > > In case of my IMX290 1920x1080 sensor, 1 doesn't work well (it stops
> > > > counting before reaching $((1920x1080)) in each bin, and even if no bin
> > > > reaches this magic value, the total count may be invalid (not equal to
> > > > the number of pixels). IIRC, 2 worked well. Maybe with higher
> > > > resolutions, I don't know.
> > > > 
> > > > I'm currently using "3" per the .h file:
> > > > isp_hist_prop:
> > > > 32E12400: 1Dh
> > > > histogram_measurement_result:
> > > > 32E12414: 0 0 1 1004 569 476 633 1197 2373 2212 1923 2945 3632 3025 5821 204589
> > > > which sums to 518400 = 1920*1080/9.
> > > > 
> > > > Setting "2", the same input scene:
> > > > 32E12400: 15h
> > > > 32E12414: 0 0 0 2194 1263 1096 1406 2528 5228 5052 4291 6354 8322 6943 13201 460522
> > > > which sums to 518400 = 1920*1080/4.
> > 
> > Yes, these look good (although I think you might've copy&pasted the wrong
> > number for the sum)
> > 
> > > > Setting "1", the same input scene:
> > > > 32E12400: Dh
> > > > 32E12414: 0 0 25 9046 4924 4317 5435 10655 20781 18965 16051 24716 32681 28368 54301 1048559
> > > > which sums to 1278824 which is rather less than 2073600.
> > > > The last number (1048559) is the magic one, no bin can go higher. Less lights and:
> > 
> > Oh? I would've expected 2^20-1 = 1048575 to be the magic number, but ok I
> > suppose the hardware caps at 1048559 instead. It probably overflowed and that's
> > why the sum is so low.
> > 
> > > > 32E12400: Dh
> > > > 32E12414: 0 0 0 0 0 0 184 3059 11970 75298 114898 211444 429772 439922 400358 386695
> > > > total = 2073600. But don't rely on it too much, the "1" has problems.
> > 
> > That's interesting. My guess would be that in practice a divider of 1 would
> > still work as long as you make sure that it doesn't overflow. Maybe the usage
> > documentation was based on a rule-of-thumb.
> > 
> > > > In short, those are integer values. One may use them as fractionals with
> > > > some clever step size, I guess.
> > > > 
> > > > >> isp_hist_h_size: 383 (= 1920 / 5 - 1)
> > > > >> isp_hist_v_size: 215 (= 1080 / 5 - 1)
> > > > >> histogram_measurement_result[16]: 0 0 0 0  0 0 0 0  0 0 0 0  0 0 0 229401
> > > > >>
> > > > >> Apparently the histogram is missing the last column (3-pixel wide,
> > > > >> though only single pixels count) and the last (same idea) row
> > > > >> of the input image: 1917 * 1077 / 3 / 3 = 229401
> > > > >
> > > > > I don't quite understand this. With a sub-window width of
> > > > > 1920 / 5 - 1 = 383, shouldn't the resulting total window width be
> > > > > 383 * 5 = 1915? Same idea for the height.
> > > > 
> > > > It would, but the stepsize = 3 makes it ignore only the last one
> > > > - i.e., normally the counted ones are 0, 3, ... 1914, 1917 (which makes
> > > > 1920/3) and with 383, it ends at 1914, thus only 3 pixels (1 really,
> > > > instead of 2) are missing from calculations (not 5). I guess the same
> > > > vertically, 1080 divides / 3 and 1075 doesn't.
> > 
> > Ah ok, I see. Thanks for the clarification.
> > 
> > > > 
> > > > > The fix looks fine though. Although, I'm wondering if there's a reason
> > > > > why there was a -1 in the first place. Does anybody know?
> > > > 
> > > > There is slight chance it's different on some other SoC, but I would be
> > > > surprised.
> > > 
> > > The documented constraint is
> > > 
> > >     hist_h_offset + hist_h_size x 5 should be less than or equal to the
> > >     horizontal size of the picture.
> > > 
> > > (and similar for the vertical direction). The initial -1 seems to be a
> > > bug.
> > 
> > Ok.
> > 
> > Looks go to me.
> > 
> > Reviewed-by: Paul ELder <paul.elder@...asonboard.com>
> 
> Should we update the commit message as you initially proposed ?

Although I don't remember explicitly proposing updating the commit message, yes
I would like an upgrade to it.

For one the subject should be prefixed with "media: rkisp1:" as opposed to just
"RKISP1:".

I'd also like a bit of clarification in the commit message about where 1917 and
1077 came from. A copy of what you explained to me, Krzysztof, would be
sufficient imo.

I think that should be good enough. The predivider discussion is technically
not in scope of this so I don't mind it not being mentioned. That or just
mention that it seems like a weight of 1 means that all 20 bits can be used as
an integer value. Up to you Krzysztof.


Thanks,

Paul

Powered by blists - more mailing lists