Re: [PATCH v10 08/13] media: rockchip: rkcif: add support for mipi csi-2 capture

[Michael Riesch <michael.riesch@collabora.com>]

Hi Mehdi,

On 8/19/25 18:46, Mehdi Djait wrote:
> Hi Michael,
> 
> I am seeing IOMMU page faults: See below.

Sorry for the late reply. I had to get a similar setup first. Now I have
a Radxa ROCK 3A and a Radxa Camera 8M (with the Sony IMX219 sensor,
should be 100% compatible to the RasPi Cam v2.1) on my table.

> On Tue, Aug 19, 2025 at 01:26:00AM +0200, Michael Riesch via B4 Relay wrote:
>> From: Michael Riesch <michael.riesch@collabora.com>
>>
>> The RK3568 Video Capture (VICAP) unit features a MIPI CSI-2 capture
>> interface that can receive video data and write it into system memory
>> using the ping-pong scheme. Add support for it.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Michael Riesch <michael.riesch@collabora.com>
> 
> [..]
> 
>>  irqreturn_t rkcif_mipi_isr(int irq, void *ctx)
>>  {
>> +	struct device *dev = ctx;
>> +	struct rkcif_device *rkcif = dev_get_drvdata(dev);
>>  	irqreturn_t ret = IRQ_NONE;
>> +	u32 intstat;
>> +
>> +	for (unsigned int i = 0; i < rkcif->match_data->mipi->mipi_num; i++) {
>> +		enum rkcif_interface_index index = RKCIF_MIPI_BASE + i;
>> +		struct rkcif_interface *interface = &rkcif->interfaces[index];
>> +
>> +		intstat = rkcif_mipi_read(interface, RKCIF_MIPI_INTSTAT);
>> +		rkcif_mipi_write(interface, RKCIF_MIPI_INTSTAT, intstat);
>> +
>> +		for (unsigned int j = 0; j < interface->streams_num; j++) {
>> +			struct rkcif_stream *stream = &interface->streams[j];
> 
> In the TRM you can see in the MIPI_INTSTAT interrupts to detect
> overflows: why not activate them ?
> 
> something like this:
> 
> #define RKCIF_MIPI_INT_Y_OVERFLOW(id)          BIT(16)
> #define RKCIF_MIPI_INT_UV_OVERFLOW(id)         BIT(17)
> #define RKCIF_MIPI_INT_FIFO_OVERFLOW(id)       BIT(18)
> #define RKCIF_MIPI_INT_CSI2RX_FIFO_OVERFLOW(id)        BIT(20)
> 
> and then OR them with the int_mask in rkcif_mipi_start_streaming()
> 
> and then you can log the err if something happened ?

I have not needed these interrupts yet. They can be added any time
whenever they are required. (Patches welcome :-))

>> +
>> +			if (intstat & RKCIF_MIPI_INT_FRAME0_END(stream->id) ||
>> +			    intstat & RKCIF_MIPI_INT_FRAME1_END(stream->id)) {
>> +				ret = IRQ_HANDLED;
>> +
>> +				if (stream->stopping) {
>> +					rkcif_mipi_stop_streaming(stream);
>> +					wake_up(&stream->wq_stopped);
>> +					continue;
>> +				}
>> +
>> +				rkcif_stream_pingpong(stream);
>> +			}
>> +		}
>> +	}
>>  
>>  	return ret;
>>  }
> 
> Now to the IOMMU page faults:
> 
> Camera Sensor: IMX219
> Frame Size: 1920x1080
> Format: SRGGB10P
> 
> Packed SRGGB10
> --> Every four consecutive samples are packed into 5 bytes
> --> Stride = 2400 bytes (1920 * 5/4)
> 
> So the imagesize = 1080 * 2400 = 2 592 000
> 
> in __vb2_buf_mem_alloc() the size of the buf will be PAGE_ALIGNED in:
> PAGE_ALIGN(vb->planes[plane].length);
> 
> So we allocate a buffer with the size: 2 592 768 -> hex = 0x297000
> 
> In rkcif_mipi_queue_buffer():
> We will queue a total of two buffers to the HW (2 because of pingpong)
> The first buffer will have the address: 0x00000000ffc00000
> 
> We start to capture and then this happens:
> 
> rk_iommu fdfe0800.iommu: Page fault at 0x00000000ffe79000 of type write
> rk_iommu fdfe0800.iommu: iova = 0x00000000ffe79000: dte_index: 0x3ff pte_index: 0x279 page_offset: 0x0
> rk_iommu fdfe0800.iommu: mmu_dte_addr: 0x0000000012cc8000 dte@0x0000000012cc8ffc: 0x11a0d001 valid: 1 pte@0x0000000011a0d9e4: 0x31b79006 valid: 0 page@0x0000000000000000 flags: 0x0
> 
> With:
> 0xffe79000 = 0xffc00000 (buffer address) + 0x297000 (buffersize)
> 
> --> So the VICAP is overflowing the buffer even though everything was
> correctly configured ?! (If I understood everything correctly ofc.)

I could reproduce this behavior and found that the (hardcoded) virtual
line width needs to be adjusted accordingly. It was set to width * 2,
and it needs to be set to width * 10 / 8.

> I also see the same problem with the SRGGB8 format. It also happens in
> the downstream Radxa/Rockchip Kernel.

Strange that the downstream kernel has issues with that. Anyway, this
shouldn't be the issue here...

> Do you see the same problem ?

... but I could reproduce this behavior as well and trace it back to the
same root cause. Here, we need width * 1, of course.

I'll integrate the fix in v12, please stay tuned!

Best regards,
Michael