* [PATCH] media: rkisp1: correct histogram window size
@ 2025-05-29 9:29 Krzysztof Hałasa
0 siblings, 0 replies; 3+ messages in thread
From: Krzysztof Hałasa @ 2025-05-29 9:29 UTC (permalink / raw)
To: Dafna Hirschfeld
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Heiko Stuebner,
linux-media, linux-rockchip, linux-arm-kernel, linux-kernel,
Jacopo Mondi, Paul Elder, Ondrej Jirman, Tomi Valkeinen
The histogram window dimensions are unnecessarily decremented. This can be
seen in the following example, where when all pixels are of maximum intensity
there are pixels that we expect to be counted but are missing from the highest
bin. Fix this by removing the unnecessary decrement.
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), all weights are set to 1:
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.
E.g. without the patch, the pixels counted in lines are:
0, 3, ... 1914 (which makes 1917/3), and patched:
0, 3, ... 1914, 1917 (which makes 1920/3).
The same is true for rows.
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>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH] media: rkisp1: correct histogram window size
@ 2025-05-23 9:10 Krzysztof Hałasa
2025-05-23 11:05 ` Paul Elder
0 siblings, 1 reply; 3+ messages in thread
From: Krzysztof Hałasa @ 2025-05-23 9:10 UTC (permalink / raw)
To: Dafna Hirschfeld
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Heiko Stuebner,
linux-media, linux-rockchip, linux-arm-kernel, linux-kernel,
Jacopo Mondi, Paul Elder, Ondrej Jirman, Tomi Valkeinen
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), all weights are set to 1:
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.
E.g. without the patch, the pixels counted in lines are:
0, 3, ... 1914 (which makes 1917/3), and patched:
0, 3, ... 1914, 1917 (which makes 1920/3).
The same is true for rows.
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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] media: rkisp1: correct histogram window size
2025-05-23 9:10 Krzysztof Hałasa
@ 2025-05-23 11:05 ` Paul Elder
0 siblings, 0 replies; 3+ messages in thread
From: Paul Elder @ 2025-05-23 11:05 UTC (permalink / raw)
To: Dafna Hirschfeld, Krzysztof Hałasa
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Heiko Stuebner,
linux-media, linux-rockchip, linux-arm-kernel, linux-kernel,
Jacopo Mondi, Ondrej Jirman, Tomi Valkeinen
Hi Krzysztof,
Thanks for the new version.
My bad, I should've mentioned in the review of the previous version; I think
you should have a paragraph in the beginning briefly explaining the problem in
english.
"The histogram window dimensions are unnecessarily decremented. This can be
seen in the following example, where when all pixels are of maximum intensity
there are pixels that we expect to be counted but are missing from the highest
bin. Fix this by removing the unnecessary decrement."
I think would be good enough.
Quoting Krzysztof Hałasa (2025-05-23 18:10:47)
> 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), all weights are set to 1:
>
> 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.
>
> E.g. without the patch, the pixels counted in lines are:
> 0, 3, ... 1914 (which makes 1917/3), and patched:
> 0, 3, ... 1914, 1917 (which makes 1920/3).
> The same is true for rows.
I like this clarification.
With the above paragraph added,
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
(I suppose if you approve it then it could simply be added when it's applied
and you wouldn't need to send a new version, but I don't have permissions for
that so uh...)
>
> 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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-05-29 9:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-29 9:29 [PATCH] media: rkisp1: correct histogram window size Krzysztof Hałasa
-- strict thread matches above, loose matches on Subject: below --
2025-05-23 9:10 Krzysztof Hałasa
2025-05-23 11:05 ` Paul Elder
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox