[v1,2/2] libcamera: formats: Add PiSP specific image and config buffer formats
diff mbox series

Message ID 20240215132710.810-3-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: PiSP image formats
Related show

Commit Message

Naushir Patuck Feb. 15, 2024, 1:27 p.m. UTC
Add the Raspberry Pi 5 PiSP specific compressed Bayer format types 1/2:
- V4L2_PIX_FMT_PISP_COMP1_xxx
- V4L2_PIX_FMT_PISP_COMP2_xxx

Add the Raspberry Pi 5 PiSP Backend config format:
- V4L2_META_FMT_RPI_BE_CFG

Additionally, we also extend libcamera format handlers to support 16-bit
Bayer formats across the media bus.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/internal/bayer_format.h |  2 +
 include/linux/drm_fourcc.h                |  4 ++
 include/linux/videodev2.h                 | 15 +++++++
 src/libcamera/bayer_format.cpp            | 18 ++++++++
 src/libcamera/formats.cpp                 | 51 ++++++++++++++++++++++-
 src/libcamera/formats.yaml                | 16 +++++++
 src/libcamera/v4l2_pixelformat.cpp        | 10 +++++
 src/libcamera/v4l2_subdevice.cpp          |  4 ++
 8 files changed, 119 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi Feb. 26, 2024, 1:49 p.m. UTC | #1
Hi Naush

On Thu, Feb 15, 2024 at 01:27:10PM +0000, Naushir Patuck wrote:
> Add the Raspberry Pi 5 PiSP specific compressed Bayer format types 1/2:
> - V4L2_PIX_FMT_PISP_COMP1_xxx
> - V4L2_PIX_FMT_PISP_COMP2_xxx
>
> Add the Raspberry Pi 5 PiSP Backend config format:
> - V4L2_META_FMT_RPI_BE_CFG
>
> Additionally, we also extend libcamera format handlers to support 16-bit
> Bayer formats across the media bus.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/internal/bayer_format.h |  2 +
>  include/linux/drm_fourcc.h                |  4 ++
>  include/linux/videodev2.h                 | 15 +++++++
>  src/libcamera/bayer_format.cpp            | 18 ++++++++
>  src/libcamera/formats.cpp                 | 51 ++++++++++++++++++++++-
>  src/libcamera/formats.yaml                | 16 +++++++
>  src/libcamera/v4l2_pixelformat.cpp        | 10 +++++
>  src/libcamera/v4l2_subdevice.cpp          |  4 ++
>  8 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> index 78ba3969913d..164743f7e9f6 100644
> --- a/include/libcamera/internal/bayer_format.h
> +++ b/include/libcamera/internal/bayer_format.h
> @@ -34,6 +34,8 @@ public:
>  		None = 0,
>  		CSI2 = 1,
>  		IPU3 = 2,
> +		PISP1 = 3,
> +		PISP2 = 4,
>  	};
>
>  	constexpr BayerFormat()
> diff --git a/include/linux/drm_fourcc.h b/include/linux/drm_fourcc.h
> index 4ee421b95730..eff27fbe5a1e 100644
> --- a/include/linux/drm_fourcc.h
> +++ b/include/linux/drm_fourcc.h
> @@ -490,6 +490,7 @@ extern "C" {
>  #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
>  #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
>  #define DRM_FORMAT_MOD_VENDOR_MIPI 0x0b
> +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0c
>
>  /* add more to the end as needed */
>
> @@ -1670,6 +1671,9 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
>   */
>  #define MIPI_FORMAT_MOD_CSI2_PACKED fourcc_mod_code(MIPI, 1)
>
> +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)
> +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)
> +

I'll send a patch to DRM/KMS for these formats as well

>  #if defined(__cplusplus)
>  }
>  #endif
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index ba48d2c89726..59af6f794680 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -789,6 +789,18 @@ struct v4l2_pix_format {
>  #define V4L2_PIX_FMT_IPU3_SGRBG10	v4l2_fourcc('i', 'p', '3', 'G') /* IPU3 packed 10-bit GRBG bayer */
>  #define V4L2_PIX_FMT_IPU3_SRGGB10	v4l2_fourcc('i', 'p', '3', 'r') /* IPU3 packed 10-bit RGGB bayer */
>
> +/* The pixel format for all our buffers (the precise format is found in the config buffer). */
> +#define V4L2_PIX_FMT_PISP_COMP1_RGGB	v4l2_fourcc('P', 'C', '1', 'R')
> +#define V4L2_PIX_FMT_PISP_COMP1_GRBG	v4l2_fourcc('P', 'C', '1', 'G')
> +#define V4L2_PIX_FMT_PISP_COMP1_GBRG	v4l2_fourcc('P', 'C', '1', 'g')
> +#define V4L2_PIX_FMT_PISP_COMP1_BGGR	v4l2_fourcc('P', 'C', '1', 'B')
> +#define V4L2_PIX_FMT_PISP_COMP1_MONO	v4l2_fourcc('P', 'C', '1', 'M')
> +#define V4L2_PIX_FMT_PISP_COMP2_RGGB	v4l2_fourcc('P', 'C', '2', 'R')
> +#define V4L2_PIX_FMT_PISP_COMP2_GRBG	v4l2_fourcc('P', 'C', '2', 'G')
> +#define V4L2_PIX_FMT_PISP_COMP2_GBRG	v4l2_fourcc('P', 'C', '2', 'g')
> +#define V4L2_PIX_FMT_PISP_COMP2_BGGR	v4l2_fourcc('P', 'C', '2', 'B')
> +#define V4L2_PIX_FMT_PISP_COMP2_MONO	v4l2_fourcc('P', 'C', '2', 'M')
> +

I would use here what has been sent upstream for the BE support

/* Raspberry Pi PiSP compressed formats. */
#define V4L2_PIX_FMT_PISP_COMP1_RGGB	v4l2_fourcc('P', 'C', '1', 'R') /* PiSP 8-bit mode 1 compressed RGGB bayer */
#define V4L2_PIX_FMT_PISP_COMP1_GRBG	v4l2_fourcc('P', 'C', '1', 'G') /* PiSP 8-bit mode 1 compressed GRBG bayer */
#define V4L2_PIX_FMT_PISP_COMP1_GBRG	v4l2_fourcc('P', 'C', '1', 'g') /* PiSP 8-bit mode 1 compressed GBRG bayer */
#define V4L2_PIX_FMT_PISP_COMP1_BGGR	v4l2_fourcc('P', 'C', '1', 'B') /* PiSP 8-bit mode 1 compressed BGGR bayer */
#define V4L2_PIX_FMT_PISP_COMP1_MONO	v4l2_fourcc('P', 'C', '1', 'M') /* PiSP 8-bit mode 1 compressed monochrome */
#define V4L2_PIX_FMT_PISP_COMP2_RGGB	v4l2_fourcc('P', 'C', '2', 'R') /* PiSP 8-bit mode 2 compressed RGGB bayer */
#define V4L2_PIX_FMT_PISP_COMP2_GRBG	v4l2_fourcc('P', 'C', '2', 'G') /* PiSP 8-bit mode 2 compressed GRBG bayer */
#define V4L2_PIX_FMT_PISP_COMP2_GBRG	v4l2_fourcc('P', 'C', '2', 'g') /* PiSP 8-bit mode 2 compressed GBRG bayer */
#define V4L2_PIX_FMT_PISP_COMP2_BGGR	v4l2_fourcc('P', 'C', '2', 'B') /* PiSP 8-bit mode 2 compressed BGGR bayer */
#define V4L2_PIX_FMT_PISP_COMP2_MONO	v4l2_fourcc('P', 'C', '2', 'M') /* PiSP 8-bit mode 2 compressed monochrome */

To make it easier to update it when these will land in upstream

>  /* SDR formats - used only for Software Defined Radio devices */
>  #define V4L2_SDR_FMT_CU8          v4l2_fourcc('C', 'U', '0', '8') /* IQ u8 */
>  #define V4L2_SDR_FMT_CU16LE       v4l2_fourcc('C', 'U', '1', '6') /* IQ u16le */
> @@ -818,6 +830,9 @@ struct v4l2_pix_format {
>  #define V4L2_META_FMT_RK_ISP1_PARAMS	v4l2_fourcc('R', 'K', '1', 'P') /* Rockchip ISP1 3A Parameters */
>  #define V4L2_META_FMT_RK_ISP1_STAT_3A	v4l2_fourcc('R', 'K', '1', 'S') /* Rockchip ISP1 3A Statistics */
>
> +/* Vendor specific - used for RaspberryPi PiSP */
> +#define V4L2_META_FMT_RPI_BE_CFG v4l2_fourcc('R', 'P', 'B', 'C') /* PiSP BE configuration */
>  /* priv field value to indicates that subsequent fields are valid. */
>  #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe
>
> diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> index 20aedfa6d925..333b1117f531 100644
> --- a/src/libcamera/bayer_format.cpp
> +++ b/src/libcamera/bayer_format.cpp
> @@ -164,6 +164,14 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
>  		{ formats::SGRBG16, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG16) } },
>  	{ { BayerFormat::RGGB, 16, BayerFormat::Packing::None },
>  		{ formats::SRGGB16, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) } },
> +	{ { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 },
> +		{ formats::BGGR16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR) } },
> +	{ { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 },
> +		{ formats::GBRG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG) } },
> +	{ { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 },
> +		{ formats::GRBG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG) } },
> +	{ { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 },
> +		{ formats::RGGB16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } },
>  	{ { BayerFormat::MONO, 8, BayerFormat::Packing::None },
>  		{ formats::R8, V4L2PixelFormat(V4L2_PIX_FMT_GREY) } },
>  	{ { BayerFormat::MONO, 10, BayerFormat::Packing::None },
> @@ -174,6 +182,8 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
>  		{ formats::R12, V4L2PixelFormat(V4L2_PIX_FMT_Y12) } },
>  	{ { BayerFormat::MONO, 16, BayerFormat::Packing::None },
>  		{ formats::R16, V4L2PixelFormat(V4L2_PIX_FMT_Y16) } },
> +	{ { BayerFormat::MONO, 16, BayerFormat::Packing::PISP1 },
> +		{ formats::MONO_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO) } },

Should the COMP2 versions be added as well ?

>  };
>
>  const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> @@ -209,6 +219,10 @@ const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
>  	{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::None } },
>  	{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::None } },
>  	{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::None } },
> +	{ MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 } },
> +	{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 } },
> +	{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 } },
> +	{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 } },

mbusCodeToBayer is an unordered_map which
"is an associative container that contains key-value pairs with unique
keys."

I'm afraid adding a new entry with the same key overwrite the existing
one ?

We already had issues with this map, as we had similar problems with the
_LE and _BE version of RGB565 which resolv to the same BayerFormat

	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },

In general, we know that resolving mbus_codes to a unique format is
ill-defined. How are you using these entries in the pi5 support ?

>  	{ MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::Packing::None } },
>  	{ MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::Packing::None } },
>  	{ MEDIA_BUS_FMT_Y12_1X12, { BayerFormat::MONO, 12, BayerFormat::Packing::None } },
> @@ -303,6 +317,10 @@ std::ostream &operator<<(std::ostream &out, const BayerFormat &f)
>  		out << "-CSI2P";
>  	else if (f.packing == BayerFormat::Packing::IPU3)
>  		out << "-IPU3P";
> +	else if (f.packing == BayerFormat::Packing::PISP1)
> +		out << "-PISP1";
> +	else if (f.packing == BayerFormat::Packing::PISP2)
> +		out << "-PISP2";
>
>  	return out;
>  }
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index a674f4179cc8..e603a7eda579 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -547,6 +547,16 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.pixelsPerGroup = 1,
>  		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
>  	} },
> +	{ formats::MONO_PISP_COMP1, {
> +		.name = "MONO_PISP_COMP1",
> +		.format = formats::MONO_PISP_COMP1,
> +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO), },
> +		.bitsPerPixel = 16,
> +		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> +		.packed = true,
> +		.pixelsPerGroup = 1,
> +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> +	} },
>
>  	/* Bayer formats. */
>  	{ formats::SBGGR8, {
> @@ -910,7 +920,46 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  		.pixelsPerGroup = 25,
>  		.planes = {{ { 32, 1 }, { 0, 0 }, { 0, 0 } }},
>  	} },
> -
> +	{ formats::BGGR16_PISP_COMP1, {
> +		.name = "BGGR16_PISP_COMP1",
> +		.format = formats::BGGR16_PISP_COMP1,
> +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
> +		.bitsPerPixel = 16,
> +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> +		.packed = true,
> +		.pixelsPerGroup = 2,
> +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> +	} },

For regular 16-bit formats this should be

		.pixelsPerGroup = 2,
		.planes = {{ { 4, 1 }, { 0, 0 }, { 0, 0 } }},

As 2 pixels of 16 bits each consume 4 bytes. Is this {2, 1}
intentional and due to compression ?

> +	{ formats::GBRG16_PISP_COMP1, {
> +		.name = "GBRG16_PISP_COMP1",
> +		.format = formats::GBRG16_PISP_COMP1,
> +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
> +		.bitsPerPixel = 16,
> +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> +		.packed = true,
> +		.pixelsPerGroup = 2,
> +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> +	} },
> +	{ formats::GRBG16_PISP_COMP1, {
> +		.name = "GRBG16_PISP_COMP1",
> +		.format = formats::GRBG16_PISP_COMP1,
> +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
> +		.bitsPerPixel = 16,
> +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> +		.packed = true,
> +		.pixelsPerGroup = 2,
> +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> +	} },
> +	{ formats::RGGB16_PISP_COMP1, {
> +		.name = "RGGB16_PISP_COMP1",
> +		.format = formats::RGGB16_PISP_COMP1,
> +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
> +		.bitsPerPixel = 16,
> +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> +		.packed = true,
> +		.pixelsPerGroup = 2,
> +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> +	} },
>  	/* Compressed formats. */
>  	{ formats::MJPEG, {
>  		.name = "MJPEG",
> diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
> index bde2cc803b98..f6df721243d0 100644
> --- a/src/libcamera/formats.yaml
> +++ b/src/libcamera/formats.yaml
> @@ -190,4 +190,20 @@ formats:
>    - SBGGR10_IPU3:
>        fourcc: DRM_FORMAT_SBGGR10
>        mod: IPU3_FORMAT_MOD_PACKED
> +
> +  - RGGB16_PISP_COMP1:
> +      fourcc: DRM_FORMAT_SRGGB16
> +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> +  - GRBG16_PISP_COMP1:
> +      fourcc: DRM_FORMAT_SGRBG16
> +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> +  - GBRG16_PISP_COMP1:
> +      fourcc: DRM_FORMAT_SGBRG16
> +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> +  - BGGR16_PISP_COMP1:
> +      fourcc: DRM_FORMAT_SBGGR16
> +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> +  - MONO_PISP_COMP1:
> +      fourcc: DRM_FORMAT_R16
> +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
>  ...
> diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
> index efb6f2940235..47baaf60199d 100644
> --- a/src/libcamera/v4l2_pixelformat.cpp
> +++ b/src/libcamera/v4l2_pixelformat.cpp
> @@ -207,6 +207,16 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{
>  		{ formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
>  	{ V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
>  		{ formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
> +		{ formats::BGGR16_PISP_COMP1, "16-bit Bayer BGBG/GRGR PiSP Compress Mode 1" } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
> +		{ formats::GBRG16_PISP_COMP1, "16-bit Bayer GBGB/RGRG PiSP Compress Mode 1" } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
> +		{ formats::GRBG16_PISP_COMP1, "16-bit Bayer GRGR/BGBG PiSP Compress Mode 1" } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
> +		{ formats::RGGB16_PISP_COMP1, "16-bit Bayer RGRG/GBGB PiSP Compress Mode 1" } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
> +		{ formats::MONO_PISP_COMP1, "16-bit Mono PiSP Compress Mode 1" } },
>
>  	/* Compressed formats. */
>  	{ V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 6d0785b7b484..aea90abaf9ef 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -134,6 +134,10 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
>  	{ MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
>  	{ MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
>  	{ MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> +	{ MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> +	{ MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> +	{ MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> +	{ MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16", PixelFormatInfo::ColourEncodingRAW } },
>  	/* \todo Clarify colour encoding for HSV formats */
>  	{ MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32", PixelFormatInfo::ColourEncodingRGB } },
>  	{ MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8", PixelFormatInfo::ColourEncodingYUV } },
> --
> 2.34.1
>
Jacopo Mondi Feb. 26, 2024, 1:55 p.m. UTC | #2
Actually...

On Mon, Feb 26, 2024 at 02:49:09PM +0100, Jacopo Mondi wrote:
> Hi Naush
>
> On Thu, Feb 15, 2024 at 01:27:10PM +0000, Naushir Patuck wrote:
> > Add the Raspberry Pi 5 PiSP specific compressed Bayer format types 1/2:
> > - V4L2_PIX_FMT_PISP_COMP1_xxx
> > - V4L2_PIX_FMT_PISP_COMP2_xxx
> >
> > Add the Raspberry Pi 5 PiSP Backend config format:
> > - V4L2_META_FMT_RPI_BE_CFG
> >
> > Additionally, we also extend libcamera format handlers to support 16-bit
> > Bayer formats across the media bus.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/internal/bayer_format.h |  2 +
> >  include/linux/drm_fourcc.h                |  4 ++
> >  include/linux/videodev2.h                 | 15 +++++++
> >  src/libcamera/bayer_format.cpp            | 18 ++++++++
> >  src/libcamera/formats.cpp                 | 51 ++++++++++++++++++++++-
> >  src/libcamera/formats.yaml                | 16 +++++++
> >  src/libcamera/v4l2_pixelformat.cpp        | 10 +++++
> >  src/libcamera/v4l2_subdevice.cpp          |  4 ++
> >  8 files changed, 119 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > index 78ba3969913d..164743f7e9f6 100644
> > --- a/include/libcamera/internal/bayer_format.h
> > +++ b/include/libcamera/internal/bayer_format.h
> > @@ -34,6 +34,8 @@ public:
> >  		None = 0,
> >  		CSI2 = 1,
> >  		IPU3 = 2,
> > +		PISP1 = 3,
> > +		PISP2 = 4,
> >  	};
> >
> >  	constexpr BayerFormat()
> > diff --git a/include/linux/drm_fourcc.h b/include/linux/drm_fourcc.h
> > index 4ee421b95730..eff27fbe5a1e 100644
> > --- a/include/linux/drm_fourcc.h
> > +++ b/include/linux/drm_fourcc.h
> > @@ -490,6 +490,7 @@ extern "C" {
> >  #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
> >  #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
> >  #define DRM_FORMAT_MOD_VENDOR_MIPI 0x0b
> > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0c
> >
> >  /* add more to the end as needed */
> >
> > @@ -1670,6 +1671,9 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
> >   */
> >  #define MIPI_FORMAT_MOD_CSI2_PACKED fourcc_mod_code(MIPI, 1)
> >
> > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)
> > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)
> > +
>
> I'll send a patch to DRM/KMS for these formats as well
>

Can these be upstreamed if no DRM format is actually using them ? Or
should we add a DRM FourCC for the compressed formats, even if they
won't be used outside of the PiSP FE/BE loop ?

> >  #if defined(__cplusplus)
> >  }
> >  #endif
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index ba48d2c89726..59af6f794680 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -789,6 +789,18 @@ struct v4l2_pix_format {
> >  #define V4L2_PIX_FMT_IPU3_SGRBG10	v4l2_fourcc('i', 'p', '3', 'G') /* IPU3 packed 10-bit GRBG bayer */
> >  #define V4L2_PIX_FMT_IPU3_SRGGB10	v4l2_fourcc('i', 'p', '3', 'r') /* IPU3 packed 10-bit RGGB bayer */
> >
> > +/* The pixel format for all our buffers (the precise format is found in the config buffer). */
> > +#define V4L2_PIX_FMT_PISP_COMP1_RGGB	v4l2_fourcc('P', 'C', '1', 'R')
> > +#define V4L2_PIX_FMT_PISP_COMP1_GRBG	v4l2_fourcc('P', 'C', '1', 'G')
> > +#define V4L2_PIX_FMT_PISP_COMP1_GBRG	v4l2_fourcc('P', 'C', '1', 'g')
> > +#define V4L2_PIX_FMT_PISP_COMP1_BGGR	v4l2_fourcc('P', 'C', '1', 'B')
> > +#define V4L2_PIX_FMT_PISP_COMP1_MONO	v4l2_fourcc('P', 'C', '1', 'M')
> > +#define V4L2_PIX_FMT_PISP_COMP2_RGGB	v4l2_fourcc('P', 'C', '2', 'R')
> > +#define V4L2_PIX_FMT_PISP_COMP2_GRBG	v4l2_fourcc('P', 'C', '2', 'G')
> > +#define V4L2_PIX_FMT_PISP_COMP2_GBRG	v4l2_fourcc('P', 'C', '2', 'g')
> > +#define V4L2_PIX_FMT_PISP_COMP2_BGGR	v4l2_fourcc('P', 'C', '2', 'B')
> > +#define V4L2_PIX_FMT_PISP_COMP2_MONO	v4l2_fourcc('P', 'C', '2', 'M')
> > +
>
> I would use here what has been sent upstream for the BE support
>
> /* Raspberry Pi PiSP compressed formats. */
> #define V4L2_PIX_FMT_PISP_COMP1_RGGB	v4l2_fourcc('P', 'C', '1', 'R') /* PiSP 8-bit mode 1 compressed RGGB bayer */
> #define V4L2_PIX_FMT_PISP_COMP1_GRBG	v4l2_fourcc('P', 'C', '1', 'G') /* PiSP 8-bit mode 1 compressed GRBG bayer */
> #define V4L2_PIX_FMT_PISP_COMP1_GBRG	v4l2_fourcc('P', 'C', '1', 'g') /* PiSP 8-bit mode 1 compressed GBRG bayer */
> #define V4L2_PIX_FMT_PISP_COMP1_BGGR	v4l2_fourcc('P', 'C', '1', 'B') /* PiSP 8-bit mode 1 compressed BGGR bayer */
> #define V4L2_PIX_FMT_PISP_COMP1_MONO	v4l2_fourcc('P', 'C', '1', 'M') /* PiSP 8-bit mode 1 compressed monochrome */
> #define V4L2_PIX_FMT_PISP_COMP2_RGGB	v4l2_fourcc('P', 'C', '2', 'R') /* PiSP 8-bit mode 2 compressed RGGB bayer */
> #define V4L2_PIX_FMT_PISP_COMP2_GRBG	v4l2_fourcc('P', 'C', '2', 'G') /* PiSP 8-bit mode 2 compressed GRBG bayer */
> #define V4L2_PIX_FMT_PISP_COMP2_GBRG	v4l2_fourcc('P', 'C', '2', 'g') /* PiSP 8-bit mode 2 compressed GBRG bayer */
> #define V4L2_PIX_FMT_PISP_COMP2_BGGR	v4l2_fourcc('P', 'C', '2', 'B') /* PiSP 8-bit mode 2 compressed BGGR bayer */
> #define V4L2_PIX_FMT_PISP_COMP2_MONO	v4l2_fourcc('P', 'C', '2', 'M') /* PiSP 8-bit mode 2 compressed monochrome */
>
> To make it easier to update it when these will land in upstream
>
> >  /* SDR formats - used only for Software Defined Radio devices */
> >  #define V4L2_SDR_FMT_CU8          v4l2_fourcc('C', 'U', '0', '8') /* IQ u8 */
> >  #define V4L2_SDR_FMT_CU16LE       v4l2_fourcc('C', 'U', '1', '6') /* IQ u16le */
> > @@ -818,6 +830,9 @@ struct v4l2_pix_format {
> >  #define V4L2_META_FMT_RK_ISP1_PARAMS	v4l2_fourcc('R', 'K', '1', 'P') /* Rockchip ISP1 3A Parameters */
> >  #define V4L2_META_FMT_RK_ISP1_STAT_3A	v4l2_fourcc('R', 'K', '1', 'S') /* Rockchip ISP1 3A Statistics */
> >
> > +/* Vendor specific - used for RaspberryPi PiSP */
> > +#define V4L2_META_FMT_RPI_BE_CFG v4l2_fourcc('R', 'P', 'B', 'C') /* PiSP BE configuration */
> >  /* priv field value to indicates that subsequent fields are valid. */
> >  #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe
> >
> > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > index 20aedfa6d925..333b1117f531 100644
> > --- a/src/libcamera/bayer_format.cpp
> > +++ b/src/libcamera/bayer_format.cpp
> > @@ -164,6 +164,14 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
> >  		{ formats::SGRBG16, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG16) } },
> >  	{ { BayerFormat::RGGB, 16, BayerFormat::Packing::None },
> >  		{ formats::SRGGB16, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) } },
> > +	{ { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 },
> > +		{ formats::BGGR16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR) } },
> > +	{ { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 },
> > +		{ formats::GBRG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG) } },
> > +	{ { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 },
> > +		{ formats::GRBG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG) } },
> > +	{ { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 },
> > +		{ formats::RGGB16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } },
> >  	{ { BayerFormat::MONO, 8, BayerFormat::Packing::None },
> >  		{ formats::R8, V4L2PixelFormat(V4L2_PIX_FMT_GREY) } },
> >  	{ { BayerFormat::MONO, 10, BayerFormat::Packing::None },
> > @@ -174,6 +182,8 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
> >  		{ formats::R12, V4L2PixelFormat(V4L2_PIX_FMT_Y12) } },
> >  	{ { BayerFormat::MONO, 16, BayerFormat::Packing::None },
> >  		{ formats::R16, V4L2PixelFormat(V4L2_PIX_FMT_Y16) } },
> > +	{ { BayerFormat::MONO, 16, BayerFormat::Packing::PISP1 },
> > +		{ formats::MONO_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO) } },
>
> Should the COMP2 versions be added as well ?
>
> >  };
> >
> >  const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> > @@ -209,6 +219,10 @@ const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> >  	{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::None } },
> >  	{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::None } },
> >  	{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::None } },
> > +	{ MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 } },
> > +	{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 } },
> > +	{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 } },
> > +	{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 } },
>
> mbusCodeToBayer is an unordered_map which
> "is an associative container that contains key-value pairs with unique
> keys."
>
> I'm afraid adding a new entry with the same key overwrite the existing
> one ?
>
> We already had issues with this map, as we had similar problems with the
> _LE and _BE version of RGB565 which resolv to the same BayerFormat
>
> 	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> 	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> 	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> 	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
>
> In general, we know that resolving mbus_codes to a unique format is
> ill-defined. How are you using these entries in the pi5 support ?
>
> >  	{ MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::Packing::None } },
> >  	{ MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::Packing::None } },
> >  	{ MEDIA_BUS_FMT_Y12_1X12, { BayerFormat::MONO, 12, BayerFormat::Packing::None } },
> > @@ -303,6 +317,10 @@ std::ostream &operator<<(std::ostream &out, const BayerFormat &f)
> >  		out << "-CSI2P";
> >  	else if (f.packing == BayerFormat::Packing::IPU3)
> >  		out << "-IPU3P";
> > +	else if (f.packing == BayerFormat::Packing::PISP1)
> > +		out << "-PISP1";
> > +	else if (f.packing == BayerFormat::Packing::PISP2)
> > +		out << "-PISP2";
> >
> >  	return out;
> >  }
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index a674f4179cc8..e603a7eda579 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -547,6 +547,16 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.pixelsPerGroup = 1,
> >  		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> >  	} },
> > +	{ formats::MONO_PISP_COMP1, {
> > +		.name = "MONO_PISP_COMP1",
> > +		.format = formats::MONO_PISP_COMP1,
> > +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO), },
> > +		.bitsPerPixel = 16,
> > +		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > +		.packed = true,
> > +		.pixelsPerGroup = 1,
> > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > +	} },
> >
> >  	/* Bayer formats. */
> >  	{ formats::SBGGR8, {
> > @@ -910,7 +920,46 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >  		.pixelsPerGroup = 25,
> >  		.planes = {{ { 32, 1 }, { 0, 0 }, { 0, 0 } }},
> >  	} },
> > -
> > +	{ formats::BGGR16_PISP_COMP1, {
> > +		.name = "BGGR16_PISP_COMP1",
> > +		.format = formats::BGGR16_PISP_COMP1,
> > +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
> > +		.bitsPerPixel = 16,
> > +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > +		.packed = true,
> > +		.pixelsPerGroup = 2,
> > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > +	} },
>
> For regular 16-bit formats this should be
>
> 		.pixelsPerGroup = 2,
> 		.planes = {{ { 4, 1 }, { 0, 0 }, { 0, 0 } }},
>
> As 2 pixels of 16 bits each consume 4 bytes. Is this {2, 1}
> intentional and due to compression ?
>
> > +	{ formats::GBRG16_PISP_COMP1, {
> > +		.name = "GBRG16_PISP_COMP1",
> > +		.format = formats::GBRG16_PISP_COMP1,
> > +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
> > +		.bitsPerPixel = 16,
> > +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > +		.packed = true,
> > +		.pixelsPerGroup = 2,
> > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > +	} },
> > +	{ formats::GRBG16_PISP_COMP1, {
> > +		.name = "GRBG16_PISP_COMP1",
> > +		.format = formats::GRBG16_PISP_COMP1,
> > +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
> > +		.bitsPerPixel = 16,
> > +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > +		.packed = true,
> > +		.pixelsPerGroup = 2,
> > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > +	} },
> > +	{ formats::RGGB16_PISP_COMP1, {
> > +		.name = "RGGB16_PISP_COMP1",
> > +		.format = formats::RGGB16_PISP_COMP1,
> > +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
> > +		.bitsPerPixel = 16,
> > +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > +		.packed = true,
> > +		.pixelsPerGroup = 2,
> > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > +	} },
> >  	/* Compressed formats. */
> >  	{ formats::MJPEG, {
> >  		.name = "MJPEG",
> > diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
> > index bde2cc803b98..f6df721243d0 100644
> > --- a/src/libcamera/formats.yaml
> > +++ b/src/libcamera/formats.yaml
> > @@ -190,4 +190,20 @@ formats:
> >    - SBGGR10_IPU3:
> >        fourcc: DRM_FORMAT_SBGGR10
> >        mod: IPU3_FORMAT_MOD_PACKED
> > +
> > +  - RGGB16_PISP_COMP1:
> > +      fourcc: DRM_FORMAT_SRGGB16
> > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > +  - GRBG16_PISP_COMP1:
> > +      fourcc: DRM_FORMAT_SGRBG16
> > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > +  - GBRG16_PISP_COMP1:
> > +      fourcc: DRM_FORMAT_SGBRG16
> > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > +  - BGGR16_PISP_COMP1:
> > +      fourcc: DRM_FORMAT_SBGGR16
> > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > +  - MONO_PISP_COMP1:
> > +      fourcc: DRM_FORMAT_R16
> > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> >  ...
> > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
> > index efb6f2940235..47baaf60199d 100644
> > --- a/src/libcamera/v4l2_pixelformat.cpp
> > +++ b/src/libcamera/v4l2_pixelformat.cpp
> > @@ -207,6 +207,16 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{
> >  		{ formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
> >  	{ V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
> >  		{ formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
> > +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
> > +		{ formats::BGGR16_PISP_COMP1, "16-bit Bayer BGBG/GRGR PiSP Compress Mode 1" } },
> > +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
> > +		{ formats::GBRG16_PISP_COMP1, "16-bit Bayer GBGB/RGRG PiSP Compress Mode 1" } },
> > +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
> > +		{ formats::GRBG16_PISP_COMP1, "16-bit Bayer GRGR/BGBG PiSP Compress Mode 1" } },
> > +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
> > +		{ formats::RGGB16_PISP_COMP1, "16-bit Bayer RGRG/GBGB PiSP Compress Mode 1" } },
> > +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
> > +		{ formats::MONO_PISP_COMP1, "16-bit Mono PiSP Compress Mode 1" } },
> >
> >  	/* Compressed formats. */
> >  	{ V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 6d0785b7b484..aea90abaf9ef 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -134,6 +134,10 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> >  	{ MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> >  	{ MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> >  	{ MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > +	{ MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > +	{ MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > +	{ MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > +	{ MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> >  	/* \todo Clarify colour encoding for HSV formats */
> >  	{ MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32", PixelFormatInfo::ColourEncodingRGB } },
> >  	{ MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8", PixelFormatInfo::ColourEncodingYUV } },
> > --
> > 2.34.1
> >
Laurent Pinchart Feb. 26, 2024, 2:06 p.m. UTC | #3
On Mon, Feb 26, 2024 at 02:55:41PM +0100, Jacopo Mondi wrote:
> Actually...
> 
> On Mon, Feb 26, 2024 at 02:49:09PM +0100, Jacopo Mondi wrote:
> > Hi Naush
> >
> > On Thu, Feb 15, 2024 at 01:27:10PM +0000, Naushir Patuck wrote:
> > > Add the Raspberry Pi 5 PiSP specific compressed Bayer format types 1/2:
> > > - V4L2_PIX_FMT_PISP_COMP1_xxx
> > > - V4L2_PIX_FMT_PISP_COMP2_xxx
> > >
> > > Add the Raspberry Pi 5 PiSP Backend config format:
> > > - V4L2_META_FMT_RPI_BE_CFG
> > >
> > > Additionally, we also extend libcamera format handlers to support 16-bit
> > > Bayer formats across the media bus.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/bayer_format.h |  2 +
> > >  include/linux/drm_fourcc.h                |  4 ++
> > >  include/linux/videodev2.h                 | 15 +++++++
> > >  src/libcamera/bayer_format.cpp            | 18 ++++++++
> > >  src/libcamera/formats.cpp                 | 51 ++++++++++++++++++++++-
> > >  src/libcamera/formats.yaml                | 16 +++++++
> > >  src/libcamera/v4l2_pixelformat.cpp        | 10 +++++
> > >  src/libcamera/v4l2_subdevice.cpp          |  4 ++
> > >  8 files changed, 119 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > > index 78ba3969913d..164743f7e9f6 100644
> > > --- a/include/libcamera/internal/bayer_format.h
> > > +++ b/include/libcamera/internal/bayer_format.h
> > > @@ -34,6 +34,8 @@ public:
> > >  		None = 0,
> > >  		CSI2 = 1,
> > >  		IPU3 = 2,
> > > +		PISP1 = 3,
> > > +		PISP2 = 4,
> > >  	};
> > >
> > >  	constexpr BayerFormat()
> > > diff --git a/include/linux/drm_fourcc.h b/include/linux/drm_fourcc.h
> > > index 4ee421b95730..eff27fbe5a1e 100644
> > > --- a/include/linux/drm_fourcc.h
> > > +++ b/include/linux/drm_fourcc.h
> > > @@ -490,6 +490,7 @@ extern "C" {
> > >  #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
> > >  #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
> > >  #define DRM_FORMAT_MOD_VENDOR_MIPI 0x0b
> > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0c
> > >
> > >  /* add more to the end as needed */
> > >
> > > @@ -1670,6 +1671,9 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
> > >   */
> > >  #define MIPI_FORMAT_MOD_CSI2_PACKED fourcc_mod_code(MIPI, 1)
> > >
> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)
> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)
> > > +
> >
> > I'll send a patch to DRM/KMS for these formats as well
> 
> Can these be upstreamed if no DRM format is actually using them ? Or
> should we add a DRM FourCC for the compressed formats, even if they
> won't be used outside of the PiSP FE/BE loop ?

I think an RFC on the dri-devel mailing list would be useful.

> > >  #if defined(__cplusplus)
> > >  }
> > >  #endif
> > > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > > index ba48d2c89726..59af6f794680 100644
> > > --- a/include/linux/videodev2.h
> > > +++ b/include/linux/videodev2.h
> > > @@ -789,6 +789,18 @@ struct v4l2_pix_format {
> > >  #define V4L2_PIX_FMT_IPU3_SGRBG10	v4l2_fourcc('i', 'p', '3', 'G') /* IPU3 packed 10-bit GRBG bayer */
> > >  #define V4L2_PIX_FMT_IPU3_SRGGB10	v4l2_fourcc('i', 'p', '3', 'r') /* IPU3 packed 10-bit RGGB bayer */
> > >
> > > +/* The pixel format for all our buffers (the precise format is found in the config buffer). */
> > > +#define V4L2_PIX_FMT_PISP_COMP1_RGGB	v4l2_fourcc('P', 'C', '1', 'R')
> > > +#define V4L2_PIX_FMT_PISP_COMP1_GRBG	v4l2_fourcc('P', 'C', '1', 'G')
> > > +#define V4L2_PIX_FMT_PISP_COMP1_GBRG	v4l2_fourcc('P', 'C', '1', 'g')
> > > +#define V4L2_PIX_FMT_PISP_COMP1_BGGR	v4l2_fourcc('P', 'C', '1', 'B')
> > > +#define V4L2_PIX_FMT_PISP_COMP1_MONO	v4l2_fourcc('P', 'C', '1', 'M')
> > > +#define V4L2_PIX_FMT_PISP_COMP2_RGGB	v4l2_fourcc('P', 'C', '2', 'R')
> > > +#define V4L2_PIX_FMT_PISP_COMP2_GRBG	v4l2_fourcc('P', 'C', '2', 'G')
> > > +#define V4L2_PIX_FMT_PISP_COMP2_GBRG	v4l2_fourcc('P', 'C', '2', 'g')
> > > +#define V4L2_PIX_FMT_PISP_COMP2_BGGR	v4l2_fourcc('P', 'C', '2', 'B')
> > > +#define V4L2_PIX_FMT_PISP_COMP2_MONO	v4l2_fourcc('P', 'C', '2', 'M')
> > > +
> >
> > I would use here what has been sent upstream for the BE support
> >
> > /* Raspberry Pi PiSP compressed formats. */
> > #define V4L2_PIX_FMT_PISP_COMP1_RGGB	v4l2_fourcc('P', 'C', '1', 'R') /* PiSP 8-bit mode 1 compressed RGGB bayer */
> > #define V4L2_PIX_FMT_PISP_COMP1_GRBG	v4l2_fourcc('P', 'C', '1', 'G') /* PiSP 8-bit mode 1 compressed GRBG bayer */
> > #define V4L2_PIX_FMT_PISP_COMP1_GBRG	v4l2_fourcc('P', 'C', '1', 'g') /* PiSP 8-bit mode 1 compressed GBRG bayer */
> > #define V4L2_PIX_FMT_PISP_COMP1_BGGR	v4l2_fourcc('P', 'C', '1', 'B') /* PiSP 8-bit mode 1 compressed BGGR bayer */
> > #define V4L2_PIX_FMT_PISP_COMP1_MONO	v4l2_fourcc('P', 'C', '1', 'M') /* PiSP 8-bit mode 1 compressed monochrome */
> > #define V4L2_PIX_FMT_PISP_COMP2_RGGB	v4l2_fourcc('P', 'C', '2', 'R') /* PiSP 8-bit mode 2 compressed RGGB bayer */
> > #define V4L2_PIX_FMT_PISP_COMP2_GRBG	v4l2_fourcc('P', 'C', '2', 'G') /* PiSP 8-bit mode 2 compressed GRBG bayer */
> > #define V4L2_PIX_FMT_PISP_COMP2_GBRG	v4l2_fourcc('P', 'C', '2', 'g') /* PiSP 8-bit mode 2 compressed GBRG bayer */
> > #define V4L2_PIX_FMT_PISP_COMP2_BGGR	v4l2_fourcc('P', 'C', '2', 'B') /* PiSP 8-bit mode 2 compressed BGGR bayer */
> > #define V4L2_PIX_FMT_PISP_COMP2_MONO	v4l2_fourcc('P', 'C', '2', 'M') /* PiSP 8-bit mode 2 compressed monochrome */
> >
> > To make it easier to update it when these will land in upstream
> >
> > >  /* SDR formats - used only for Software Defined Radio devices */
> > >  #define V4L2_SDR_FMT_CU8          v4l2_fourcc('C', 'U', '0', '8') /* IQ u8 */
> > >  #define V4L2_SDR_FMT_CU16LE       v4l2_fourcc('C', 'U', '1', '6') /* IQ u16le */
> > > @@ -818,6 +830,9 @@ struct v4l2_pix_format {
> > >  #define V4L2_META_FMT_RK_ISP1_PARAMS	v4l2_fourcc('R', 'K', '1', 'P') /* Rockchip ISP1 3A Parameters */
> > >  #define V4L2_META_FMT_RK_ISP1_STAT_3A	v4l2_fourcc('R', 'K', '1', 'S') /* Rockchip ISP1 3A Statistics */
> > >
> > > +/* Vendor specific - used for RaspberryPi PiSP */
> > > +#define V4L2_META_FMT_RPI_BE_CFG v4l2_fourcc('R', 'P', 'B', 'C') /* PiSP BE configuration */
> > >  /* priv field value to indicates that subsequent fields are valid. */
> > >  #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe
> > >
> > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > > index 20aedfa6d925..333b1117f531 100644
> > > --- a/src/libcamera/bayer_format.cpp
> > > +++ b/src/libcamera/bayer_format.cpp
> > > @@ -164,6 +164,14 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
> > >  		{ formats::SGRBG16, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG16) } },
> > >  	{ { BayerFormat::RGGB, 16, BayerFormat::Packing::None },
> > >  		{ formats::SRGGB16, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) } },
> > > +	{ { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 },
> > > +		{ formats::BGGR16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR) } },
> > > +	{ { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 },
> > > +		{ formats::GBRG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG) } },
> > > +	{ { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 },
> > > +		{ formats::GRBG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG) } },
> > > +	{ { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 },
> > > +		{ formats::RGGB16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } },
> > >  	{ { BayerFormat::MONO, 8, BayerFormat::Packing::None },
> > >  		{ formats::R8, V4L2PixelFormat(V4L2_PIX_FMT_GREY) } },
> > >  	{ { BayerFormat::MONO, 10, BayerFormat::Packing::None },
> > > @@ -174,6 +182,8 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
> > >  		{ formats::R12, V4L2PixelFormat(V4L2_PIX_FMT_Y12) } },
> > >  	{ { BayerFormat::MONO, 16, BayerFormat::Packing::None },
> > >  		{ formats::R16, V4L2PixelFormat(V4L2_PIX_FMT_Y16) } },
> > > +	{ { BayerFormat::MONO, 16, BayerFormat::Packing::PISP1 },
> > > +		{ formats::MONO_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO) } },
> >
> > Should the COMP2 versions be added as well ?
> >
> > >  };
> > >
> > >  const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> > > @@ -209,6 +219,10 @@ const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> > >  	{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::None } },
> > >  	{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::None } },
> > >  	{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 } },
> > > +	{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 } },
> > > +	{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 } },
> > > +	{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 } },
> >
> > mbusCodeToBayer is an unordered_map which
> > "is an associative container that contains key-value pairs with unique
> > keys."
> >
> > I'm afraid adding a new entry with the same key overwrite the existing
> > one ?
> >
> > We already had issues with this map, as we had similar problems with the
> > _LE and _BE version of RGB565 which resolv to the same BayerFormat
> >
> > 	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > 	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > 	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > 	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> >
> > In general, we know that resolving mbus_codes to a unique format is
> > ill-defined. How are you using these entries in the pi5 support ?
> >
> > >  	{ MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::Packing::None } },
> > >  	{ MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::Packing::None } },
> > >  	{ MEDIA_BUS_FMT_Y12_1X12, { BayerFormat::MONO, 12, BayerFormat::Packing::None } },
> > > @@ -303,6 +317,10 @@ std::ostream &operator<<(std::ostream &out, const BayerFormat &f)
> > >  		out << "-CSI2P";
> > >  	else if (f.packing == BayerFormat::Packing::IPU3)
> > >  		out << "-IPU3P";
> > > +	else if (f.packing == BayerFormat::Packing::PISP1)
> > > +		out << "-PISP1";
> > > +	else if (f.packing == BayerFormat::Packing::PISP2)
> > > +		out << "-PISP2";
> > >
> > >  	return out;
> > >  }
> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > index a674f4179cc8..e603a7eda579 100644
> > > --- a/src/libcamera/formats.cpp
> > > +++ b/src/libcamera/formats.cpp
> > > @@ -547,6 +547,16 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.pixelsPerGroup = 1,
> > >  		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > >  	} },
> > > +	{ formats::MONO_PISP_COMP1, {
> > > +		.name = "MONO_PISP_COMP1",
> > > +		.format = formats::MONO_PISP_COMP1,
> > > +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO), },
> > > +		.bitsPerPixel = 16,
> > > +		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > > +		.packed = true,
> > > +		.pixelsPerGroup = 1,
> > > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > +	} },
> > >
> > >  	/* Bayer formats. */
> > >  	{ formats::SBGGR8, {
> > > @@ -910,7 +920,46 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >  		.pixelsPerGroup = 25,
> > >  		.planes = {{ { 32, 1 }, { 0, 0 }, { 0, 0 } }},
> > >  	} },
> > > -
> > > +	{ formats::BGGR16_PISP_COMP1, {
> > > +		.name = "BGGR16_PISP_COMP1",
> > > +		.format = formats::BGGR16_PISP_COMP1,
> > > +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
> > > +		.bitsPerPixel = 16,
> > > +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > +		.packed = true,
> > > +		.pixelsPerGroup = 2,
> > > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > +	} },
> >
> > For regular 16-bit formats this should be
> >
> > 		.pixelsPerGroup = 2,
> > 		.planes = {{ { 4, 1 }, { 0, 0 }, { 0, 0 } }},
> >
> > As 2 pixels of 16 bits each consume 4 bytes. Is this {2, 1}
> > intentional and due to compression ?
> >
> > > +	{ formats::GBRG16_PISP_COMP1, {
> > > +		.name = "GBRG16_PISP_COMP1",
> > > +		.format = formats::GBRG16_PISP_COMP1,
> > > +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
> > > +		.bitsPerPixel = 16,
> > > +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > +		.packed = true,
> > > +		.pixelsPerGroup = 2,
> > > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > +	} },
> > > +	{ formats::GRBG16_PISP_COMP1, {
> > > +		.name = "GRBG16_PISP_COMP1",
> > > +		.format = formats::GRBG16_PISP_COMP1,
> > > +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
> > > +		.bitsPerPixel = 16,
> > > +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > +		.packed = true,
> > > +		.pixelsPerGroup = 2,
> > > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > +	} },
> > > +	{ formats::RGGB16_PISP_COMP1, {
> > > +		.name = "RGGB16_PISP_COMP1",
> > > +		.format = formats::RGGB16_PISP_COMP1,
> > > +		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
> > > +		.bitsPerPixel = 16,
> > > +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > +		.packed = true,
> > > +		.pixelsPerGroup = 2,
> > > +		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > +	} },
> > >  	/* Compressed formats. */
> > >  	{ formats::MJPEG, {
> > >  		.name = "MJPEG",
> > > diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
> > > index bde2cc803b98..f6df721243d0 100644
> > > --- a/src/libcamera/formats.yaml
> > > +++ b/src/libcamera/formats.yaml
> > > @@ -190,4 +190,20 @@ formats:
> > >    - SBGGR10_IPU3:
> > >        fourcc: DRM_FORMAT_SBGGR10
> > >        mod: IPU3_FORMAT_MOD_PACKED
> > > +
> > > +  - RGGB16_PISP_COMP1:
> > > +      fourcc: DRM_FORMAT_SRGGB16
> > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > +  - GRBG16_PISP_COMP1:
> > > +      fourcc: DRM_FORMAT_SGRBG16
> > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > +  - GBRG16_PISP_COMP1:
> > > +      fourcc: DRM_FORMAT_SGBRG16
> > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > +  - BGGR16_PISP_COMP1:
> > > +      fourcc: DRM_FORMAT_SBGGR16
> > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > +  - MONO_PISP_COMP1:
> > > +      fourcc: DRM_FORMAT_R16
> > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > >  ...
> > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
> > > index efb6f2940235..47baaf60199d 100644
> > > --- a/src/libcamera/v4l2_pixelformat.cpp
> > > +++ b/src/libcamera/v4l2_pixelformat.cpp
> > > @@ -207,6 +207,16 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{
> > >  		{ formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
> > >  	{ V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
> > >  		{ formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
> > > +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
> > > +		{ formats::BGGR16_PISP_COMP1, "16-bit Bayer BGBG/GRGR PiSP Compress Mode 1" } },
> > > +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
> > > +		{ formats::GBRG16_PISP_COMP1, "16-bit Bayer GBGB/RGRG PiSP Compress Mode 1" } },
> > > +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
> > > +		{ formats::GRBG16_PISP_COMP1, "16-bit Bayer GRGR/BGBG PiSP Compress Mode 1" } },
> > > +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
> > > +		{ formats::RGGB16_PISP_COMP1, "16-bit Bayer RGRG/GBGB PiSP Compress Mode 1" } },
> > > +	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
> > > +		{ formats::MONO_PISP_COMP1, "16-bit Mono PiSP Compress Mode 1" } },
> > >
> > >  	/* Compressed formats. */
> > >  	{ V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 6d0785b7b484..aea90abaf9ef 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -134,6 +134,10 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > >  	{ MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > >  	{ MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > >  	{ MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > +	{ MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > +	{ MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > +	{ MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > +	{ MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > >  	/* \todo Clarify colour encoding for HSV formats */
> > >  	{ MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32", PixelFormatInfo::ColourEncodingRGB } },
> > >  	{ MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8", PixelFormatInfo::ColourEncodingYUV } },
Naushir Patuck Feb. 27, 2024, 10:20 a.m. UTC | #4
Hi Jacopo,

Thanks for the feedback, and sending the updates to the DRM mailing list!

On Mon, 26 Feb 2024 at 13:49, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Thu, Feb 15, 2024 at 01:27:10PM +0000, Naushir Patuck wrote:
> > Add the Raspberry Pi 5 PiSP specific compressed Bayer format types 1/2:
> > - V4L2_PIX_FMT_PISP_COMP1_xxx
> > - V4L2_PIX_FMT_PISP_COMP2_xxx
> >
> > Add the Raspberry Pi 5 PiSP Backend config format:
> > - V4L2_META_FMT_RPI_BE_CFG
> >
> > Additionally, we also extend libcamera format handlers to support 16-bit
> > Bayer formats across the media bus.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/internal/bayer_format.h |  2 +
> >  include/linux/drm_fourcc.h                |  4 ++
> >  include/linux/videodev2.h                 | 15 +++++++
> >  src/libcamera/bayer_format.cpp            | 18 ++++++++
> >  src/libcamera/formats.cpp                 | 51 ++++++++++++++++++++++-
> >  src/libcamera/formats.yaml                | 16 +++++++
> >  src/libcamera/v4l2_pixelformat.cpp        | 10 +++++
> >  src/libcamera/v4l2_subdevice.cpp          |  4 ++
> >  8 files changed, 119 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > index 78ba3969913d..164743f7e9f6 100644
> > --- a/include/libcamera/internal/bayer_format.h
> > +++ b/include/libcamera/internal/bayer_format.h
> > @@ -34,6 +34,8 @@ public:
> >               None = 0,
> >               CSI2 = 1,
> >               IPU3 = 2,
> > +             PISP1 = 3,
> > +             PISP2 = 4,
> >       };
> >
> >       constexpr BayerFormat()
> > diff --git a/include/linux/drm_fourcc.h b/include/linux/drm_fourcc.h
> > index 4ee421b95730..eff27fbe5a1e 100644
> > --- a/include/linux/drm_fourcc.h
> > +++ b/include/linux/drm_fourcc.h
> > @@ -490,6 +490,7 @@ extern "C" {
> >  #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
> >  #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
> >  #define DRM_FORMAT_MOD_VENDOR_MIPI 0x0b
> > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0c
> >
> >  /* add more to the end as needed */
> >
> > @@ -1670,6 +1671,9 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
> >   */
> >  #define MIPI_FORMAT_MOD_CSI2_PACKED fourcc_mod_code(MIPI, 1)
> >
> > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)
> > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)
> > +
>
> I'll send a patch to DRM/KMS for these formats as well
>
> >  #if defined(__cplusplus)
> >  }
> >  #endif
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index ba48d2c89726..59af6f794680 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -789,6 +789,18 @@ struct v4l2_pix_format {
> >  #define V4L2_PIX_FMT_IPU3_SGRBG10    v4l2_fourcc('i', 'p', '3', 'G') /* IPU3 packed 10-bit GRBG bayer */
> >  #define V4L2_PIX_FMT_IPU3_SRGGB10    v4l2_fourcc('i', 'p', '3', 'r') /* IPU3 packed 10-bit RGGB bayer */
> >
> > +/* The pixel format for all our buffers (the precise format is found in the config buffer). */
> > +#define V4L2_PIX_FMT_PISP_COMP1_RGGB v4l2_fourcc('P', 'C', '1', 'R')
> > +#define V4L2_PIX_FMT_PISP_COMP1_GRBG v4l2_fourcc('P', 'C', '1', 'G')
> > +#define V4L2_PIX_FMT_PISP_COMP1_GBRG v4l2_fourcc('P', 'C', '1', 'g')
> > +#define V4L2_PIX_FMT_PISP_COMP1_BGGR v4l2_fourcc('P', 'C', '1', 'B')
> > +#define V4L2_PIX_FMT_PISP_COMP1_MONO v4l2_fourcc('P', 'C', '1', 'M')
> > +#define V4L2_PIX_FMT_PISP_COMP2_RGGB v4l2_fourcc('P', 'C', '2', 'R')
> > +#define V4L2_PIX_FMT_PISP_COMP2_GRBG v4l2_fourcc('P', 'C', '2', 'G')
> > +#define V4L2_PIX_FMT_PISP_COMP2_GBRG v4l2_fourcc('P', 'C', '2', 'g')
> > +#define V4L2_PIX_FMT_PISP_COMP2_BGGR v4l2_fourcc('P', 'C', '2', 'B')
> > +#define V4L2_PIX_FMT_PISP_COMP2_MONO v4l2_fourcc('P', 'C', '2', 'M')
> > +
>
> I would use here what has been sent upstream for the BE support
>
> /* Raspberry Pi PiSP compressed formats. */
> #define V4L2_PIX_FMT_PISP_COMP1_RGGB    v4l2_fourcc('P', 'C', '1', 'R') /* PiSP 8-bit mode 1 compressed RGGB bayer */
> #define V4L2_PIX_FMT_PISP_COMP1_GRBG    v4l2_fourcc('P', 'C', '1', 'G') /* PiSP 8-bit mode 1 compressed GRBG bayer */
> #define V4L2_PIX_FMT_PISP_COMP1_GBRG    v4l2_fourcc('P', 'C', '1', 'g') /* PiSP 8-bit mode 1 compressed GBRG bayer */
> #define V4L2_PIX_FMT_PISP_COMP1_BGGR    v4l2_fourcc('P', 'C', '1', 'B') /* PiSP 8-bit mode 1 compressed BGGR bayer */
> #define V4L2_PIX_FMT_PISP_COMP1_MONO    v4l2_fourcc('P', 'C', '1', 'M') /* PiSP 8-bit mode 1 compressed monochrome */
> #define V4L2_PIX_FMT_PISP_COMP2_RGGB    v4l2_fourcc('P', 'C', '2', 'R') /* PiSP 8-bit mode 2 compressed RGGB bayer */
> #define V4L2_PIX_FMT_PISP_COMP2_GRBG    v4l2_fourcc('P', 'C', '2', 'G') /* PiSP 8-bit mode 2 compressed GRBG bayer */
> #define V4L2_PIX_FMT_PISP_COMP2_GBRG    v4l2_fourcc('P', 'C', '2', 'g') /* PiSP 8-bit mode 2 compressed GBRG bayer */
> #define V4L2_PIX_FMT_PISP_COMP2_BGGR    v4l2_fourcc('P', 'C', '2', 'B') /* PiSP 8-bit mode 2 compressed BGGR bayer */
> #define V4L2_PIX_FMT_PISP_COMP2_MONO    v4l2_fourcc('P', 'C', '2', 'M') /* PiSP 8-bit mode 2 compressed monochrome */
>
> To make it easier to update it when these will land in upstream

Ack.

>
> >  /* SDR formats - used only for Software Defined Radio devices */
> >  #define V4L2_SDR_FMT_CU8          v4l2_fourcc('C', 'U', '0', '8') /* IQ u8 */
> >  #define V4L2_SDR_FMT_CU16LE       v4l2_fourcc('C', 'U', '1', '6') /* IQ u16le */
> > @@ -818,6 +830,9 @@ struct v4l2_pix_format {
> >  #define V4L2_META_FMT_RK_ISP1_PARAMS v4l2_fourcc('R', 'K', '1', 'P') /* Rockchip ISP1 3A Parameters */
> >  #define V4L2_META_FMT_RK_ISP1_STAT_3A        v4l2_fourcc('R', 'K', '1', 'S') /* Rockchip ISP1 3A Statistics */
> >
> > +/* Vendor specific - used for RaspberryPi PiSP */
> > +#define V4L2_META_FMT_RPI_BE_CFG v4l2_fourcc('R', 'P', 'B', 'C') /* PiSP BE configuration */
> >  /* priv field value to indicates that subsequent fields are valid. */
> >  #define V4L2_PIX_FMT_PRIV_MAGIC              0xfeedcafe
> >
> > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > index 20aedfa6d925..333b1117f531 100644
> > --- a/src/libcamera/bayer_format.cpp
> > +++ b/src/libcamera/bayer_format.cpp
> > @@ -164,6 +164,14 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
> >               { formats::SGRBG16, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG16) } },
> >       { { BayerFormat::RGGB, 16, BayerFormat::Packing::None },
> >               { formats::SRGGB16, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) } },
> > +     { { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 },
> > +             { formats::BGGR16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR) } },
> > +     { { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 },
> > +             { formats::GBRG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG) } },
> > +     { { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 },
> > +             { formats::GRBG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG) } },
> > +     { { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 },
> > +             { formats::RGGB16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } },
> >       { { BayerFormat::MONO, 8, BayerFormat::Packing::None },
> >               { formats::R8, V4L2PixelFormat(V4L2_PIX_FMT_GREY) } },
> >       { { BayerFormat::MONO, 10, BayerFormat::Packing::None },
> > @@ -174,6 +182,8 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
> >               { formats::R12, V4L2PixelFormat(V4L2_PIX_FMT_Y12) } },
> >       { { BayerFormat::MONO, 16, BayerFormat::Packing::None },
> >               { formats::R16, V4L2PixelFormat(V4L2_PIX_FMT_Y16) } },
> > +     { { BayerFormat::MONO, 16, BayerFormat::Packing::PISP1 },
> > +             { formats::MONO_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO) } },
>
> Should the COMP2 versions be added as well ?

To be honest, I doubt we will ever use COMP2, COMP1 is the better
option almost always.  But I'll add it for completeness.


>
> >  };
> >
> >  const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> > @@ -209,6 +219,10 @@ const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> >       { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::None } },
> >       { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::None } },
> >       { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::None } },
> > +     { MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 } },
> > +     { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 } },
> > +     { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 } },
> > +     { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 } },
>
> mbusCodeToBayer is an unordered_map which
> "is an associative container that contains key-value pairs with unique
> keys."
>
> I'm afraid adding a new entry with the same key overwrite the existing
> one ?

Oops you are right!  I think this came about from when we originally
had MBUS codes for the PISP comp formats, and those subsequently got
removed.

>
> We already had issues with this map, as we had similar problems with the
> _LE and _BE version of RGB565 which resolv to the same BayerFormat
>
>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
>
> In general, we know that resolving mbus_codes to a unique format is
> ill-defined. How are you using these entries in the pi5 support ?

As luck would have it, the Pi 5 pipeline handler never calls
BayerFormat::fromMbusCode() with a PISP_COMP type format so the code
never gets exercised and goes wrong!

I think the right solution is to remove MEDIA_BUS_FMT_* for the
PISP_COMP formats as they genuinely don't have MBUS codes associated
with them.

>
> >       { MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::Packing::None } },
> >       { MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::Packing::None } },
> >       { MEDIA_BUS_FMT_Y12_1X12, { BayerFormat::MONO, 12, BayerFormat::Packing::None } },
> > @@ -303,6 +317,10 @@ std::ostream &operator<<(std::ostream &out, const BayerFormat &f)
> >               out << "-CSI2P";
> >       else if (f.packing == BayerFormat::Packing::IPU3)
> >               out << "-IPU3P";
> > +     else if (f.packing == BayerFormat::Packing::PISP1)
> > +             out << "-PISP1";
> > +     else if (f.packing == BayerFormat::Packing::PISP2)
> > +             out << "-PISP2";
> >
> >       return out;
> >  }
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index a674f4179cc8..e603a7eda579 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -547,6 +547,16 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >               .pixelsPerGroup = 1,
> >               .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> >       } },
> > +     { formats::MONO_PISP_COMP1, {
> > +             .name = "MONO_PISP_COMP1",
> > +             .format = formats::MONO_PISP_COMP1,
> > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO), },
> > +             .bitsPerPixel = 16,
> > +             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > +             .packed = true,
> > +             .pixelsPerGroup = 1,
> > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > +     } },
> >
> >       /* Bayer formats. */
> >       { formats::SBGGR8, {
> > @@ -910,7 +920,46 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >               .pixelsPerGroup = 25,
> >               .planes = {{ { 32, 1 }, { 0, 0 }, { 0, 0 } }},
> >       } },
> > -
> > +     { formats::BGGR16_PISP_COMP1, {
> > +             .name = "BGGR16_PISP_COMP1",
> > +             .format = formats::BGGR16_PISP_COMP1,
> > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
> > +             .bitsPerPixel = 16,
> > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > +             .packed = true,
> > +             .pixelsPerGroup = 2,
> > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > +     } },
>
> For regular 16-bit formats this should be
>
>                 .pixelsPerGroup = 2,
>                 .planes = {{ { 4, 1 }, { 0, 0 }, { 0, 0 } }},
>
> As 2 pixels of 16 bits each consume 4 bytes. Is this {2, 1}
> intentional and due to compression ?

Yes, I used {2, 1} to denote the 8-bit/sample after compression.  Do
you think I should change to {4, 1}?

Regards,
Naush


>
> > +     { formats::GBRG16_PISP_COMP1, {
> > +             .name = "GBRG16_PISP_COMP1",
> > +             .format = formats::GBRG16_PISP_COMP1,
> > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
> > +             .bitsPerPixel = 16,
> > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > +             .packed = true,
> > +             .pixelsPerGroup = 2,
> > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > +     } },
> > +     { formats::GRBG16_PISP_COMP1, {
> > +             .name = "GRBG16_PISP_COMP1",
> > +             .format = formats::GRBG16_PISP_COMP1,
> > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
> > +             .bitsPerPixel = 16,
> > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > +             .packed = true,
> > +             .pixelsPerGroup = 2,
> > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > +     } },
> > +     { formats::RGGB16_PISP_COMP1, {
> > +             .name = "RGGB16_PISP_COMP1",
> > +             .format = formats::RGGB16_PISP_COMP1,
> > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
> > +             .bitsPerPixel = 16,
> > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > +             .packed = true,
> > +             .pixelsPerGroup = 2,
> > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > +     } },
> >       /* Compressed formats. */
> >       { formats::MJPEG, {
> >               .name = "MJPEG",
> > diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
> > index bde2cc803b98..f6df721243d0 100644
> > --- a/src/libcamera/formats.yaml
> > +++ b/src/libcamera/formats.yaml
> > @@ -190,4 +190,20 @@ formats:
> >    - SBGGR10_IPU3:
> >        fourcc: DRM_FORMAT_SBGGR10
> >        mod: IPU3_FORMAT_MOD_PACKED
> > +
> > +  - RGGB16_PISP_COMP1:
> > +      fourcc: DRM_FORMAT_SRGGB16
> > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > +  - GRBG16_PISP_COMP1:
> > +      fourcc: DRM_FORMAT_SGRBG16
> > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > +  - GBRG16_PISP_COMP1:
> > +      fourcc: DRM_FORMAT_SGBRG16
> > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > +  - BGGR16_PISP_COMP1:
> > +      fourcc: DRM_FORMAT_SBGGR16
> > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > +  - MONO_PISP_COMP1:
> > +      fourcc: DRM_FORMAT_R16
> > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> >  ...
> > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
> > index efb6f2940235..47baaf60199d 100644
> > --- a/src/libcamera/v4l2_pixelformat.cpp
> > +++ b/src/libcamera/v4l2_pixelformat.cpp
> > @@ -207,6 +207,16 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{
> >               { formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
> >       { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
> >               { formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
> > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
> > +             { formats::BGGR16_PISP_COMP1, "16-bit Bayer BGBG/GRGR PiSP Compress Mode 1" } },
> > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
> > +             { formats::GBRG16_PISP_COMP1, "16-bit Bayer GBGB/RGRG PiSP Compress Mode 1" } },
> > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
> > +             { formats::GRBG16_PISP_COMP1, "16-bit Bayer GRGR/BGBG PiSP Compress Mode 1" } },
> > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
> > +             { formats::RGGB16_PISP_COMP1, "16-bit Bayer RGRG/GBGB PiSP Compress Mode 1" } },
> > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
> > +             { formats::MONO_PISP_COMP1, "16-bit Mono PiSP Compress Mode 1" } },
> >
> >       /* Compressed formats. */
> >       { V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 6d0785b7b484..aea90abaf9ef 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -134,6 +134,10 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> >       { MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> >       { MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> >       { MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > +     { MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > +     { MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > +     { MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > +     { MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> >       /* \todo Clarify colour encoding for HSV formats */
> >       { MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32", PixelFormatInfo::ColourEncodingRGB } },
> >       { MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8", PixelFormatInfo::ColourEncodingYUV } },
> > --
> > 2.34.1
> >
Jacopo Mondi Feb. 28, 2024, 11:14 a.m. UTC | #5
Hi Naush

On Tue, Feb 27, 2024 at 10:20:40AM +0000, Naushir Patuck wrote:
> Hi Jacopo,
>
> Thanks for the feedback, and sending the updates to the DRM mailing list!
>

As usual, nothing's easy :)

> On Mon, 26 Feb 2024 at 13:49, Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Naush
> >
> > On Thu, Feb 15, 2024 at 01:27:10PM +0000, Naushir Patuck wrote:
> > > Add the Raspberry Pi 5 PiSP specific compressed Bayer format types 1/2:
> > > - V4L2_PIX_FMT_PISP_COMP1_xxx
> > > - V4L2_PIX_FMT_PISP_COMP2_xxx
> > >
> > > Add the Raspberry Pi 5 PiSP Backend config format:
> > > - V4L2_META_FMT_RPI_BE_CFG
> > >
> > > Additionally, we also extend libcamera format handlers to support 16-bit
> > > Bayer formats across the media bus.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/bayer_format.h |  2 +
> > >  include/linux/drm_fourcc.h                |  4 ++
> > >  include/linux/videodev2.h                 | 15 +++++++
> > >  src/libcamera/bayer_format.cpp            | 18 ++++++++
> > >  src/libcamera/formats.cpp                 | 51 ++++++++++++++++++++++-
> > >  src/libcamera/formats.yaml                | 16 +++++++
> > >  src/libcamera/v4l2_pixelformat.cpp        | 10 +++++
> > >  src/libcamera/v4l2_subdevice.cpp          |  4 ++
> > >  8 files changed, 119 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > > index 78ba3969913d..164743f7e9f6 100644
> > > --- a/include/libcamera/internal/bayer_format.h
> > > +++ b/include/libcamera/internal/bayer_format.h
> > > @@ -34,6 +34,8 @@ public:
> > >               None = 0,
> > >               CSI2 = 1,
> > >               IPU3 = 2,
> > > +             PISP1 = 3,
> > > +             PISP2 = 4,
> > >       };
> > >
> > >       constexpr BayerFormat()
> > > diff --git a/include/linux/drm_fourcc.h b/include/linux/drm_fourcc.h
> > > index 4ee421b95730..eff27fbe5a1e 100644
> > > --- a/include/linux/drm_fourcc.h
> > > +++ b/include/linux/drm_fourcc.h
> > > @@ -490,6 +490,7 @@ extern "C" {
> > >  #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
> > >  #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
> > >  #define DRM_FORMAT_MOD_VENDOR_MIPI 0x0b
> > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0c
> > >
> > >  /* add more to the end as needed */
> > >
> > > @@ -1670,6 +1671,9 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
> > >   */
> > >  #define MIPI_FORMAT_MOD_CSI2_PACKED fourcc_mod_code(MIPI, 1)
> > >
> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)
> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)
> > > +
> >
> > I'll send a patch to DRM/KMS for these formats as well
> >
> > >  #if defined(__cplusplus)
> > >  }
> > >  #endif
> > > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > > index ba48d2c89726..59af6f794680 100644
> > > --- a/include/linux/videodev2.h
> > > +++ b/include/linux/videodev2.h
> > > @@ -789,6 +789,18 @@ struct v4l2_pix_format {
> > >  #define V4L2_PIX_FMT_IPU3_SGRBG10    v4l2_fourcc('i', 'p', '3', 'G') /* IPU3 packed 10-bit GRBG bayer */
> > >  #define V4L2_PIX_FMT_IPU3_SRGGB10    v4l2_fourcc('i', 'p', '3', 'r') /* IPU3 packed 10-bit RGGB bayer */
> > >
> > > +/* The pixel format for all our buffers (the precise format is found in the config buffer). */
> > > +#define V4L2_PIX_FMT_PISP_COMP1_RGGB v4l2_fourcc('P', 'C', '1', 'R')
> > > +#define V4L2_PIX_FMT_PISP_COMP1_GRBG v4l2_fourcc('P', 'C', '1', 'G')
> > > +#define V4L2_PIX_FMT_PISP_COMP1_GBRG v4l2_fourcc('P', 'C', '1', 'g')
> > > +#define V4L2_PIX_FMT_PISP_COMP1_BGGR v4l2_fourcc('P', 'C', '1', 'B')
> > > +#define V4L2_PIX_FMT_PISP_COMP1_MONO v4l2_fourcc('P', 'C', '1', 'M')
> > > +#define V4L2_PIX_FMT_PISP_COMP2_RGGB v4l2_fourcc('P', 'C', '2', 'R')
> > > +#define V4L2_PIX_FMT_PISP_COMP2_GRBG v4l2_fourcc('P', 'C', '2', 'G')
> > > +#define V4L2_PIX_FMT_PISP_COMP2_GBRG v4l2_fourcc('P', 'C', '2', 'g')
> > > +#define V4L2_PIX_FMT_PISP_COMP2_BGGR v4l2_fourcc('P', 'C', '2', 'B')
> > > +#define V4L2_PIX_FMT_PISP_COMP2_MONO v4l2_fourcc('P', 'C', '2', 'M')
> > > +
> >
> > I would use here what has been sent upstream for the BE support
> >
> > /* Raspberry Pi PiSP compressed formats. */
> > #define V4L2_PIX_FMT_PISP_COMP1_RGGB    v4l2_fourcc('P', 'C', '1', 'R') /* PiSP 8-bit mode 1 compressed RGGB bayer */
> > #define V4L2_PIX_FMT_PISP_COMP1_GRBG    v4l2_fourcc('P', 'C', '1', 'G') /* PiSP 8-bit mode 1 compressed GRBG bayer */
> > #define V4L2_PIX_FMT_PISP_COMP1_GBRG    v4l2_fourcc('P', 'C', '1', 'g') /* PiSP 8-bit mode 1 compressed GBRG bayer */
> > #define V4L2_PIX_FMT_PISP_COMP1_BGGR    v4l2_fourcc('P', 'C', '1', 'B') /* PiSP 8-bit mode 1 compressed BGGR bayer */
> > #define V4L2_PIX_FMT_PISP_COMP1_MONO    v4l2_fourcc('P', 'C', '1', 'M') /* PiSP 8-bit mode 1 compressed monochrome */
> > #define V4L2_PIX_FMT_PISP_COMP2_RGGB    v4l2_fourcc('P', 'C', '2', 'R') /* PiSP 8-bit mode 2 compressed RGGB bayer */
> > #define V4L2_PIX_FMT_PISP_COMP2_GRBG    v4l2_fourcc('P', 'C', '2', 'G') /* PiSP 8-bit mode 2 compressed GRBG bayer */
> > #define V4L2_PIX_FMT_PISP_COMP2_GBRG    v4l2_fourcc('P', 'C', '2', 'g') /* PiSP 8-bit mode 2 compressed GBRG bayer */
> > #define V4L2_PIX_FMT_PISP_COMP2_BGGR    v4l2_fourcc('P', 'C', '2', 'B') /* PiSP 8-bit mode 2 compressed BGGR bayer */
> > #define V4L2_PIX_FMT_PISP_COMP2_MONO    v4l2_fourcc('P', 'C', '2', 'M') /* PiSP 8-bit mode 2 compressed monochrome */
> >
> > To make it easier to update it when these will land in upstream
>
> Ack.
>
> >
> > >  /* SDR formats - used only for Software Defined Radio devices */
> > >  #define V4L2_SDR_FMT_CU8          v4l2_fourcc('C', 'U', '0', '8') /* IQ u8 */
> > >  #define V4L2_SDR_FMT_CU16LE       v4l2_fourcc('C', 'U', '1', '6') /* IQ u16le */
> > > @@ -818,6 +830,9 @@ struct v4l2_pix_format {
> > >  #define V4L2_META_FMT_RK_ISP1_PARAMS v4l2_fourcc('R', 'K', '1', 'P') /* Rockchip ISP1 3A Parameters */
> > >  #define V4L2_META_FMT_RK_ISP1_STAT_3A        v4l2_fourcc('R', 'K', '1', 'S') /* Rockchip ISP1 3A Statistics */
> > >
> > > +/* Vendor specific - used for RaspberryPi PiSP */
> > > +#define V4L2_META_FMT_RPI_BE_CFG v4l2_fourcc('R', 'P', 'B', 'C') /* PiSP BE configuration */
> > >  /* priv field value to indicates that subsequent fields are valid. */
> > >  #define V4L2_PIX_FMT_PRIV_MAGIC              0xfeedcafe
> > >
> > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > > index 20aedfa6d925..333b1117f531 100644
> > > --- a/src/libcamera/bayer_format.cpp
> > > +++ b/src/libcamera/bayer_format.cpp
> > > @@ -164,6 +164,14 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
> > >               { formats::SGRBG16, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG16) } },
> > >       { { BayerFormat::RGGB, 16, BayerFormat::Packing::None },
> > >               { formats::SRGGB16, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) } },
> > > +     { { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 },
> > > +             { formats::BGGR16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR) } },
> > > +     { { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 },
> > > +             { formats::GBRG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG) } },
> > > +     { { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 },
> > > +             { formats::GRBG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG) } },
> > > +     { { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 },
> > > +             { formats::RGGB16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } },
> > >       { { BayerFormat::MONO, 8, BayerFormat::Packing::None },
> > >               { formats::R8, V4L2PixelFormat(V4L2_PIX_FMT_GREY) } },
> > >       { { BayerFormat::MONO, 10, BayerFormat::Packing::None },
> > > @@ -174,6 +182,8 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
> > >               { formats::R12, V4L2PixelFormat(V4L2_PIX_FMT_Y12) } },
> > >       { { BayerFormat::MONO, 16, BayerFormat::Packing::None },
> > >               { formats::R16, V4L2PixelFormat(V4L2_PIX_FMT_Y16) } },
> > > +     { { BayerFormat::MONO, 16, BayerFormat::Packing::PISP1 },
> > > +             { formats::MONO_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO) } },
> >
> > Should the COMP2 versions be added as well ?
>
> To be honest, I doubt we will ever use COMP2, COMP1 is the better
> option almost always.  But I'll add it for completeness.
>

If you don't think they're useful feel free to skip them or add them
later

>
> >
> > >  };
> > >
> > >  const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> > > @@ -209,6 +219,10 @@ const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> > >       { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::None } },
> > >       { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::None } },
> > >       { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::None } },
> > > +     { MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 } },
> > > +     { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 } },
> > > +     { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 } },
> > > +     { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 } },
> >
> > mbusCodeToBayer is an unordered_map which
> > "is an associative container that contains key-value pairs with unique
> > keys."
> >
> > I'm afraid adding a new entry with the same key overwrite the existing
> > one ?
>
> Oops you are right!  I think this came about from when we originally
> had MBUS codes for the PISP comp formats, and those subsequently got
> removed.
>
> >
> > We already had issues with this map, as we had similar problems with the
> > _LE and _BE version of RGB565 which resolv to the same BayerFormat
> >
> >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> >
> > In general, we know that resolving mbus_codes to a unique format is
> > ill-defined. How are you using these entries in the pi5 support ?
>
> As luck would have it, the Pi 5 pipeline handler never calls
> BayerFormat::fromMbusCode() with a PISP_COMP type format so the code
> never gets exercised and goes wrong!
>
> I think the right solution is to remove MEDIA_BUS_FMT_* for the
> PISP_COMP formats as they genuinely don't have MBUS codes associated
> with them.
>

Not 100% sure I get this. Are you proposing not to add the PISP1
compressed formats to the mbusCodeToBayer map ?

> >
> > >       { MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::Packing::None } },
> > >       { MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::Packing::None } },
> > >       { MEDIA_BUS_FMT_Y12_1X12, { BayerFormat::MONO, 12, BayerFormat::Packing::None } },
> > > @@ -303,6 +317,10 @@ std::ostream &operator<<(std::ostream &out, const BayerFormat &f)
> > >               out << "-CSI2P";
> > >       else if (f.packing == BayerFormat::Packing::IPU3)
> > >               out << "-IPU3P";
> > > +     else if (f.packing == BayerFormat::Packing::PISP1)
> > > +             out << "-PISP1";
> > > +     else if (f.packing == BayerFormat::Packing::PISP2)
> > > +             out << "-PISP2";
> > >
> > >       return out;
> > >  }
> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > index a674f4179cc8..e603a7eda579 100644
> > > --- a/src/libcamera/formats.cpp
> > > +++ b/src/libcamera/formats.cpp
> > > @@ -547,6 +547,16 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >               .pixelsPerGroup = 1,
> > >               .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > >       } },
> > > +     { formats::MONO_PISP_COMP1, {
> > > +             .name = "MONO_PISP_COMP1",
> > > +             .format = formats::MONO_PISP_COMP1,
> > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO), },
> > > +             .bitsPerPixel = 16,
> > > +             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > > +             .packed = true,
> > > +             .pixelsPerGroup = 1,
> > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > +     } },
> > >
> > >       /* Bayer formats. */
> > >       { formats::SBGGR8, {
> > > @@ -910,7 +920,46 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >               .pixelsPerGroup = 25,
> > >               .planes = {{ { 32, 1 }, { 0, 0 }, { 0, 0 } }},
> > >       } },
> > > -
> > > +     { formats::BGGR16_PISP_COMP1, {
> > > +             .name = "BGGR16_PISP_COMP1",
> > > +             .format = formats::BGGR16_PISP_COMP1,
> > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
> > > +             .bitsPerPixel = 16,
> > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > +             .packed = true,
> > > +             .pixelsPerGroup = 2,
> > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > +     } },
> >
> > For regular 16-bit formats this should be
> >
> >                 .pixelsPerGroup = 2,
> >                 .planes = {{ { 4, 1 }, { 0, 0 }, { 0, 0 } }},
> >
> > As 2 pixels of 16 bits each consume 4 bytes. Is this {2, 1}
> > intentional and due to compression ?
>
> Yes, I used {2, 1} to denote the 8-bit/sample after compression.  Do
> you think I should change to {4, 1}?

Depends on how many bytes the format actually consumes :)

For 16bpp RAW formats, we have pixelsPerGroup = 2 and each sample
consumes 2 bytes.

According to the pixelsPerGroup definition:

 * A pixel group is defined as the minimum number of pixels (including padding)
 * necessary in a row when the image has only one column of effective pixels.

So for a RAW Bayer formats, to get a complete full-color "superpixel"
we need at least two pixels per row.

        RG
        GB

Each "RG" or "GB" pair is composed by 2 16-bit samples, for a total of
32bits of data, from which the {4, 1} value in planes[0]

According to "Table 2", Chapter 5 of the PiSP datasheet, the
compressed formats are described as

     8-bits per pixel single channel compressed with mode n, for n = 1, 2, 3

Which suggests each "RG" or "GB" pair is actually 2 samples of 8 bits
each, for a plane size of {2, 1}.

Is this correct ? Is the actual .bitsPerPixel value for the compressed
formats 8 instead of 16 then ?

Thanks
  j

>
> Regards,
> Naush
>
>
> >
> > > +     { formats::GBRG16_PISP_COMP1, {
> > > +             .name = "GBRG16_PISP_COMP1",
> > > +             .format = formats::GBRG16_PISP_COMP1,
> > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
> > > +             .bitsPerPixel = 16,
> > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > +             .packed = true,
> > > +             .pixelsPerGroup = 2,
> > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > +     } },
> > > +     { formats::GRBG16_PISP_COMP1, {
> > > +             .name = "GRBG16_PISP_COMP1",
> > > +             .format = formats::GRBG16_PISP_COMP1,
> > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
> > > +             .bitsPerPixel = 16,
> > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > +             .packed = true,
> > > +             .pixelsPerGroup = 2,
> > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > +     } },
> > > +     { formats::RGGB16_PISP_COMP1, {
> > > +             .name = "RGGB16_PISP_COMP1",
> > > +             .format = formats::RGGB16_PISP_COMP1,
> > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
> > > +             .bitsPerPixel = 16,
> > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > +             .packed = true,
> > > +             .pixelsPerGroup = 2,
> > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > +     } },
> > >       /* Compressed formats. */
> > >       { formats::MJPEG, {
> > >               .name = "MJPEG",
> > > diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
> > > index bde2cc803b98..f6df721243d0 100644
> > > --- a/src/libcamera/formats.yaml
> > > +++ b/src/libcamera/formats.yaml
> > > @@ -190,4 +190,20 @@ formats:
> > >    - SBGGR10_IPU3:
> > >        fourcc: DRM_FORMAT_SBGGR10
> > >        mod: IPU3_FORMAT_MOD_PACKED
> > > +
> > > +  - RGGB16_PISP_COMP1:
> > > +      fourcc: DRM_FORMAT_SRGGB16
> > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > +  - GRBG16_PISP_COMP1:
> > > +      fourcc: DRM_FORMAT_SGRBG16
> > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > +  - GBRG16_PISP_COMP1:
> > > +      fourcc: DRM_FORMAT_SGBRG16
> > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > +  - BGGR16_PISP_COMP1:
> > > +      fourcc: DRM_FORMAT_SBGGR16
> > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > +  - MONO_PISP_COMP1:
> > > +      fourcc: DRM_FORMAT_R16
> > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > >  ...
> > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
> > > index efb6f2940235..47baaf60199d 100644
> > > --- a/src/libcamera/v4l2_pixelformat.cpp
> > > +++ b/src/libcamera/v4l2_pixelformat.cpp
> > > @@ -207,6 +207,16 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{
> > >               { formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
> > >       { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
> > >               { formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
> > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
> > > +             { formats::BGGR16_PISP_COMP1, "16-bit Bayer BGBG/GRGR PiSP Compress Mode 1" } },
> > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
> > > +             { formats::GBRG16_PISP_COMP1, "16-bit Bayer GBGB/RGRG PiSP Compress Mode 1" } },
> > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
> > > +             { formats::GRBG16_PISP_COMP1, "16-bit Bayer GRGR/BGBG PiSP Compress Mode 1" } },
> > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
> > > +             { formats::RGGB16_PISP_COMP1, "16-bit Bayer RGRG/GBGB PiSP Compress Mode 1" } },
> > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
> > > +             { formats::MONO_PISP_COMP1, "16-bit Mono PiSP Compress Mode 1" } },
> > >
> > >       /* Compressed formats. */
> > >       { V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 6d0785b7b484..aea90abaf9ef 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -134,6 +134,10 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > >       { MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > >       { MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > >       { MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > +     { MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > +     { MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > +     { MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > +     { MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > >       /* \todo Clarify colour encoding for HSV formats */
> > >       { MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32", PixelFormatInfo::ColourEncodingRGB } },
> > >       { MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8", PixelFormatInfo::ColourEncodingYUV } },
> > > --
> > > 2.34.1
> > >
Naushir Patuck Feb. 28, 2024, 4:45 p.m. UTC | #6
Hi Jacopo,

On Wed, 28 Feb 2024 at 11:14, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Tue, Feb 27, 2024 at 10:20:40AM +0000, Naushir Patuck wrote:
> > Hi Jacopo,
> >
> > Thanks for the feedback, and sending the updates to the DRM mailing list!
> >
>
> As usual, nothing's easy :)
>
> > On Mon, 26 Feb 2024 at 13:49, Jacopo Mondi
> > <jacopo.mondi@ideasonboard.com> wrote:
> > >
> > > Hi Naush
> > >
> > > On Thu, Feb 15, 2024 at 01:27:10PM +0000, Naushir Patuck wrote:
> > > > Add the Raspberry Pi 5 PiSP specific compressed Bayer format types 1/2:
> > > > - V4L2_PIX_FMT_PISP_COMP1_xxx
> > > > - V4L2_PIX_FMT_PISP_COMP2_xxx
> > > >
> > > > Add the Raspberry Pi 5 PiSP Backend config format:
> > > > - V4L2_META_FMT_RPI_BE_CFG
> > > >
> > > > Additionally, we also extend libcamera format handlers to support 16-bit
> > > > Bayer formats across the media bus.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > ---
> > > >  include/libcamera/internal/bayer_format.h |  2 +
> > > >  include/linux/drm_fourcc.h                |  4 ++
> > > >  include/linux/videodev2.h                 | 15 +++++++
> > > >  src/libcamera/bayer_format.cpp            | 18 ++++++++
> > > >  src/libcamera/formats.cpp                 | 51 ++++++++++++++++++++++-
> > > >  src/libcamera/formats.yaml                | 16 +++++++
> > > >  src/libcamera/v4l2_pixelformat.cpp        | 10 +++++
> > > >  src/libcamera/v4l2_subdevice.cpp          |  4 ++
> > > >  8 files changed, 119 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > > > index 78ba3969913d..164743f7e9f6 100644
> > > > --- a/include/libcamera/internal/bayer_format.h
> > > > +++ b/include/libcamera/internal/bayer_format.h
> > > > @@ -34,6 +34,8 @@ public:
> > > >               None = 0,
> > > >               CSI2 = 1,
> > > >               IPU3 = 2,
> > > > +             PISP1 = 3,
> > > > +             PISP2 = 4,
> > > >       };
> > > >
> > > >       constexpr BayerFormat()
> > > > diff --git a/include/linux/drm_fourcc.h b/include/linux/drm_fourcc.h
> > > > index 4ee421b95730..eff27fbe5a1e 100644
> > > > --- a/include/linux/drm_fourcc.h
> > > > +++ b/include/linux/drm_fourcc.h
> > > > @@ -490,6 +490,7 @@ extern "C" {
> > > >  #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
> > > >  #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
> > > >  #define DRM_FORMAT_MOD_VENDOR_MIPI 0x0b
> > > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0c
> > > >
> > > >  /* add more to the end as needed */
> > > >
> > > > @@ -1670,6 +1671,9 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
> > > >   */
> > > >  #define MIPI_FORMAT_MOD_CSI2_PACKED fourcc_mod_code(MIPI, 1)
> > > >
> > > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)
> > > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)
> > > > +
> > >
> > > I'll send a patch to DRM/KMS for these formats as well
> > >
> > > >  #if defined(__cplusplus)
> > > >  }
> > > >  #endif
> > > > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > > > index ba48d2c89726..59af6f794680 100644
> > > > --- a/include/linux/videodev2.h
> > > > +++ b/include/linux/videodev2.h
> > > > @@ -789,6 +789,18 @@ struct v4l2_pix_format {
> > > >  #define V4L2_PIX_FMT_IPU3_SGRBG10    v4l2_fourcc('i', 'p', '3', 'G') /* IPU3 packed 10-bit GRBG bayer */
> > > >  #define V4L2_PIX_FMT_IPU3_SRGGB10    v4l2_fourcc('i', 'p', '3', 'r') /* IPU3 packed 10-bit RGGB bayer */
> > > >
> > > > +/* The pixel format for all our buffers (the precise format is found in the config buffer). */
> > > > +#define V4L2_PIX_FMT_PISP_COMP1_RGGB v4l2_fourcc('P', 'C', '1', 'R')
> > > > +#define V4L2_PIX_FMT_PISP_COMP1_GRBG v4l2_fourcc('P', 'C', '1', 'G')
> > > > +#define V4L2_PIX_FMT_PISP_COMP1_GBRG v4l2_fourcc('P', 'C', '1', 'g')
> > > > +#define V4L2_PIX_FMT_PISP_COMP1_BGGR v4l2_fourcc('P', 'C', '1', 'B')
> > > > +#define V4L2_PIX_FMT_PISP_COMP1_MONO v4l2_fourcc('P', 'C', '1', 'M')
> > > > +#define V4L2_PIX_FMT_PISP_COMP2_RGGB v4l2_fourcc('P', 'C', '2', 'R')
> > > > +#define V4L2_PIX_FMT_PISP_COMP2_GRBG v4l2_fourcc('P', 'C', '2', 'G')
> > > > +#define V4L2_PIX_FMT_PISP_COMP2_GBRG v4l2_fourcc('P', 'C', '2', 'g')
> > > > +#define V4L2_PIX_FMT_PISP_COMP2_BGGR v4l2_fourcc('P', 'C', '2', 'B')
> > > > +#define V4L2_PIX_FMT_PISP_COMP2_MONO v4l2_fourcc('P', 'C', '2', 'M')
> > > > +
> > >
> > > I would use here what has been sent upstream for the BE support
> > >
> > > /* Raspberry Pi PiSP compressed formats. */
> > > #define V4L2_PIX_FMT_PISP_COMP1_RGGB    v4l2_fourcc('P', 'C', '1', 'R') /* PiSP 8-bit mode 1 compressed RGGB bayer */
> > > #define V4L2_PIX_FMT_PISP_COMP1_GRBG    v4l2_fourcc('P', 'C', '1', 'G') /* PiSP 8-bit mode 1 compressed GRBG bayer */
> > > #define V4L2_PIX_FMT_PISP_COMP1_GBRG    v4l2_fourcc('P', 'C', '1', 'g') /* PiSP 8-bit mode 1 compressed GBRG bayer */
> > > #define V4L2_PIX_FMT_PISP_COMP1_BGGR    v4l2_fourcc('P', 'C', '1', 'B') /* PiSP 8-bit mode 1 compressed BGGR bayer */
> > > #define V4L2_PIX_FMT_PISP_COMP1_MONO    v4l2_fourcc('P', 'C', '1', 'M') /* PiSP 8-bit mode 1 compressed monochrome */
> > > #define V4L2_PIX_FMT_PISP_COMP2_RGGB    v4l2_fourcc('P', 'C', '2', 'R') /* PiSP 8-bit mode 2 compressed RGGB bayer */
> > > #define V4L2_PIX_FMT_PISP_COMP2_GRBG    v4l2_fourcc('P', 'C', '2', 'G') /* PiSP 8-bit mode 2 compressed GRBG bayer */
> > > #define V4L2_PIX_FMT_PISP_COMP2_GBRG    v4l2_fourcc('P', 'C', '2', 'g') /* PiSP 8-bit mode 2 compressed GBRG bayer */
> > > #define V4L2_PIX_FMT_PISP_COMP2_BGGR    v4l2_fourcc('P', 'C', '2', 'B') /* PiSP 8-bit mode 2 compressed BGGR bayer */
> > > #define V4L2_PIX_FMT_PISP_COMP2_MONO    v4l2_fourcc('P', 'C', '2', 'M') /* PiSP 8-bit mode 2 compressed monochrome */
> > >
> > > To make it easier to update it when these will land in upstream
> >
> > Ack.
> >
> > >
> > > >  /* SDR formats - used only for Software Defined Radio devices */
> > > >  #define V4L2_SDR_FMT_CU8          v4l2_fourcc('C', 'U', '0', '8') /* IQ u8 */
> > > >  #define V4L2_SDR_FMT_CU16LE       v4l2_fourcc('C', 'U', '1', '6') /* IQ u16le */
> > > > @@ -818,6 +830,9 @@ struct v4l2_pix_format {
> > > >  #define V4L2_META_FMT_RK_ISP1_PARAMS v4l2_fourcc('R', 'K', '1', 'P') /* Rockchip ISP1 3A Parameters */
> > > >  #define V4L2_META_FMT_RK_ISP1_STAT_3A        v4l2_fourcc('R', 'K', '1', 'S') /* Rockchip ISP1 3A Statistics */
> > > >
> > > > +/* Vendor specific - used for RaspberryPi PiSP */
> > > > +#define V4L2_META_FMT_RPI_BE_CFG v4l2_fourcc('R', 'P', 'B', 'C') /* PiSP BE configuration */
> > > >  /* priv field value to indicates that subsequent fields are valid. */
> > > >  #define V4L2_PIX_FMT_PRIV_MAGIC              0xfeedcafe
> > > >
> > > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > > > index 20aedfa6d925..333b1117f531 100644
> > > > --- a/src/libcamera/bayer_format.cpp
> > > > +++ b/src/libcamera/bayer_format.cpp
> > > > @@ -164,6 +164,14 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
> > > >               { formats::SGRBG16, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG16) } },
> > > >       { { BayerFormat::RGGB, 16, BayerFormat::Packing::None },
> > > >               { formats::SRGGB16, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) } },
> > > > +     { { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 },
> > > > +             { formats::BGGR16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR) } },
> > > > +     { { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 },
> > > > +             { formats::GBRG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG) } },
> > > > +     { { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 },
> > > > +             { formats::GRBG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG) } },
> > > > +     { { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 },
> > > > +             { formats::RGGB16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } },
> > > >       { { BayerFormat::MONO, 8, BayerFormat::Packing::None },
> > > >               { formats::R8, V4L2PixelFormat(V4L2_PIX_FMT_GREY) } },
> > > >       { { BayerFormat::MONO, 10, BayerFormat::Packing::None },
> > > > @@ -174,6 +182,8 @@ const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
> > > >               { formats::R12, V4L2PixelFormat(V4L2_PIX_FMT_Y12) } },
> > > >       { { BayerFormat::MONO, 16, BayerFormat::Packing::None },
> > > >               { formats::R16, V4L2PixelFormat(V4L2_PIX_FMT_Y16) } },
> > > > +     { { BayerFormat::MONO, 16, BayerFormat::Packing::PISP1 },
> > > > +             { formats::MONO_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO) } },
> > >
> > > Should the COMP2 versions be added as well ?
> >
> > To be honest, I doubt we will ever use COMP2, COMP1 is the better
> > option almost always.  But I'll add it for completeness.
> >
>
> If you don't think they're useful feel free to skip them or add them
> later

I think that's best.

>
> >
> > >
> > > >  };
> > > >
> > > >  const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> > > > @@ -209,6 +219,10 @@ const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> > > >       { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::None } },
> > > >       { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::None } },
> > > >       { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::None } },
> > > > +     { MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 } },
> > > > +     { MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 } },
> > > > +     { MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 } },
> > > > +     { MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 } },
> > >
> > > mbusCodeToBayer is an unordered_map which
> > > "is an associative container that contains key-value pairs with unique
> > > keys."
> > >
> > > I'm afraid adding a new entry with the same key overwrite the existing
> > > one ?
> >
> > Oops you are right!  I think this came about from when we originally
> > had MBUS codes for the PISP comp formats, and those subsequently got
> > removed.
> >
> > >
> > > We already had issues with this map, as we had similar problems with the
> > > _LE and _BE version of RGB565 which resolv to the same BayerFormat
> > >
> > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > >         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > >
> > > In general, we know that resolving mbus_codes to a unique format is
> > > ill-defined. How are you using these entries in the pi5 support ?
> >
> > As luck would have it, the Pi 5 pipeline handler never calls
> > BayerFormat::fromMbusCode() with a PISP_COMP type format so the code
> > never gets exercised and goes wrong!
> >
> > I think the right solution is to remove MEDIA_BUS_FMT_* for the
> > PISP_COMP formats as they genuinely don't have MBUS codes associated
> > with them.
> >
>
> Not 100% sure I get this. Are you proposing not to add the PISP1
> compressed formats to the mbusCodeToBayer map ?

That's correct.  PISP_COMP formats do not actually have mubs codes as
they are not strictly formats used across a bus, only on the output.
So we remove them from this table.  The Pi 5 pipeline handler will
never need to access an mbus code for a PISP_COMP format.

>
> > >
> > > >       { MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::Packing::None } },
> > > >       { MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::Packing::None } },
> > > >       { MEDIA_BUS_FMT_Y12_1X12, { BayerFormat::MONO, 12, BayerFormat::Packing::None } },
> > > > @@ -303,6 +317,10 @@ std::ostream &operator<<(std::ostream &out, const BayerFormat &f)
> > > >               out << "-CSI2P";
> > > >       else if (f.packing == BayerFormat::Packing::IPU3)
> > > >               out << "-IPU3P";
> > > > +     else if (f.packing == BayerFormat::Packing::PISP1)
> > > > +             out << "-PISP1";
> > > > +     else if (f.packing == BayerFormat::Packing::PISP2)
> > > > +             out << "-PISP2";
> > > >
> > > >       return out;
> > > >  }
> > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > > index a674f4179cc8..e603a7eda579 100644
> > > > --- a/src/libcamera/formats.cpp
> > > > +++ b/src/libcamera/formats.cpp
> > > > @@ -547,6 +547,16 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >               .pixelsPerGroup = 1,
> > > >               .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > >       } },
> > > > +     { formats::MONO_PISP_COMP1, {
> > > > +             .name = "MONO_PISP_COMP1",
> > > > +             .format = formats::MONO_PISP_COMP1,
> > > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO), },
> > > > +             .bitsPerPixel = 16,
> > > > +             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > > > +             .packed = true,
> > > > +             .pixelsPerGroup = 1,
> > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > +     } },
> > > >
> > > >       /* Bayer formats. */
> > > >       { formats::SBGGR8, {
> > > > @@ -910,7 +920,46 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > > >               .pixelsPerGroup = 25,
> > > >               .planes = {{ { 32, 1 }, { 0, 0 }, { 0, 0 } }},
> > > >       } },
> > > > -
> > > > +     { formats::BGGR16_PISP_COMP1, {
> > > > +             .name = "BGGR16_PISP_COMP1",
> > > > +             .format = formats::BGGR16_PISP_COMP1,
> > > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
> > > > +             .bitsPerPixel = 16,
> > > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > > +             .packed = true,
> > > > +             .pixelsPerGroup = 2,
> > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > +     } },
> > >
> > > For regular 16-bit formats this should be
> > >
> > >                 .pixelsPerGroup = 2,
> > >                 .planes = {{ { 4, 1 }, { 0, 0 }, { 0, 0 } }},
> > >
> > > As 2 pixels of 16 bits each consume 4 bytes. Is this {2, 1}
> > > intentional and due to compression ?
> >
> > Yes, I used {2, 1} to denote the 8-bit/sample after compression.  Do
> > you think I should change to {4, 1}?
>
> Depends on how many bytes the format actually consumes :)
>
> For 16bpp RAW formats, we have pixelsPerGroup = 2 and each sample
> consumes 2 bytes.
>
> According to the pixelsPerGroup definition:
>
>  * A pixel group is defined as the minimum number of pixels (including padding)
>  * necessary in a row when the image has only one column of effective pixels.
>
> So for a RAW Bayer formats, to get a complete full-color "superpixel"
> we need at least two pixels per row.
>
>         RG
>         GB
>
> Each "RG" or "GB" pair is composed by 2 16-bit samples, for a total of
> 32bits of data, from which the {4, 1} value in planes[0]
>
> According to "Table 2", Chapter 5 of the PiSP datasheet, the
> compressed formats are described as
>
>      8-bits per pixel single channel compressed with mode n, for n = 1, 2, 3
>
> Which suggests each "RG" or "GB" pair is actually 2 samples of 8 bits
> each, for a plane size of {2, 1}.
>
> Is this correct ? Is the actual .bitsPerPixel value for the compressed
> formats 8 instead of 16 then ?

This is correct, each sample is 8-bits so "RG"/"GB" pairs are 16-bits each.
So it sounds like I need to use .planes =  {2, 1}, but .bitsPerPixel = 8 maybe?

Naush

>
> Thanks
>   j
>
> >
> > Regards,
> > Naush
> >
> >
> > >
> > > > +     { formats::GBRG16_PISP_COMP1, {
> > > > +             .name = "GBRG16_PISP_COMP1",
> > > > +             .format = formats::GBRG16_PISP_COMP1,
> > > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
> > > > +             .bitsPerPixel = 16,
> > > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > > +             .packed = true,
> > > > +             .pixelsPerGroup = 2,
> > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > +     } },
> > > > +     { formats::GRBG16_PISP_COMP1, {
> > > > +             .name = "GRBG16_PISP_COMP1",
> > > > +             .format = formats::GRBG16_PISP_COMP1,
> > > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
> > > > +             .bitsPerPixel = 16,
> > > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > > +             .packed = true,
> > > > +             .pixelsPerGroup = 2,
> > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > +     } },
> > > > +     { formats::RGGB16_PISP_COMP1, {
> > > > +             .name = "RGGB16_PISP_COMP1",
> > > > +             .format = formats::RGGB16_PISP_COMP1,
> > > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
> > > > +             .bitsPerPixel = 16,
> > > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > > +             .packed = true,
> > > > +             .pixelsPerGroup = 2,
> > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > +     } },
> > > >       /* Compressed formats. */
> > > >       { formats::MJPEG, {
> > > >               .name = "MJPEG",
> > > > diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
> > > > index bde2cc803b98..f6df721243d0 100644
> > > > --- a/src/libcamera/formats.yaml
> > > > +++ b/src/libcamera/formats.yaml
> > > > @@ -190,4 +190,20 @@ formats:
> > > >    - SBGGR10_IPU3:
> > > >        fourcc: DRM_FORMAT_SBGGR10
> > > >        mod: IPU3_FORMAT_MOD_PACKED
> > > > +
> > > > +  - RGGB16_PISP_COMP1:
> > > > +      fourcc: DRM_FORMAT_SRGGB16
> > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > +  - GRBG16_PISP_COMP1:
> > > > +      fourcc: DRM_FORMAT_SGRBG16
> > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > +  - GBRG16_PISP_COMP1:
> > > > +      fourcc: DRM_FORMAT_SGBRG16
> > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > +  - BGGR16_PISP_COMP1:
> > > > +      fourcc: DRM_FORMAT_SBGGR16
> > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > +  - MONO_PISP_COMP1:
> > > > +      fourcc: DRM_FORMAT_R16
> > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > >  ...
> > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
> > > > index efb6f2940235..47baaf60199d 100644
> > > > --- a/src/libcamera/v4l2_pixelformat.cpp
> > > > +++ b/src/libcamera/v4l2_pixelformat.cpp
> > > > @@ -207,6 +207,16 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{
> > > >               { formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
> > > >       { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
> > > >               { formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
> > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
> > > > +             { formats::BGGR16_PISP_COMP1, "16-bit Bayer BGBG/GRGR PiSP Compress Mode 1" } },
> > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
> > > > +             { formats::GBRG16_PISP_COMP1, "16-bit Bayer GBGB/RGRG PiSP Compress Mode 1" } },
> > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
> > > > +             { formats::GRBG16_PISP_COMP1, "16-bit Bayer GRGR/BGBG PiSP Compress Mode 1" } },
> > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
> > > > +             { formats::RGGB16_PISP_COMP1, "16-bit Bayer RGRG/GBGB PiSP Compress Mode 1" } },
> > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
> > > > +             { formats::MONO_PISP_COMP1, "16-bit Mono PiSP Compress Mode 1" } },
> > > >
> > > >       /* Compressed formats. */
> > > >       { V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > > index 6d0785b7b484..aea90abaf9ef 100644
> > > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > > @@ -134,6 +134,10 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > > >       { MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > >       { MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > >       { MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > > +     { MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > > +     { MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > > +     { MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > > +     { MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > >       /* \todo Clarify colour encoding for HSV formats */
> > > >       { MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32", PixelFormatInfo::ColourEncodingRGB } },
> > > >       { MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8", PixelFormatInfo::ColourEncodingYUV } },
> > > > --
> > > > 2.34.1
> > > >
Jacopo Mondi Feb. 28, 2024, 5:16 p.m. UTC | #7
Hi Naush

On Wed, Feb 28, 2024 at 04:45:54PM +0000, Naushir Patuck wrote:
> Hi Jacopo,
>
> > > > > -
> > > > > +     { formats::BGGR16_PISP_COMP1, {
> > > > > +             .name = "BGGR16_PISP_COMP1",
> > > > > +             .format = formats::BGGR16_PISP_COMP1,
> > > > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
> > > > > +             .bitsPerPixel = 16,
> > > > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > > > +             .packed = true,
> > > > > +             .pixelsPerGroup = 2,
> > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > +     } },
> > > >
> > > > For regular 16-bit formats this should be
> > > >
> > > >                 .pixelsPerGroup = 2,
> > > >                 .planes = {{ { 4, 1 }, { 0, 0 }, { 0, 0 } }},
> > > >
> > > > As 2 pixels of 16 bits each consume 4 bytes. Is this {2, 1}
> > > > intentional and due to compression ?
> > >
> > > Yes, I used {2, 1} to denote the 8-bit/sample after compression.  Do
> > > you think I should change to {4, 1}?
> >
> > Depends on how many bytes the format actually consumes :)
> >
> > For 16bpp RAW formats, we have pixelsPerGroup = 2 and each sample
> > consumes 2 bytes.
> >
> > According to the pixelsPerGroup definition:
> >
> >  * A pixel group is defined as the minimum number of pixels (including padding)
> >  * necessary in a row when the image has only one column of effective pixels.
> >
> > So for a RAW Bayer formats, to get a complete full-color "superpixel"
> > we need at least two pixels per row.
> >
> >         RG
> >         GB
> >
> > Each "RG" or "GB" pair is composed by 2 16-bit samples, for a total of
> > 32bits of data, from which the {4, 1} value in planes[0]
> >
> > According to "Table 2", Chapter 5 of the PiSP datasheet, the
> > compressed formats are described as
> >
> >      8-bits per pixel single channel compressed with mode n, for n = 1, 2, 3
> >
> > Which suggests each "RG" or "GB" pair is actually 2 samples of 8 bits
> > each, for a plane size of {2, 1}.
> >
> > Is this correct ? Is the actual .bitsPerPixel value for the compressed
> > formats 8 instead of 16 then ?
>
> This is correct, each sample is 8-bits so "RG"/"GB" pairs are 16-bits each.
> So it sounds like I need to use .planes =  {2, 1}, but .bitsPerPixel = 8 maybe?

I now wonder if the '16' in the format names is correct then :)

>
> Naush
>
> >
> > Thanks
> >   j
> >
> > >
> > > Regards,
> > > Naush
> > >
> > >
> > > >
> > > > > +     { formats::GBRG16_PISP_COMP1, {
> > > > > +             .name = "GBRG16_PISP_COMP1",
> > > > > +             .format = formats::GBRG16_PISP_COMP1,
> > > > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
> > > > > +             .bitsPerPixel = 16,
> > > > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > > > +             .packed = true,
> > > > > +             .pixelsPerGroup = 2,
> > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > +     } },
> > > > > +     { formats::GRBG16_PISP_COMP1, {
> > > > > +             .name = "GRBG16_PISP_COMP1",
> > > > > +             .format = formats::GRBG16_PISP_COMP1,
> > > > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
> > > > > +             .bitsPerPixel = 16,
> > > > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > > > +             .packed = true,
> > > > > +             .pixelsPerGroup = 2,
> > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > +     } },
> > > > > +     { formats::RGGB16_PISP_COMP1, {
> > > > > +             .name = "RGGB16_PISP_COMP1",
> > > > > +             .format = formats::RGGB16_PISP_COMP1,
> > > > > +             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
> > > > > +             .bitsPerPixel = 16,
> > > > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > > > > +             .packed = true,
> > > > > +             .pixelsPerGroup = 2,
> > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > +     } },
> > > > >       /* Compressed formats. */
> > > > >       { formats::MJPEG, {
> > > > >               .name = "MJPEG",
> > > > > diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
> > > > > index bde2cc803b98..f6df721243d0 100644
> > > > > --- a/src/libcamera/formats.yaml
> > > > > +++ b/src/libcamera/formats.yaml
> > > > > @@ -190,4 +190,20 @@ formats:
> > > > >    - SBGGR10_IPU3:
> > > > >        fourcc: DRM_FORMAT_SBGGR10
> > > > >        mod: IPU3_FORMAT_MOD_PACKED
> > > > > +
> > > > > +  - RGGB16_PISP_COMP1:
> > > > > +      fourcc: DRM_FORMAT_SRGGB16
> > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > +  - GRBG16_PISP_COMP1:
> > > > > +      fourcc: DRM_FORMAT_SGRBG16
> > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > +  - GBRG16_PISP_COMP1:
> > > > > +      fourcc: DRM_FORMAT_SGBRG16
> > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > +  - BGGR16_PISP_COMP1:
> > > > > +      fourcc: DRM_FORMAT_SBGGR16
> > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > +  - MONO_PISP_COMP1:
> > > > > +      fourcc: DRM_FORMAT_R16
> > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > >  ...
> > > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
> > > > > index efb6f2940235..47baaf60199d 100644
> > > > > --- a/src/libcamera/v4l2_pixelformat.cpp
> > > > > +++ b/src/libcamera/v4l2_pixelformat.cpp
> > > > > @@ -207,6 +207,16 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{
> > > > >               { formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
> > > > >       { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
> > > > >               { formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
> > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
> > > > > +             { formats::BGGR16_PISP_COMP1, "16-bit Bayer BGBG/GRGR PiSP Compress Mode 1" } },
> > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
> > > > > +             { formats::GBRG16_PISP_COMP1, "16-bit Bayer GBGB/RGRG PiSP Compress Mode 1" } },
> > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
> > > > > +             { formats::GRBG16_PISP_COMP1, "16-bit Bayer GRGR/BGBG PiSP Compress Mode 1" } },
> > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
> > > > > +             { formats::RGGB16_PISP_COMP1, "16-bit Bayer RGRG/GBGB PiSP Compress Mode 1" } },
> > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
> > > > > +             { formats::MONO_PISP_COMP1, "16-bit Mono PiSP Compress Mode 1" } },
> > > > >
> > > > >       /* Compressed formats. */
> > > > >       { V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > > > index 6d0785b7b484..aea90abaf9ef 100644
> > > > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > > > @@ -134,6 +134,10 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > > > >       { MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > > >       { MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > > >       { MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > > > +     { MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > > > +     { MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > > > +     { MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > > > +     { MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16", PixelFormatInfo::ColourEncodingRAW } },
> > > > >       /* \todo Clarify colour encoding for HSV formats */
> > > > >       { MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32", PixelFormatInfo::ColourEncodingRGB } },
> > > > >       { MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8", PixelFormatInfo::ColourEncodingYUV } },
> > > > > --
> > > > > 2.34.1
> > > > >
Naushir Patuck Feb. 28, 2024, 5:19 p.m. UTC | #8
On Wed, 28 Feb 2024, 5:16 pm Jacopo Mondi, <jacopo.mondi@ideasonboard.com>
wrote:

> Hi Naush
>
> On Wed, Feb 28, 2024 at 04:45:54PM +0000, Naushir Patuck wrote:
> > Hi Jacopo,
> >
> > > > > > -
> > > > > > +     { formats::BGGR16_PISP_COMP1, {
> > > > > > +             .name = "BGGR16_PISP_COMP1",
> > > > > > +             .format = formats::BGGR16_PISP_COMP1,
> > > > > > +             .v4l2Formats = {
> V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
> > > > > > +             .bitsPerPixel = 16,
> > > > > > +             .colourEncoding =
> PixelFormatInfo::ColourEncodingRAW,
> > > > > > +             .packed = true,
> > > > > > +             .pixelsPerGroup = 2,
> > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > +     } },
> > > > >
> > > > > For regular 16-bit formats this should be
> > > > >
> > > > >                 .pixelsPerGroup = 2,
> > > > >                 .planes = {{ { 4, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > >
> > > > > As 2 pixels of 16 bits each consume 4 bytes. Is this {2, 1}
> > > > > intentional and due to compression ?
> > > >
> > > > Yes, I used {2, 1} to denote the 8-bit/sample after compression.  Do
> > > > you think I should change to {4, 1}?
> > >
> > > Depends on how many bytes the format actually consumes :)
> > >
> > > For 16bpp RAW formats, we have pixelsPerGroup = 2 and each sample
> > > consumes 2 bytes.
> > >
> > > According to the pixelsPerGroup definition:
> > >
> > >  * A pixel group is defined as the minimum number of pixels (including
> padding)
> > >  * necessary in a row when the image has only one column of effective
> pixels.
> > >
> > > So for a RAW Bayer formats, to get a complete full-color "superpixel"
> > > we need at least two pixels per row.
> > >
> > >         RG
> > >         GB
> > >
> > > Each "RG" or "GB" pair is composed by 2 16-bit samples, for a total of
> > > 32bits of data, from which the {4, 1} value in planes[0]
> > >
> > > According to "Table 2", Chapter 5 of the PiSP datasheet, the
> > > compressed formats are described as
> > >
> > >      8-bits per pixel single channel compressed with mode n, for n =
> 1, 2, 3
> > >
> > > Which suggests each "RG" or "GB" pair is actually 2 samples of 8 bits
> > > each, for a plane size of {2, 1}.
> > >
> > > Is this correct ? Is the actual .bitsPerPixel value for the compressed
> > > formats 8 instead of 16 then ?
> >
> > This is correct, each sample is 8-bits so "RG"/"GB" pairs are 16-bits
> each.
> > So it sounds like I need to use .planes =  {2, 1}, but .bitsPerPixel = 8
> maybe?
>
> I now wonder if the '16' in the format names is correct then :)
>

It's meant to signify 16-bits compressed to 8-bits (in this case).  Maybe a
defined naming scheme is needed for such cases?


> >
> > Naush
> >
> > >
> > > Thanks
> > >   j
> > >
> > > >
> > > > Regards,
> > > > Naush
> > > >
> > > >
> > > > >
> > > > > > +     { formats::GBRG16_PISP_COMP1, {
> > > > > > +             .name = "GBRG16_PISP_COMP1",
> > > > > > +             .format = formats::GBRG16_PISP_COMP1,
> > > > > > +             .v4l2Formats = {
> V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
> > > > > > +             .bitsPerPixel = 16,
> > > > > > +             .colourEncoding =
> PixelFormatInfo::ColourEncodingRAW,
> > > > > > +             .packed = true,
> > > > > > +             .pixelsPerGroup = 2,
> > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > +     } },
> > > > > > +     { formats::GRBG16_PISP_COMP1, {
> > > > > > +             .name = "GRBG16_PISP_COMP1",
> > > > > > +             .format = formats::GRBG16_PISP_COMP1,
> > > > > > +             .v4l2Formats = {
> V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
> > > > > > +             .bitsPerPixel = 16,
> > > > > > +             .colourEncoding =
> PixelFormatInfo::ColourEncodingRAW,
> > > > > > +             .packed = true,
> > > > > > +             .pixelsPerGroup = 2,
> > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > +     } },
> > > > > > +     { formats::RGGB16_PISP_COMP1, {
> > > > > > +             .name = "RGGB16_PISP_COMP1",
> > > > > > +             .format = formats::RGGB16_PISP_COMP1,
> > > > > > +             .v4l2Formats = {
> V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
> > > > > > +             .bitsPerPixel = 16,
> > > > > > +             .colourEncoding =
> PixelFormatInfo::ColourEncodingRAW,
> > > > > > +             .packed = true,
> > > > > > +             .pixelsPerGroup = 2,
> > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > +     } },
> > > > > >       /* Compressed formats. */
> > > > > >       { formats::MJPEG, {
> > > > > >               .name = "MJPEG",
> > > > > > diff --git a/src/libcamera/formats.yaml
> b/src/libcamera/formats.yaml
> > > > > > index bde2cc803b98..f6df721243d0 100644
> > > > > > --- a/src/libcamera/formats.yaml
> > > > > > +++ b/src/libcamera/formats.yaml
> > > > > > @@ -190,4 +190,20 @@ formats:
> > > > > >    - SBGGR10_IPU3:
> > > > > >        fourcc: DRM_FORMAT_SBGGR10
> > > > > >        mod: IPU3_FORMAT_MOD_PACKED
> > > > > > +
> > > > > > +  - RGGB16_PISP_COMP1:
> > > > > > +      fourcc: DRM_FORMAT_SRGGB16
> > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > +  - GRBG16_PISP_COMP1:
> > > > > > +      fourcc: DRM_FORMAT_SGRBG16
> > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > +  - GBRG16_PISP_COMP1:
> > > > > > +      fourcc: DRM_FORMAT_SGBRG16
> > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > +  - BGGR16_PISP_COMP1:
> > > > > > +      fourcc: DRM_FORMAT_SBGGR16
> > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > +  - MONO_PISP_COMP1:
> > > > > > +      fourcc: DRM_FORMAT_R16
> > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > >  ...
> > > > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp
> b/src/libcamera/v4l2_pixelformat.cpp
> > > > > > index efb6f2940235..47baaf60199d 100644
> > > > > > --- a/src/libcamera/v4l2_pixelformat.cpp
> > > > > > +++ b/src/libcamera/v4l2_pixelformat.cpp
> > > > > > @@ -207,6 +207,16 @@ const std::map<V4L2PixelFormat,
> V4L2PixelFormat::Info> vpf2pf{
> > > > > >               { formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
> > > > > >       { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
> > > > > >               { formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
> > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
> > > > > > +             { formats::BGGR16_PISP_COMP1, "16-bit Bayer
> BGBG/GRGR PiSP Compress Mode 1" } },
> > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
> > > > > > +             { formats::GBRG16_PISP_COMP1, "16-bit Bayer
> GBGB/RGRG PiSP Compress Mode 1" } },
> > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
> > > > > > +             { formats::GRBG16_PISP_COMP1, "16-bit Bayer
> GRGR/BGBG PiSP Compress Mode 1" } },
> > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
> > > > > > +             { formats::RGGB16_PISP_COMP1, "16-bit Bayer
> RGRG/GBGB PiSP Compress Mode 1" } },
> > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
> > > > > > +             { formats::MONO_PISP_COMP1, "16-bit Mono PiSP
> Compress Mode 1" } },
> > > > > >
> > > > > >       /* Compressed formats. */
> > > > > >       { V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > > > > > diff --git a/src/libcamera/v4l2_subdevice.cpp
> b/src/libcamera/v4l2_subdevice.cpp
> > > > > > index 6d0785b7b484..aea90abaf9ef 100644
> > > > > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > > > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > > > > @@ -134,6 +134,10 @@ const std::map<uint32_t,
> V4L2SubdeviceFormatInfo> formatInfoMap = {
> > > > > >       { MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12",
> PixelFormatInfo::ColourEncodingRAW } },
> > > > > >       { MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12",
> PixelFormatInfo::ColourEncodingRAW } },
> > > > > >       { MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12",
> PixelFormatInfo::ColourEncodingRAW } },
> > > > > > +     { MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16",
> PixelFormatInfo::ColourEncodingRAW } },
> > > > > > +     { MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16",
> PixelFormatInfo::ColourEncodingRAW } },
> > > > > > +     { MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16",
> PixelFormatInfo::ColourEncodingRAW } },
> > > > > > +     { MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16",
> PixelFormatInfo::ColourEncodingRAW } },
> > > > > >       /* \todo Clarify colour encoding for HSV formats */
> > > > > >       { MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32",
> PixelFormatInfo::ColourEncodingRGB } },
> > > > > >       { MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8",
> PixelFormatInfo::ColourEncodingYUV } },
> > > > > > --
> > > > > > 2.34.1
> > > > > >
>
Jacopo Mondi Feb. 29, 2024, 7:40 a.m. UTC | #9
Hi Naush

On Wed, Feb 28, 2024 at 05:19:47PM +0000, Naushir Patuck wrote:
> On Wed, 28 Feb 2024, 5:16 pm Jacopo Mondi, <jacopo.mondi@ideasonboard.com>
> wrote:
>
> > Hi Naush
> >
> > On Wed, Feb 28, 2024 at 04:45:54PM +0000, Naushir Patuck wrote:
> > > Hi Jacopo,
> > >
> > > > > > > -
> > > > > > > +     { formats::BGGR16_PISP_COMP1, {
> > > > > > > +             .name = "BGGR16_PISP_COMP1",
> > > > > > > +             .format = formats::BGGR16_PISP_COMP1,
> > > > > > > +             .v4l2Formats = {
> > V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
> > > > > > > +             .bitsPerPixel = 16,
> > > > > > > +             .colourEncoding =
> > PixelFormatInfo::ColourEncodingRAW,
> > > > > > > +             .packed = true,
> > > > > > > +             .pixelsPerGroup = 2,
> > > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > > +     } },
> > > > > >
> > > > > > For regular 16-bit formats this should be
> > > > > >
> > > > > >                 .pixelsPerGroup = 2,
> > > > > >                 .planes = {{ { 4, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > >
> > > > > > As 2 pixels of 16 bits each consume 4 bytes. Is this {2, 1}
> > > > > > intentional and due to compression ?
> > > > >
> > > > > Yes, I used {2, 1} to denote the 8-bit/sample after compression.  Do
> > > > > you think I should change to {4, 1}?
> > > >
> > > > Depends on how many bytes the format actually consumes :)
> > > >
> > > > For 16bpp RAW formats, we have pixelsPerGroup = 2 and each sample
> > > > consumes 2 bytes.
> > > >
> > > > According to the pixelsPerGroup definition:
> > > >
> > > >  * A pixel group is defined as the minimum number of pixels (including
> > padding)
> > > >  * necessary in a row when the image has only one column of effective
> > pixels.
> > > >
> > > > So for a RAW Bayer formats, to get a complete full-color "superpixel"
> > > > we need at least two pixels per row.
> > > >
> > > >         RG
> > > >         GB
> > > >
> > > > Each "RG" or "GB" pair is composed by 2 16-bit samples, for a total of
> > > > 32bits of data, from which the {4, 1} value in planes[0]
> > > >
> > > > According to "Table 2", Chapter 5 of the PiSP datasheet, the
> > > > compressed formats are described as
> > > >
> > > >      8-bits per pixel single channel compressed with mode n, for n =
> > 1, 2, 3
> > > >
> > > > Which suggests each "RG" or "GB" pair is actually 2 samples of 8 bits
> > > > each, for a plane size of {2, 1}.
> > > >
> > > > Is this correct ? Is the actual .bitsPerPixel value for the compressed
> > > > formats 8 instead of 16 then ?
> > >
> > > This is correct, each sample is 8-bits so "RG"/"GB" pairs are 16-bits
> > each.
> > > So it sounds like I need to use .planes =  {2, 1}, but .bitsPerPixel = 8
> > maybe?
> >
> > I now wonder if the '16' in the format names is correct then :)
> >
>
> It's meant to signify 16-bits compressed to 8-bits (in this case).  Maybe a
> defined naming scheme is needed for such cases?
>

What about simply leaving the bitdpeth out from the format name like
it is done for the V4L2 pixel formats ?

#define V4L2_PIX_FMT_PISP_COMP1_RGGB   v4l2_fourcc('P', 'C', '1', 'R') /* PiSP 8-bit mode 1 compressed RGGB bayer */

Something like

     { formats::BGGR_PISP_COMP1, {
             .name = "BGGR_PISP_COMP1",
             .format = formats::BGGR_PISP_COMP1,
             .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
             .bitsPerPixel = 8,
             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
             .packed = true,
             .pixelsPerGroup = 2,
             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
     } },

?

>
> > >
> > > Naush
> > >
> > > >
> > > > Thanks
> > > >   j
> > > >
> > > > >
> > > > > Regards,
> > > > > Naush
> > > > >
> > > > >
> > > > > >
> > > > > > > +     { formats::GBRG16_PISP_COMP1, {
> > > > > > > +             .name = "GBRG16_PISP_COMP1",
> > > > > > > +             .format = formats::GBRG16_PISP_COMP1,
> > > > > > > +             .v4l2Formats = {
> > V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
> > > > > > > +             .bitsPerPixel = 16,
> > > > > > > +             .colourEncoding =
> > PixelFormatInfo::ColourEncodingRAW,
> > > > > > > +             .packed = true,
> > > > > > > +             .pixelsPerGroup = 2,
> > > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > > +     } },
> > > > > > > +     { formats::GRBG16_PISP_COMP1, {
> > > > > > > +             .name = "GRBG16_PISP_COMP1",
> > > > > > > +             .format = formats::GRBG16_PISP_COMP1,
> > > > > > > +             .v4l2Formats = {
> > V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
> > > > > > > +             .bitsPerPixel = 16,
> > > > > > > +             .colourEncoding =
> > PixelFormatInfo::ColourEncodingRAW,
> > > > > > > +             .packed = true,
> > > > > > > +             .pixelsPerGroup = 2,
> > > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > > +     } },
> > > > > > > +     { formats::RGGB16_PISP_COMP1, {
> > > > > > > +             .name = "RGGB16_PISP_COMP1",
> > > > > > > +             .format = formats::RGGB16_PISP_COMP1,
> > > > > > > +             .v4l2Formats = {
> > V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
> > > > > > > +             .bitsPerPixel = 16,
> > > > > > > +             .colourEncoding =
> > PixelFormatInfo::ColourEncodingRAW,
> > > > > > > +             .packed = true,
> > > > > > > +             .pixelsPerGroup = 2,
> > > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > > +     } },
> > > > > > >       /* Compressed formats. */
> > > > > > >       { formats::MJPEG, {
> > > > > > >               .name = "MJPEG",
> > > > > > > diff --git a/src/libcamera/formats.yaml
> > b/src/libcamera/formats.yaml
> > > > > > > index bde2cc803b98..f6df721243d0 100644
> > > > > > > --- a/src/libcamera/formats.yaml
> > > > > > > +++ b/src/libcamera/formats.yaml
> > > > > > > @@ -190,4 +190,20 @@ formats:
> > > > > > >    - SBGGR10_IPU3:
> > > > > > >        fourcc: DRM_FORMAT_SBGGR10
> > > > > > >        mod: IPU3_FORMAT_MOD_PACKED
> > > > > > > +
> > > > > > > +  - RGGB16_PISP_COMP1:
> > > > > > > +      fourcc: DRM_FORMAT_SRGGB16
> > > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > > +  - GRBG16_PISP_COMP1:
> > > > > > > +      fourcc: DRM_FORMAT_SGRBG16
> > > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > > +  - GBRG16_PISP_COMP1:
> > > > > > > +      fourcc: DRM_FORMAT_SGBRG16
> > > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > > +  - BGGR16_PISP_COMP1:
> > > > > > > +      fourcc: DRM_FORMAT_SBGGR16
> > > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > > +  - MONO_PISP_COMP1:
> > > > > > > +      fourcc: DRM_FORMAT_R16
> > > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > >  ...
> > > > > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp
> > b/src/libcamera/v4l2_pixelformat.cpp
> > > > > > > index efb6f2940235..47baaf60199d 100644
> > > > > > > --- a/src/libcamera/v4l2_pixelformat.cpp
> > > > > > > +++ b/src/libcamera/v4l2_pixelformat.cpp
> > > > > > > @@ -207,6 +207,16 @@ const std::map<V4L2PixelFormat,
> > V4L2PixelFormat::Info> vpf2pf{
> > > > > > >               { formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
> > > > > > >       { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
> > > > > > >               { formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
> > > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
> > > > > > > +             { formats::BGGR16_PISP_COMP1, "16-bit Bayer
> > BGBG/GRGR PiSP Compress Mode 1" } },
> > > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
> > > > > > > +             { formats::GBRG16_PISP_COMP1, "16-bit Bayer
> > GBGB/RGRG PiSP Compress Mode 1" } },
> > > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
> > > > > > > +             { formats::GRBG16_PISP_COMP1, "16-bit Bayer
> > GRGR/BGBG PiSP Compress Mode 1" } },
> > > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
> > > > > > > +             { formats::RGGB16_PISP_COMP1, "16-bit Bayer
> > RGRG/GBGB PiSP Compress Mode 1" } },
> > > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
> > > > > > > +             { formats::MONO_PISP_COMP1, "16-bit Mono PiSP
> > Compress Mode 1" } },
> > > > > > >
> > > > > > >       /* Compressed formats. */
> > > > > > >       { V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > > > > > > diff --git a/src/libcamera/v4l2_subdevice.cpp
> > b/src/libcamera/v4l2_subdevice.cpp
> > > > > > > index 6d0785b7b484..aea90abaf9ef 100644
> > > > > > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > > > > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > > > > > @@ -134,6 +134,10 @@ const std::map<uint32_t,
> > V4L2SubdeviceFormatInfo> formatInfoMap = {
> > > > > > >       { MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12",
> > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > >       { MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12",
> > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > >       { MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12",
> > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > +     { MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16",
> > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > +     { MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16",
> > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > +     { MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16",
> > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > +     { MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16",
> > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > >       /* \todo Clarify colour encoding for HSV formats */
> > > > > > >       { MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32",
> > PixelFormatInfo::ColourEncodingRGB } },
> > > > > > >       { MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8",
> > PixelFormatInfo::ColourEncodingYUV } },
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > >
> >
Naushir Patuck Feb. 29, 2024, 8:18 a.m. UTC | #10
Hey Jacopo,

On Thu, 29 Feb 2024 at 07:40, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Wed, Feb 28, 2024 at 05:19:47PM +0000, Naushir Patuck wrote:
> > On Wed, 28 Feb 2024, 5:16 pm Jacopo Mondi, <jacopo.mondi@ideasonboard.com>
> > wrote:
> >
> > > Hi Naush
> > >
> > > On Wed, Feb 28, 2024 at 04:45:54PM +0000, Naushir Patuck wrote:
> > > > Hi Jacopo,
> > > >
> > > > > > > > -
> > > > > > > > +     { formats::BGGR16_PISP_COMP1, {
> > > > > > > > +             .name = "BGGR16_PISP_COMP1",
> > > > > > > > +             .format = formats::BGGR16_PISP_COMP1,
> > > > > > > > +             .v4l2Formats = {
> > > V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
> > > > > > > > +             .bitsPerPixel = 16,
> > > > > > > > +             .colourEncoding =
> > > PixelFormatInfo::ColourEncodingRAW,
> > > > > > > > +             .packed = true,
> > > > > > > > +             .pixelsPerGroup = 2,
> > > > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > > > +     } },
> > > > > > >
> > > > > > > For regular 16-bit formats this should be
> > > > > > >
> > > > > > >                 .pixelsPerGroup = 2,
> > > > > > >                 .planes = {{ { 4, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > >
> > > > > > > As 2 pixels of 16 bits each consume 4 bytes. Is this {2, 1}
> > > > > > > intentional and due to compression ?
> > > > > >
> > > > > > Yes, I used {2, 1} to denote the 8-bit/sample after compression.  Do
> > > > > > you think I should change to {4, 1}?
> > > > >
> > > > > Depends on how many bytes the format actually consumes :)
> > > > >
> > > > > For 16bpp RAW formats, we have pixelsPerGroup = 2 and each sample
> > > > > consumes 2 bytes.
> > > > >
> > > > > According to the pixelsPerGroup definition:
> > > > >
> > > > >  * A pixel group is defined as the minimum number of pixels (including
> > > padding)
> > > > >  * necessary in a row when the image has only one column of effective
> > > pixels.
> > > > >
> > > > > So for a RAW Bayer formats, to get a complete full-color "superpixel"
> > > > > we need at least two pixels per row.
> > > > >
> > > > >         RG
> > > > >         GB
> > > > >
> > > > > Each "RG" or "GB" pair is composed by 2 16-bit samples, for a total of
> > > > > 32bits of data, from which the {4, 1} value in planes[0]
> > > > >
> > > > > According to "Table 2", Chapter 5 of the PiSP datasheet, the
> > > > > compressed formats are described as
> > > > >
> > > > >      8-bits per pixel single channel compressed with mode n, for n =
> > > 1, 2, 3
> > > > >
> > > > > Which suggests each "RG" or "GB" pair is actually 2 samples of 8 bits
> > > > > each, for a plane size of {2, 1}.
> > > > >
> > > > > Is this correct ? Is the actual .bitsPerPixel value for the compressed
> > > > > formats 8 instead of 16 then ?
> > > >
> > > > This is correct, each sample is 8-bits so "RG"/"GB" pairs are 16-bits
> > > each.
> > > > So it sounds like I need to use .planes =  {2, 1}, but .bitsPerPixel = 8
> > > maybe?
> > >
> > > I now wonder if the '16' in the format names is correct then :)
> > >
> >
> > It's meant to signify 16-bits compressed to 8-bits (in this case).  Maybe a
> > defined naming scheme is needed for such cases?
> >
>
> What about simply leaving the bitdpeth out from the format name like
> it is done for the V4L2 pixel formats ?

That sounds good to me!  I'll make the change in the next patch.

>
> #define V4L2_PIX_FMT_PISP_COMP1_RGGB   v4l2_fourcc('P', 'C', '1', 'R') /* PiSP 8-bit mode 1 compressed RGGB bayer */
>
> Something like
>
>      { formats::BGGR_PISP_COMP1, {
>              .name = "BGGR_PISP_COMP1",
>              .format = formats::BGGR_PISP_COMP1,
>              .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
>              .bitsPerPixel = 8,
>              .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
>              .packed = true,
>              .pixelsPerGroup = 2,
>              .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
>      } },
>
> ?
>
> >
> > > >
> > > > Naush
> > > >
> > > > >
> > > > > Thanks
> > > > >   j
> > > > >
> > > > > >
> > > > > > Regards,
> > > > > > Naush
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > +     { formats::GBRG16_PISP_COMP1, {
> > > > > > > > +             .name = "GBRG16_PISP_COMP1",
> > > > > > > > +             .format = formats::GBRG16_PISP_COMP1,
> > > > > > > > +             .v4l2Formats = {
> > > V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
> > > > > > > > +             .bitsPerPixel = 16,
> > > > > > > > +             .colourEncoding =
> > > PixelFormatInfo::ColourEncodingRAW,
> > > > > > > > +             .packed = true,
> > > > > > > > +             .pixelsPerGroup = 2,
> > > > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > > > +     } },
> > > > > > > > +     { formats::GRBG16_PISP_COMP1, {
> > > > > > > > +             .name = "GRBG16_PISP_COMP1",
> > > > > > > > +             .format = formats::GRBG16_PISP_COMP1,
> > > > > > > > +             .v4l2Formats = {
> > > V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
> > > > > > > > +             .bitsPerPixel = 16,
> > > > > > > > +             .colourEncoding =
> > > PixelFormatInfo::ColourEncodingRAW,
> > > > > > > > +             .packed = true,
> > > > > > > > +             .pixelsPerGroup = 2,
> > > > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > > > +     } },
> > > > > > > > +     { formats::RGGB16_PISP_COMP1, {
> > > > > > > > +             .name = "RGGB16_PISP_COMP1",
> > > > > > > > +             .format = formats::RGGB16_PISP_COMP1,
> > > > > > > > +             .v4l2Formats = {
> > > V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
> > > > > > > > +             .bitsPerPixel = 16,
> > > > > > > > +             .colourEncoding =
> > > PixelFormatInfo::ColourEncodingRAW,
> > > > > > > > +             .packed = true,
> > > > > > > > +             .pixelsPerGroup = 2,
> > > > > > > > +             .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > > > > > > +     } },
> > > > > > > >       /* Compressed formats. */
> > > > > > > >       { formats::MJPEG, {
> > > > > > > >               .name = "MJPEG",
> > > > > > > > diff --git a/src/libcamera/formats.yaml
> > > b/src/libcamera/formats.yaml
> > > > > > > > index bde2cc803b98..f6df721243d0 100644
> > > > > > > > --- a/src/libcamera/formats.yaml
> > > > > > > > +++ b/src/libcamera/formats.yaml
> > > > > > > > @@ -190,4 +190,20 @@ formats:
> > > > > > > >    - SBGGR10_IPU3:
> > > > > > > >        fourcc: DRM_FORMAT_SBGGR10
> > > > > > > >        mod: IPU3_FORMAT_MOD_PACKED
> > > > > > > > +
> > > > > > > > +  - RGGB16_PISP_COMP1:
> > > > > > > > +      fourcc: DRM_FORMAT_SRGGB16
> > > > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > > > +  - GRBG16_PISP_COMP1:
> > > > > > > > +      fourcc: DRM_FORMAT_SGRBG16
> > > > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > > > +  - GBRG16_PISP_COMP1:
> > > > > > > > +      fourcc: DRM_FORMAT_SGBRG16
> > > > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > > > +  - BGGR16_PISP_COMP1:
> > > > > > > > +      fourcc: DRM_FORMAT_SBGGR16
> > > > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > > > +  - MONO_PISP_COMP1:
> > > > > > > > +      fourcc: DRM_FORMAT_R16
> > > > > > > > +      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
> > > > > > > >  ...
> > > > > > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp
> > > b/src/libcamera/v4l2_pixelformat.cpp
> > > > > > > > index efb6f2940235..47baaf60199d 100644
> > > > > > > > --- a/src/libcamera/v4l2_pixelformat.cpp
> > > > > > > > +++ b/src/libcamera/v4l2_pixelformat.cpp
> > > > > > > > @@ -207,6 +207,16 @@ const std::map<V4L2PixelFormat,
> > > V4L2PixelFormat::Info> vpf2pf{
> > > > > > > >               { formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
> > > > > > > >       { V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
> > > > > > > >               { formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
> > > > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
> > > > > > > > +             { formats::BGGR16_PISP_COMP1, "16-bit Bayer
> > > BGBG/GRGR PiSP Compress Mode 1" } },
> > > > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
> > > > > > > > +             { formats::GBRG16_PISP_COMP1, "16-bit Bayer
> > > GBGB/RGRG PiSP Compress Mode 1" } },
> > > > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
> > > > > > > > +             { formats::GRBG16_PISP_COMP1, "16-bit Bayer
> > > GRGR/BGBG PiSP Compress Mode 1" } },
> > > > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
> > > > > > > > +             { formats::RGGB16_PISP_COMP1, "16-bit Bayer
> > > RGRG/GBGB PiSP Compress Mode 1" } },
> > > > > > > > +     { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
> > > > > > > > +             { formats::MONO_PISP_COMP1, "16-bit Mono PiSP
> > > Compress Mode 1" } },
> > > > > > > >
> > > > > > > >       /* Compressed formats. */
> > > > > > > >       { V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > > > > > > > diff --git a/src/libcamera/v4l2_subdevice.cpp
> > > b/src/libcamera/v4l2_subdevice.cpp
> > > > > > > > index 6d0785b7b484..aea90abaf9ef 100644
> > > > > > > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > > > > > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > > > > > > @@ -134,6 +134,10 @@ const std::map<uint32_t,
> > > V4L2SubdeviceFormatInfo> formatInfoMap = {
> > > > > > > >       { MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12",
> > > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > >       { MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12",
> > > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > >       { MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12",
> > > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > > +     { MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16",
> > > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > > +     { MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16",
> > > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > > +     { MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16",
> > > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > > +     { MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16",
> > > PixelFormatInfo::ColourEncodingRAW } },
> > > > > > > >       /* \todo Clarify colour encoding for HSV formats */
> > > > > > > >       { MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32",
> > > PixelFormatInfo::ColourEncodingRGB } },
> > > > > > > >       { MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8",
> > > PixelFormatInfo::ColourEncodingYUV } },
> > > > > > > > --
> > > > > > > > 2.34.1
> > > > > > > >
> > >

Patch
diff mbox series

diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
index 78ba3969913d..164743f7e9f6 100644
--- a/include/libcamera/internal/bayer_format.h
+++ b/include/libcamera/internal/bayer_format.h
@@ -34,6 +34,8 @@  public:
 		None = 0,
 		CSI2 = 1,
 		IPU3 = 2,
+		PISP1 = 3,
+		PISP2 = 4,
 	};
 
 	constexpr BayerFormat()
diff --git a/include/linux/drm_fourcc.h b/include/linux/drm_fourcc.h
index 4ee421b95730..eff27fbe5a1e 100644
--- a/include/linux/drm_fourcc.h
+++ b/include/linux/drm_fourcc.h
@@ -490,6 +490,7 @@  extern "C" {
 #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
 #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
 #define DRM_FORMAT_MOD_VENDOR_MIPI 0x0b
+#define DRM_FORMAT_MOD_VENDOR_RPI 0x0c
 
 /* add more to the end as needed */
 
@@ -1670,6 +1671,9 @@  drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
  */
 #define MIPI_FORMAT_MOD_CSI2_PACKED fourcc_mod_code(MIPI, 1)
 
+#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)
+#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)
+
 #if defined(__cplusplus)
 }
 #endif
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index ba48d2c89726..59af6f794680 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -789,6 +789,18 @@  struct v4l2_pix_format {
 #define V4L2_PIX_FMT_IPU3_SGRBG10	v4l2_fourcc('i', 'p', '3', 'G') /* IPU3 packed 10-bit GRBG bayer */
 #define V4L2_PIX_FMT_IPU3_SRGGB10	v4l2_fourcc('i', 'p', '3', 'r') /* IPU3 packed 10-bit RGGB bayer */
 
+/* The pixel format for all our buffers (the precise format is found in the config buffer). */
+#define V4L2_PIX_FMT_PISP_COMP1_RGGB	v4l2_fourcc('P', 'C', '1', 'R')
+#define V4L2_PIX_FMT_PISP_COMP1_GRBG	v4l2_fourcc('P', 'C', '1', 'G')
+#define V4L2_PIX_FMT_PISP_COMP1_GBRG	v4l2_fourcc('P', 'C', '1', 'g')
+#define V4L2_PIX_FMT_PISP_COMP1_BGGR	v4l2_fourcc('P', 'C', '1', 'B')
+#define V4L2_PIX_FMT_PISP_COMP1_MONO	v4l2_fourcc('P', 'C', '1', 'M')
+#define V4L2_PIX_FMT_PISP_COMP2_RGGB	v4l2_fourcc('P', 'C', '2', 'R')
+#define V4L2_PIX_FMT_PISP_COMP2_GRBG	v4l2_fourcc('P', 'C', '2', 'G')
+#define V4L2_PIX_FMT_PISP_COMP2_GBRG	v4l2_fourcc('P', 'C', '2', 'g')
+#define V4L2_PIX_FMT_PISP_COMP2_BGGR	v4l2_fourcc('P', 'C', '2', 'B')
+#define V4L2_PIX_FMT_PISP_COMP2_MONO	v4l2_fourcc('P', 'C', '2', 'M')
+
 /* SDR formats - used only for Software Defined Radio devices */
 #define V4L2_SDR_FMT_CU8          v4l2_fourcc('C', 'U', '0', '8') /* IQ u8 */
 #define V4L2_SDR_FMT_CU16LE       v4l2_fourcc('C', 'U', '1', '6') /* IQ u16le */
@@ -818,6 +830,9 @@  struct v4l2_pix_format {
 #define V4L2_META_FMT_RK_ISP1_PARAMS	v4l2_fourcc('R', 'K', '1', 'P') /* Rockchip ISP1 3A Parameters */
 #define V4L2_META_FMT_RK_ISP1_STAT_3A	v4l2_fourcc('R', 'K', '1', 'S') /* Rockchip ISP1 3A Statistics */
 
+/* Vendor specific - used for RaspberryPi PiSP */
+#define V4L2_META_FMT_RPI_BE_CFG v4l2_fourcc('R', 'P', 'B', 'C') /* PiSP BE configuration */
+
 /* priv field value to indicates that subsequent fields are valid. */
 #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe
 
diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
index 20aedfa6d925..333b1117f531 100644
--- a/src/libcamera/bayer_format.cpp
+++ b/src/libcamera/bayer_format.cpp
@@ -164,6 +164,14 @@  const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
 		{ formats::SGRBG16, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG16) } },
 	{ { BayerFormat::RGGB, 16, BayerFormat::Packing::None },
 		{ formats::SRGGB16, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) } },
+	{ { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 },
+		{ formats::BGGR16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR) } },
+	{ { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 },
+		{ formats::GBRG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG) } },
+	{ { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 },
+		{ formats::GRBG16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG) } },
+	{ { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 },
+		{ formats::RGGB16_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } },
 	{ { BayerFormat::MONO, 8, BayerFormat::Packing::None },
 		{ formats::R8, V4L2PixelFormat(V4L2_PIX_FMT_GREY) } },
 	{ { BayerFormat::MONO, 10, BayerFormat::Packing::None },
@@ -174,6 +182,8 @@  const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
 		{ formats::R12, V4L2PixelFormat(V4L2_PIX_FMT_Y12) } },
 	{ { BayerFormat::MONO, 16, BayerFormat::Packing::None },
 		{ formats::R16, V4L2PixelFormat(V4L2_PIX_FMT_Y16) } },
+	{ { BayerFormat::MONO, 16, BayerFormat::Packing::PISP1 },
+		{ formats::MONO_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO) } },
 };
 
 const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
@@ -209,6 +219,10 @@  const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
 	{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::None } },
 	{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::None } },
 	{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::None } },
+	{ MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::Packing::PISP1 } },
+	{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::Packing::PISP1 } },
+	{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::Packing::PISP1 } },
+	{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 } },
 	{ MEDIA_BUS_FMT_Y8_1X8, { BayerFormat::MONO, 8, BayerFormat::Packing::None } },
 	{ MEDIA_BUS_FMT_Y10_1X10, { BayerFormat::MONO, 10, BayerFormat::Packing::None } },
 	{ MEDIA_BUS_FMT_Y12_1X12, { BayerFormat::MONO, 12, BayerFormat::Packing::None } },
@@ -303,6 +317,10 @@  std::ostream &operator<<(std::ostream &out, const BayerFormat &f)
 		out << "-CSI2P";
 	else if (f.packing == BayerFormat::Packing::IPU3)
 		out << "-IPU3P";
+	else if (f.packing == BayerFormat::Packing::PISP1)
+		out << "-PISP1";
+	else if (f.packing == BayerFormat::Packing::PISP2)
+		out << "-PISP2";
 
 	return out;
 }
diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
index a674f4179cc8..e603a7eda579 100644
--- a/src/libcamera/formats.cpp
+++ b/src/libcamera/formats.cpp
@@ -547,6 +547,16 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.pixelsPerGroup = 1,
 		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
 	} },
+	{ formats::MONO_PISP_COMP1, {
+		.name = "MONO_PISP_COMP1",
+		.format = formats::MONO_PISP_COMP1,
+		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO), },
+		.bitsPerPixel = 16,
+		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
+		.packed = true,
+		.pixelsPerGroup = 1,
+		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
+	} },
 
 	/* Bayer formats. */
 	{ formats::SBGGR8, {
@@ -910,7 +920,46 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 		.pixelsPerGroup = 25,
 		.planes = {{ { 32, 1 }, { 0, 0 }, { 0, 0 } }},
 	} },
-
+	{ formats::BGGR16_PISP_COMP1, {
+		.name = "BGGR16_PISP_COMP1",
+		.format = formats::BGGR16_PISP_COMP1,
+		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR), },
+		.bitsPerPixel = 16,
+		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
+		.packed = true,
+		.pixelsPerGroup = 2,
+		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
+	} },
+	{ formats::GBRG16_PISP_COMP1, {
+		.name = "GBRG16_PISP_COMP1",
+		.format = formats::GBRG16_PISP_COMP1,
+		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG), },
+		.bitsPerPixel = 16,
+		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
+		.packed = true,
+		.pixelsPerGroup = 2,
+		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
+	} },
+	{ formats::GRBG16_PISP_COMP1, {
+		.name = "GRBG16_PISP_COMP1",
+		.format = formats::GRBG16_PISP_COMP1,
+		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG), },
+		.bitsPerPixel = 16,
+		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
+		.packed = true,
+		.pixelsPerGroup = 2,
+		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
+	} },
+	{ formats::RGGB16_PISP_COMP1, {
+		.name = "RGGB16_PISP_COMP1",
+		.format = formats::RGGB16_PISP_COMP1,
+		.v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
+		.bitsPerPixel = 16,
+		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
+		.packed = true,
+		.pixelsPerGroup = 2,
+		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
+	} },
 	/* Compressed formats. */
 	{ formats::MJPEG, {
 		.name = "MJPEG",
diff --git a/src/libcamera/formats.yaml b/src/libcamera/formats.yaml
index bde2cc803b98..f6df721243d0 100644
--- a/src/libcamera/formats.yaml
+++ b/src/libcamera/formats.yaml
@@ -190,4 +190,20 @@  formats:
   - SBGGR10_IPU3:
       fourcc: DRM_FORMAT_SBGGR10
       mod: IPU3_FORMAT_MOD_PACKED
+
+  - RGGB16_PISP_COMP1:
+      fourcc: DRM_FORMAT_SRGGB16
+      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
+  - GRBG16_PISP_COMP1:
+      fourcc: DRM_FORMAT_SGRBG16
+      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
+  - GBRG16_PISP_COMP1:
+      fourcc: DRM_FORMAT_SGBRG16
+      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
+  - BGGR16_PISP_COMP1:
+      fourcc: DRM_FORMAT_SBGGR16
+      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
+  - MONO_PISP_COMP1:
+      fourcc: DRM_FORMAT_R16
+      mod: PISP_FORMAT_MOD_COMPRESS_MODE1
 ...
diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
index efb6f2940235..47baaf60199d 100644
--- a/src/libcamera/v4l2_pixelformat.cpp
+++ b/src/libcamera/v4l2_pixelformat.cpp
@@ -207,6 +207,16 @@  const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{
 		{ formats::SGRBG16, "16-bit Bayer GRGR/BGBG" } },
 	{ V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16),
 		{ formats::SRGGB16, "16-bit Bayer RGRG/GBGB" } },
+	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_BGGR),
+		{ formats::BGGR16_PISP_COMP1, "16-bit Bayer BGBG/GRGR PiSP Compress Mode 1" } },
+	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GBRG),
+		{ formats::GBRG16_PISP_COMP1, "16-bit Bayer GBGB/RGRG PiSP Compress Mode 1" } },
+	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_GRBG),
+		{ formats::GRBG16_PISP_COMP1, "16-bit Bayer GRGR/BGBG PiSP Compress Mode 1" } },
+	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB),
+		{ formats::RGGB16_PISP_COMP1, "16-bit Bayer RGRG/GBGB PiSP Compress Mode 1" } },
+	{ V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_MONO),
+		{ formats::MONO_PISP_COMP1, "16-bit Mono PiSP Compress Mode 1" } },
 
 	/* Compressed formats. */
 	{ V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
index 6d0785b7b484..aea90abaf9ef 100644
--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -134,6 +134,10 @@  const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
 	{ MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
 	{ MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
 	{ MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12", PixelFormatInfo::ColourEncodingRAW } },
+	{ MEDIA_BUS_FMT_SBGGR16_1X16, { 16, "SBGGR16_1x16", PixelFormatInfo::ColourEncodingRAW } },
+	{ MEDIA_BUS_FMT_SGBRG16_1X16, { 16, "SGBRG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
+	{ MEDIA_BUS_FMT_SGRBG16_1X16, { 16, "SGRBG16_1x16", PixelFormatInfo::ColourEncodingRAW } },
+	{ MEDIA_BUS_FMT_SRGGB16_1X16, { 16, "SRGGB16_1x16", PixelFormatInfo::ColourEncodingRAW } },
 	/* \todo Clarify colour encoding for HSV formats */
 	{ MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32", PixelFormatInfo::ColourEncodingRGB } },
 	{ MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8", PixelFormatInfo::ColourEncodingYUV } },