[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2fd6wgsiwfx4raharcrpmmtayhkipjnz64u2cbprhsxkna3lhv@yshftexhmgns>
Date: Fri, 30 May 2025 09:56:32 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Krzysztof Hałasa <khalasa@...p.pl>,
Xavier Roumegue <xavier.roumegue@....nxp.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>, Rui Miguel Silva <rmfrfs@...il.com>,
Martin Kepplinger <martink@...teo.de>, Purism Kernel Team <kernel@...i.sm>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, linux-media@...r.kernel.org, imx@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Enable MIPI filtering by DT on i.MX8M*
Hi Krzysztof
thanks for the detailed analysis
On Thu, May 29, 2025 at 01:27:56PM +0200, Krzysztof Hałasa wrote:
> Hi Jacopo,
>
> Jacopo Mondi <jacopo.mondi@...asonboard.com> writes:
>
> >> - 10 User Defined 8-bit Data Type 8 packets, line-sized, DT=0x37, called
> >> apparently "Vertical OB" by Sony datasheet
> >
> > These are optically black pixels and should probably be discarded by
> > the gasket as well as embedded data ?
>
> Yes, apparently.
>
> >> I hope I got this right, this is straight from oscilloscope (only
> >> checked IDs on IMX462, will confirm IMX290 later but it looks the same).
> >> In 1280x1080p25 mode there are 4 (not 10) "vertical OB" packets, and 720
> >> RGGB lines instead of 1080.
> >
> > Is this correct ? you ask for 1280x1080 and get back 720 lines ?
>
> Well, no, I just checked both modes and these are the differences.
> IOW, nothing unexpected. Wrong copy & paste or so.
>
> >> IMX462 produces just a tiny 2-pixel dot in the left top corner, possibly
> >> shifting some data to the right (I remember it did that, but I can't
> >> observe it now - could be a kernel (driver) version change?).
> >
> > What are those two pixels ? Does the datasheet describes them ?
>
> Nope, I guess it's a silicon bug. It corrupts 3 RAW-12 pixels, though
> (32 bits > 2 * 12 bits).
>
> >> 32EC0138h 2D8000h ISP Dewarp Control Register (ISP_DEWARP_CONTROL)
> >> ISP ID mode 0, ISP1: DT 0h (unknown), ISP2: DT 2Ch (RAW12) left-just mode
> >
> > But this other register has (again) one other filtering option and
> > reading the value here it is set to filter RAW12 (mipi_isp2_data_type)
> >
> > Weirdly enough, I don't see this register being programmed in the
> > mainline gasket driver...
>
> I guess it's drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c:
>
> static int rkisp1_gasket_enable(struct rkisp1_device *rkisp1,
> struct media_pad *source)
> {
> ...
> regmap_update_bits(rkisp1->gasket, ISP_DEWARP_CONTROL, mask, val);
Ah indeed, thanks!
>
> Now this register doesn't filter data: if you set it with a different
> value, the data still goes through. It's just processed differently.
> For example, my sensor is 12-bit (in addition to 10-bit). If I set
> ISP_DEWARP_CONTROL to 0xB68 (or 0x368) meaning RAW 14-bit, the image
> only gets darker - ISP thinks it's getting 14-bit samples.
Thanks for testing
>
> The only filtering (while using ISP) is, apparently, in the ISP
> Configuration Register (MIPI_CSIS_ISPCONFIG_CH0) and subsequently in its
> shadow counterpart. And it somehow may be enabled in CSIS Common Control
> Register (MIPI_CSIS_CMN_CTRL). But if you don't enable it there, the
> data is still filtered (e.g. wrong value in MIPI_CSIS_ISPCONFIG_CH0
> prevents data flow). The filtering is only needed for preventing pixel
> corruption (these 3 pixels).
Let me recap all of this.
With:
- MIPI_CSIx_CSIS_COMMON_CTRL[11:10]
"Select Interleave mode" = 0x00 = CH0 only, no data interleave
- MIPI_CSIx_ISP_CONFIGn[7:2]
"Image Data Format" = 0x2c = RAW12
Embedded data and OB pixels are filtered (which means we're filtering
on DT even if MIPI_CSIx_CSIS_COMMON_CTRL[11:10] = 0x0x would suggest
filtering is not enabled)
However corrupted pixels are still sent through.
If you
- MIPI_CSIx_CSIS_COMMON_CTRL[11:10]
"Select Interleave mode" = 0x01 = DT (Data type) only
Embedded data and OB pixels are still filtered, and your corrupt
pixels are filtered as well.
Now, why are "corrupted pixels" filtered away by enabling this option ?
As far as I understand bad pixels are corrupt in their data values,
the CSI-2 packet header which contains the DT is correct. Is my
understanding correct ?
>
> So it means, for at least i.MX8MP, the DT filtering bit in
> MIPI_CSIS_CMN_CTRL should always be enabled.
It would be nice to actually understand what it does before enabling
it unconditionally.
I've cc-ed Xavier from NXP which maybe can help shed some light on
that register function ?
Thanks for the investigation
>
> >> MIPI_CSI2:
> >> 32E50004h 4705h CSIS Common Control Register (MIPI_CSIS_CMN_CTRL)
> >
> > Do you mean 0x14705 ? I'm asking because Shadow Crtl is BIT(16). Surprisingly
> > BIT(14) is marked as reserved in the datasheet version I have
>
> No, it's 0x4705 (usually).
> With 0xFF05 (resulting from write with 0xfffffffd) it's still working
> correctly (with DT filtering). Write 0xfffffbfd (= no DT filtering) and
> the magic light pixels in the left top corner reappear. So it means the
> INTERLEAVE_MODE bits (11 and 10) are probably two independent bits, with
> bit 11 probably not used at all.
>
>
> In my copy (i.MX 8M Plus Applications Processor Reference Manual, Rev.
> 1, 06/2021), CSIS Common Control Register (MIPI_CSIx_CSIS_COMMON_CTRL):
> - bits 31-17, 15, 13, 12, 7-2 are zero and reserved (though bits 12 and
> 2 are additionally marked "This read-only field is reserved and always
> has the value 0")
> - bit 14 is reserved and shown as "1"
> - bit 16 is "UPDATE_SHADOW", and it clears itself after a write (unless
> the pipeline locks up or something alike)
> - bits 11 and 10 are "INTERLEAVE_MODE":
> Select Interleave mode
> ISP Configuration register of CH# is used for data interleave.
> 00 CH0 only, no data interleave
> 01 DT (Data type) only
> 10 Reserved
> 11 Reserved
> - bits 9 and 8 are LANE_NUMBER, 0 to 3 means 1 to 4.
> - bit 1 is SW_RESET
> - bit 0 is CSI_EN
>
> In fact, bit 2 does not "always have the value 0", it's set to 1. Both
> bits 14 and 2 can be reset to 0, though (without apparent change in the
> image).
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa
>
Powered by blists - more mailing lists