Message-ID: <20250521101042.GC12514@pendragon.ideasonboard.com>
Date: Wed, 21 May 2025 12:10:42 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Krzysztof Hałasa <khalasa@...p.pl>
Cc: Paul Elder <paul.elder@...asonboard.com>,
	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

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.

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

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.

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

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists