pisp BE upstreaming

[jmondi]

Describe the bug

Let me here collect the potential issues that might need to be handled differently in mainline than in BSP.

The first and probably most evident one is the pre-allocation of the video device numbers. https://github.com/raspberrypi/linux/blob/rpi-6.1.y/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c#L27 This is not gonna fly in mainline most probably and could either be solved with changes to the framework or a patch could be kept downstream.

Secondly there's the question about why some formats have a JPEG colorspace assigned even if we know such colorspace is reserved for JPEG formats (I know there's a long discussion and I had to read it over again).

fail: v4l2-test-formats.cpp(360): pixelformat != V4L2_PIX_FMT_JPEG && pixelformat != V4L2_PIX_FMT_MJPEG && colorspace == V4L2_COLORSPACE_JPEG

There's great confusion there, but basically the idea is to avoid using V4L2_COLORSPACE_JPEG for non-JPEG formats in favor of specifying all colorimetry fields explicitly.

I would simply drop JPEG there to avoid questions on the list and use V4L2_COLORSPACE_SMPTE170M as it's done for the YVU format variants (even if V4L2_COLORSPACE_SMPTE170M is different from V4L2_COLORSPACE_JPEG as it specifies a different transfer function). As David said, we're going to set all three colorspace-related fields explicitly from userspace so this doesn't actually matter too much ?

	{
		.fourcc		    = V4L2_PIX_FMT_YUV420,
		/* 128 alignment to ensure U/V planes are 64 byte aligned. */
		.align		    = 128,
		.bit_depth	    = 8,
		.plane_factor	    = { P3(1), P3(0.25), P3(0.25) },
		.num_planes	    = 1,
		.colorspace_mask    = V4L2_COLORSPACE_MASK_ALL_SRGB,
		.colorspace_default = V4L2_COLORSPACE_JPEG,
	},
	{
		.fourcc		    = V4L2_PIX_FMT_YVU420,
		/* 128 alignment to ensure U/V planes are 64 byte aligned. */
		.align		    = 128,
		.bit_depth	    = 8,
		.plane_factor	    = { P3(1), P3(0.25), P3(0.25) },
		.num_planes	    = 1,
		.colorspace_mask    = V4L2_COLORSPACE_MASK_ALL_SRGB,
		.colorspace_default = V4L2_COLORSPACE_SMPTE170M,
	},

Apart from that, I have a few compliance checks only and one sparse/smatch warning

sparse: WARNINGS:

drivers/media/platform/raspberrypi/pisp_be/pisp_be.c:613:9: warning: context imbalance in 'pispbe_schedule_internal' - unexpected unlock
drivers/media/platform/raspberrypi/pisp_be/pisp_be.c:655:13: warning: context imbalance in 'pispbe_schedule_one' - different lock contexts for basic block
drivers/media/platform/raspberrypi/pisp_be/pisp_be.c:674:13: warning: context imbalance in 'pispbe_schedule_any' - wrong count at exit

smatch: WARNINGS:

drivers/media/platform/raspberrypi/pisp_be/pisp_be.c:695 pispbe_schedule_any() warn: inconsistent returns '&pispbe->hw_lock'.
  Locked on  : 692
  Unlocked on: 695
drivers/media/platform/raspberrypi/pisp_be/pisp_be.c:695 pispbe_schedule_any() warn: inconsistent returns 'flags'.
  Locked on  : 692
  Unlocked on: 695

which are triggered by the locking/unlocking scheme implemented by the job scheduler that we know it's a tad weird. I'm not sure we need to fix it, just reporting the WARNS and will look into details.

Steps to reproduce the behaviour

https://git.kernel.org/pub/scm/linux/kernel/git/jmondi/linux.git/log/?h=jmondi/rpi5-be-upstream/libcamera

Device (s)

Raspberry Pi 5

System

Windows CE :p

Logs

No response

Additional context

No response

[6by9]

The first and probably most evident one is the pre-allocation of the video device numbers. https://github.com/raspberrypi/linux/blob/rpi-6.1.y/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c#L27 This is not gonna fly in mainline most probably and could either be solved with changes to the framework or a patch could be kept downstream.

libcamera should be finding the relevant nodes via Media Controller, so it shouldn't care.

It's more if you plug a USB camera in, most simple apps assume it'll be /dev/video0, therefore having platform devices jump early in and register as /dev/video0 breaks those apps. Yes the CFE may also get in early and claim /dev/video0, but at least that is semi-logical as you've added a camera.

As long as libcamera works, then keeping the request for higher numbered nodes downstream is fine by me (it's 3 lines).

[jmondi]

The first and probably most evident one is the pre-allocation of the video device numbers. https://github.com/raspberrypi/linux/blob/rpi-6.1.y/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c#L27 This is not gonna fly in mainline most probably and could either be solved with changes to the framework or a patch could be kept downstream.

libcamera should be finding the relevant nodes via Media Controller, so it shouldn't care.

It's more if you plug a USB camera in, most simple apps assume it'll be /dev/video0, therefore having platform devices jump early in and register as /dev/video0 breaks those apps. Yes the CFE may also get in early and claim /dev/video0, but at least that is semi-logical as you've added a camera.

As long as libcamera works, then keeping the request for higher numbered nodes downstream is fine by me (it's 3 lines).

Yeah not an issue for libcamera, it was just a not to know if you're fine keeping it downstream, as I understand you don't want your USB camera users be disappointed by cheese not working anymore

[naushir]

The first and probably most evident one is the pre-allocation of the video device numbers. https://github.com/raspberrypi/linux/blob/rpi-6.1.y/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c#L27 This is not gonna fly in mainline most probably and could either be solved with changes to the framework or a patch could be kept downstream.

libcamera should be finding the relevant nodes via Media Controller, so it shouldn't care.

It's more if you plug a USB camera in, most simple apps assume it'll be /dev/video0, therefore having platform devices jump early in and register as /dev/video0 breaks those apps. Yes the CFE may also get in early and claim /dev/video0, but at least that is semi-logical as you've added a camera.

As long as libcamera works, then keeping the request for higher numbered nodes downstream is fine by me (it's 3 lines).

I'm fine with this as well. We can keep this as a downstream only patch.

[naushir]

Secondly there's the question about why some formats have a JPEG colorspace assigned even if we know such colorspace is reserved for JPEG formats (I know there's a long discussion and I had to read it over again).

fail: v4l2-test-formats.cpp(360): pixelformat != V4L2_PIX_FMT_JPEG && pixelformat != V4L2_PIX_FMT_MJPEG && colorspace == V4L2_COLORSPACE_JPEG

There's great confusion there, but basically the idea is to avoid using V4L2_COLORSPACE_JPEG for non-JPEG formats in favor of specifying all colorimetry fields explicitly.

I would simply drop JPEG there to avoid questions on the list and use V4L2_COLORSPACE_SMPTE170M as it's done for the YVU format variants (even if V4L2_COLORSPACE_SMPTE170M is different from V4L2_COLORSPACE_JPEG as it specifies a different transfer function). As David said, we're going to set all three colorspace-related fields explicitly from userspace so this doesn't actually matter too much ?

	{
		.fourcc		    = V4L2_PIX_FMT_YUV420,
		/* 128 alignment to ensure U/V planes are 64 byte aligned. */
		.align		    = 128,
		.bit_depth	    = 8,
		.plane_factor	    = { P3(1), P3(0.25), P3(0.25) },
		.num_planes	    = 1,
		.colorspace_mask    = V4L2_COLORSPACE_MASK_ALL_SRGB,
		.colorspace_default = V4L2_COLORSPACE_JPEG,
	},
	{
		.fourcc		    = V4L2_PIX_FMT_YVU420,
		/* 128 alignment to ensure U/V planes are 64 byte aligned. */
		.align		    = 128,
		.bit_depth	    = 8,
		.plane_factor	    = { P3(1), P3(0.25), P3(0.25) },
		.num_planes	    = 1,
		.colorspace_mask    = V4L2_COLORSPACE_MASK_ALL_SRGB,
		.colorspace_default = V4L2_COLORSPACE_SMPTE170M,
	},

Let's change to V4L2_COLORSPACE_SMPTE170M, @davidplowman and objections?

[davidplowman]

I guess V4L2_COLORSPACE_SMPTE170M is fine, I expect we always set what we want explicitly. There's more of a tendency to use sYCC as a colour space these days, and V4L2's "JPEG" colour space is in some respects a placeholder for that because V4L2 doesn't have (am I right?) anything for sYCC. But the handling of all this stuff is so super-confused I would be happy with (1) anything that doesn't get complained about so long as (2) we always set what we want explicitly.

[jmondi]

I've now noticed there is no uapi file for the back end config buffer.

I presume it's in libpisp ? I guess we need one for kernel, and I'm wondering given the ISP is fully documented how much we need to document in the kernel.

Also @pinchartl @tomba

[naushir]

The UAPI file is effectively here: https://github.com/raspberrypi/linux/blob/rpi-6.1.y/drivers/media/platform/raspberrypi/pisp_be/pisp_be_config.h

This file is duplicated in libpisp/libcamera.

[jmondi]

The UAPI file is effectively here: https://github.com/raspberrypi/linux/blob/rpi-6.1.y/drivers/media/platform/raspberrypi/pisp_be/pisp_be_config.h

This file is duplicated in libpisp/libcamera.

I'll move it to uapi then

[jmondi]

V4L2_PIX_FMT_RPI_BE is said to be /* Opaque BE format for HW verification. */

Does it need to be upstreamed ?

[jmondi]

Also I recall that there was a documentation draft for the RAW compressed formats but I don't seem to be able to find it. Any pointer ?

[naushir]

V4L2_PIX_FMT_RPI_BE is said to be /* Opaque BE format for HW verification. */

Does it need to be upstreamed ?

Ideally yes if it can. The definition of the opaque format is given in the spec document, so hopefully there should be no problem with it.

[naushir]

Also I recall that there was a documentation draft for the RAW compressed formats but I don't seem to be able to find it. Any pointer ?

There was an email from @njhollinghurst describing it, but we also have source code for the decoding implementation at https://github.com/raspberrypi/rpicam-apps/blob/main/image/dng.cpp

[jmondi]

V4L2_PIX_FMT_RPI_BE is said to be /* Opaque BE format for HW verification. */ Does it need to be upstreamed ?

Ideally yes if it can. The definition of the opaque format is given in the spec document, so hopefully there should be no problem with it.

Could you point me to the documentation of the opaque format please so I can make kernel doc out of it ? I don't find it explicitly mentioned in the PiSP spec