[PATCH] RKISP1: correct histogram window size

[PATCH] RKISP1: correct histogram window size

[Krzysztof Hałasa]

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

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

with the patch applied:
isp_hist_h_size: 384 (= 1920 / 5)
isp_hist_v_size: 216 (= 1080 / 5)
histogram_measurement_result[16]: 0 0 0 0  0 0 0 0  0 0 0 0  0 0 0 230400

1920 * 1080 / 3 / 3 = 230400

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index b28f4140c8a3..ca9b3e711e5f 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -819,8 +819,8 @@ static void rkisp1_hst_config_v10(struct rkisp1_params *params,
 		     arg->meas_window.v_offs);
 
 	block_hsize = arg->meas_window.h_size /
-		      RKISP1_CIF_ISP_HIST_COLUMN_NUM_V10 - 1;
-	block_vsize = arg->meas_window.v_size / RKISP1_CIF_ISP_HIST_ROW_NUM_V10 - 1;
+		      RKISP1_CIF_ISP_HIST_COLUMN_NUM_V10;
+	block_vsize = arg->meas_window.v_size / RKISP1_CIF_ISP_HIST_ROW_NUM_V10;
 
 	rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_HIST_H_SIZE_V10,
 		     block_hsize);

-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH] RKISP1: correct histogram window size

[Paul Elder]

Hi Krzysztof,

Thanks for the patch.

The code change looks sound, but I'm confused about the reasoning behind
it.

On Fri, May 09, 2025 at 09:51:46AM +0200, Krzysztof Hałasa wrote:
> 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.

Do you have any other information on this? Is it known that it's stable
and consistent to use all 20 bits anyway?

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

Also according to my understanding, the / 3 calculation is correct, but
it comes from the step size and not about missing last column/row.
Where does the missing last column/row come from?

> 
> with the patch applied:
> isp_hist_h_size: 384 (= 1920 / 5)
> isp_hist_v_size: 216 (= 1080 / 5)
> histogram_measurement_result[16]: 0 0 0 0  0 0 0 0  0 0 0 0  0 0 0 230400
> 
> 1920 * 1080 / 3 / 3 = 230400

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?


Thanks,

Paul

> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index b28f4140c8a3..ca9b3e711e5f 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -819,8 +819,8 @@ static void rkisp1_hst_config_v10(struct rkisp1_params *params,
>  		     arg->meas_window.v_offs);
>  
>  	block_hsize = arg->meas_window.h_size /
> -		      RKISP1_CIF_ISP_HIST_COLUMN_NUM_V10 - 1;
> -	block_vsize = arg->meas_window.v_size / RKISP1_CIF_ISP_HIST_ROW_NUM_V10 - 1;
> +		      RKISP1_CIF_ISP_HIST_COLUMN_NUM_V10;
> +	block_vsize = arg->meas_window.v_size / RKISP1_CIF_ISP_HIST_ROW_NUM_V10;
>  
>  	rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_HIST_H_SIZE_V10,
>  		     block_hsize);
> 
> -- 
> Krzysztof "Chris" Hałasa
> 
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH] RKISP1: correct histogram window size

[Krzysztof Hałasa]

Hi Paul,

I'm sorry it took that long.

Paul Elder <paul.elder@ideasonboard.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.
>
> Do you have any other information on this? Is it known that it's stable
> and consistent to use all 20 bits anyway?

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.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH] RKISP1: correct histogram window size

[Laurent Pinchart]

On Tue, May 20, 2025 at 03:26:58PM +0200, Krzysztof Hałasa wrote:
> Paul Elder <paul.elder@ideasonboard.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

Re: [PATCH] RKISP1: correct histogram window size

[Paul Elder]

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@ideasonboard.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...?)

> 
> 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@ideasonboard.com>

Re: [PATCH] RKISP1: correct histogram window size

[Laurent Pinchart]

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@ideasonboard.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@ideasonboard.com>

Should we update the commit message as you initially proposed ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] RKISP1: correct histogram window size

[Paul Elder]

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@ideasonboard.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@ideasonboard.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

Re: [PATCH] RKISP1: correct histogram window size

[Krzysztof Hałasa]

Paul Elder <paul.elder@ideasonboard.com> writes:

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

Definitely :-)

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

I don't know. It seems it counts all right until reaching this magic
1048559 = 0xFFFEF. Then it stops at this value and stays there.

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

I don't know. TBH I guess I haven't tested it with mainline kernel (and
RK1ISP driver), only with the NXP VVCAM driver. But with stepsize = 1
I was getting certain differences in total number of pixels (not big
ones and not always - maybe 10% of frames or so, or less). Without
reaching the magic value, I mean.

If if was simple cap at 1048559 we could correct it in software (up to
2 Mpixels) - there could only be one bin overflowing.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa