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 5B537C2D0CD for ; Wed, 21 May 2025 19:05:41 +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=skjzg2uz8BH+niRisjzH3Un/beL40R1SkUTg5asWY6o=; b=HfTUpmSgc4mK89C4JfOt+OFftu gU7aQvB2AZa5GFA9e1f/Kf1BE8BCiV4af2m5XM5jOK/As5vbfedFNUYhiPahgvbTk4gS1yUcD5Ean nk0BAkoYDCChxMl9PSjBcDzcILmp38fjztAVbV73SpD9jjXMLVYgxWEQ35pFJbLwy4In92ALGLRJv +/6G5qVULeO6DSWMWxVoMsum07/D+Hps5oqpi7/s41kwPWKCwh2XK67Yq8Rm6V4GHIVmOgxj/QgcA N7vazqPgCHpm6TxYiBiqeg39dhrIa3HSCJDhqWfh33ndrQTCj+zRiS6tv8c6EfsFlkndtbs1mwzHf VR6Oeiug==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uHoku-0000000Gha1-3H7x; Wed, 21 May 2025 19:05:32 +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 1uHoij-0000000GhPC-1MWF; Wed, 21 May 2025 19:03:18 +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 01FD36AF; Wed, 21 May 2025 21:02:52 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1747854173; bh=vPvuwM/u6pIaMk4JleeZDQfpq6e7V/QhYzjGPzUWzvU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=b2z2+Tyww+BF+od3NVjTp2B0clH/Tgpur5Oa7hsMZDItXAFI7fJ7LGL4PZr9IZrXk spTxfIW7+zUmBa3KN3286SE5SoeKNG1AzVLCfiAVx8wfgAXRaqAJABKFIwuViNMDGw kbahZi1oSnS+ziSLw/Qks9hTKeTtMqYOQD2bXpHU= Date: Wed, 21 May 2025 21:03:08 +0200 From: Laurent Pinchart To: Paul Elder Cc: Krzysztof =?utf-8?Q?Ha=C5=82asa?= , 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: <20250521190308.GB6792@pendragon.ideasonboard.com> References: <20250521101042.GC12514@pendragon.ideasonboard.com> <174784811736.14042.11404187248848039485@calcite> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <174784811736.14042.11404187248848039485@calcite> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250521_120317_830439_DC1984E3 X-CRM114-Status: GOOD ( 49.77 ) 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 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 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(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 > > (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 Should we update the commit message as you initially proposed ? -- Regards, Laurent Pinchart