From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5EFC8C54E65 for ; Wed, 21 May 2025 10:30:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=1MK7Xrodqhlu8yhuvAl9KhCWFk0Xymqc7fBJim3qekA=; b=G2mlac5bKWMXhv1sEV6sTg83BQ 0mOFzbrYDUqcSqwvw/X34nmXPc1JdxaftjOcjBjvUKRReqnBWWnOYvUAMa14crru5Hck7DZx/K8lo PvtsRg4lFBc77srXzKKBseQd6mPHaWnBxjICDdEsTzxEfZRyiyY/X8WgcXRL7BXq+E1mZyBtZYuz3 pwKJl8Owa80UK7excw6M6Srm5LHpPEabBY18fPIdSaGAIS0FZ+FtDF8yGBmEfznJO6vo2eJ7VuYqV FQIGRn+P9l2xPWs7i6fn1E/z3tjpqHjNyup3T5nYc3aZWzcLvx99w1AY8jQhP+BoIHqcU3zPzKAOb PuRrZ0eQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uHghx-0000000FT4c-1ezL; Wed, 21 May 2025 10:29:57 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1uHgPW-0000000FPal-1Jcc; Wed, 21 May 2025 10:10:55 +0000 Received: from pendragon.ideasonboard.com (179.218-130-109.adsl-dyn.isp.belgacom.be [109.130.218.179]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 179C76B5; Wed, 21 May 2025 12:10:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1747822227; bh=dxAfHCbzu/1/eFKa6ozmSU01XQUjFGYoBCNB5/x0dQI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=NbjLX9Gb7voEGiUjcAeGHMkJeBmtKQK2Pd2u3OJRJ2CvlfC9Al3BQ2d2Qxut4L43M 2WI0vWQKJu24p++0F2f3v69dFLLgvq8yzJ7sZrPWaGAJSrLj6+Hm+nPS4hiXBIlET3 qLACFCIacYNE2XZODgeqacGyOovzxS50Z12Q8Ca4= Date: Wed, 21 May 2025 12:10:42 +0200 From: Laurent Pinchart To: Krzysztof =?utf-8?Q?Ha=C5=82asa?= Cc: Paul Elder , Dafna Hirschfeld , Mauro Carvalho Chehab , Heiko Stuebner , linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Jacopo Mondi , Ondrej Jirman , Tomi Valkeinen , stefan.klug@ideasonboard.com Subject: Re: [PATCH] RKISP1: correct histogram window size Message-ID: <20250521101042.GC12514@pendragon.ideasonboard.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250521_031054_492756_9596161B X-CRM114-Status: GOOD ( 32.47 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, May 20, 2025 at 03:26:58PM +0200, Krzysztof Hałasa wrote: > Paul Elder 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(std::ceil(root)); return std::clamp(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