| Message ID | m3tt5u9q7h.fsf@t19.piap.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RKISP1: correct histogram window size | expand |
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.
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 ?
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);
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>