lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<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

Powered by Openwall GNU/*/Linux Powered by OpenVZ