Re: [PATCH] RKISP1: correct histogram window size

From: Paul Elder <paul.elder@ideasonboard.com>
To: "Krzysztof Hałasa" <khalasa@piap.pl>
Cc: Dafna Hirschfeld <dafna@fastmail.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Jacopo Mondi <jacopo.mondi@ideasonboard.com>,
	Ondrej Jirman <megi@xff.cz>,
	Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
	stefan.klug@ideasonboard.com
Subject: Re: [PATCH] RKISP1: correct histogram window size
Date: Fri, 9 May 2025 14:29:06 +0200	[thread overview]
Message-ID: <aB31Eg6oRpcHHEsb@pyrite.rasen.tech> (raw)
In-Reply-To: <m3tt5u9q7h.fsf@t19.piap.pl>

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip