linux-kernel - Re: [PATCH] RKISP1: correct histogram window size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <m3jz6b8lb1.fsf@t19.piap.pl>
Date: Tue, 20 May 2025 15:26:58 +0200
From: Krzysztof Hałasa <khalasa@...p.pl>
To: Paul Elder <paul.elder@...asonboard.com>
Cc: Dafna Hirschfeld <dafna@...tmail.com>,  Laurent Pinchart
 <laurent.pinchart@...asonboard.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

Hi Paul,

I'm sorry it took that long.

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