[PATCH kms++ 0/4] Support Y210

[PATCH kms++ 0/4] Support Y210

[Tomi Valkeinen]

From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Hi,

These kms++ patches add support for Y210 format.

I didn't find a super clear description of the byte order for Y210
anywhere.  If someone knows what it is supposed to be, I'd appreciate
verifying the code =).

 Tomi

Tomi Valkeinen (4):
  kms++: PixelFormats: Fix formatting
  kms++: PixelFormats: Add Y210 format
  kms++util: Add endian.h
  kms++util: Add Y210 drawing support

 kms++/inc/kms++/pixelformats.h   |  2 +
 kms++/src/pixelformats.cpp       | 66 ++++++++------------------------
 kms++util/inc/kms++util/endian.h | 46 ++++++++++++++++++++++
 kms++util/src/drawing.cpp        | 31 +++++++++++++++
 4 files changed, 95 insertions(+), 50 deletions(-)
 create mode 100644 kms++util/inc/kms++util/endian.h

-- 
2.34.1

[PATCH kms++ 1/4] kms++: PixelFormats: Fix formatting

[Tomi Valkeinen]

Fix formatting for some pixel formats.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 kms++/src/pixelformats.cpp | 60 +++++++-------------------------------
 1 file changed, 10 insertions(+), 50 deletions(-)

diff --git a/kms++/src/pixelformats.cpp b/kms++/src/pixelformats.cpp
index d739efd..7afbf09 100644
--- a/kms++/src/pixelformats.cpp
+++ b/kms++/src/pixelformats.cpp
@@ -32,107 +32,67 @@ static const map<PixelFormat, PixelFormatInfo> format_info_array = {
 	{ PixelFormat::NV12, {
 				     PixelColorType::YUV,
 				     2,
-				     { {
-					       8,
-					       1,
-					       1,
-				       },
+				     { { 8, 1, 1 },
 				       { 8, 2, 2 } },
 			     } },
 	{ PixelFormat::NV21, {
 				     PixelColorType::YUV,
 				     2,
-				     { {
-					       8,
-					       1,
-					       1,
-				       },
+				     { { 8, 1, 1 },
 				       { 8, 2, 2 } },
 			     } },
 	{ PixelFormat::NV16, {
 				     PixelColorType::YUV,
 				     2,
-				     { {
-					       8,
-					       1,
-					       1,
-				       },
+				     { { 8, 1, 1 },
 				       { 8, 2, 1 } },
 			     } },
 	{ PixelFormat::NV61, {
 				     PixelColorType::YUV,
 				     2,
-				     { {
-					       8,
-					       1,
-					       1,
-				       },
+				     { { 8, 1, 1 },
 				       { 8, 2, 1 } },
 			     } },
 	/* YUV planar */
 	{ PixelFormat::YUV420, {
 				       PixelColorType::YUV,
 				       3,
-				       { {
-						 8,
-						 1,
-						 1,
-					 },
+				       { { 8, 1, 1 },
 					 { 8, 2, 2 },
 					 { 8, 2, 2 } },
 			       } },
 	{ PixelFormat::YVU420, {
 				       PixelColorType::YUV,
 				       3,
-				       { {
-						 8,
-						 1,
-						 1,
-					 },
+				       { { 8, 1, 1 },
 					 { 8, 2, 2 },
 					 { 8, 2, 2 } },
 			       } },
 	{ PixelFormat::YUV422, {
 				       PixelColorType::YUV,
 				       3,
-				       { {
-						 8,
-						 1,
-						 1,
-					 },
+				       { { 8, 1, 1 },
 					 { 8, 2, 1 },
 					 { 8, 2, 1 } },
 			       } },
 	{ PixelFormat::YVU422, {
 				       PixelColorType::YUV,
 				       3,
-				       { {
-						 8,
-						 1,
-						 1,
-					 },
+				       { { 8, 1, 1 },
 					 { 8, 2, 1 },
 					 { 8, 2, 1 } },
 			       } },
 	{ PixelFormat::YUV444, {
 				       PixelColorType::YUV,
 				       3,
-				       { {
-						 8,
-						 1,
-						 1,
-					 },
+				       { { 8, 1, 1 },
 					 { 8, 1, 1 },
 					 { 8, 1, 1 } },
 			       } },
 	{ PixelFormat::YVU444, {
 				       PixelColorType::YUV,
 				       3,
-				       { {
-						 8,
-						 1,
-						 1,
-					 },
+				       { { 8, 1, 1 },
 					 { 8, 1, 1 },
 					 { 8, 1, 1 } },
 			       } },
-- 
2.34.1

[PATCH kms++ 2/4] kms++: PixelFormats: Add Y210 format

[Tomi Valkeinen]

Add Y210 pixel format.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 kms++/inc/kms++/pixelformats.h | 2 ++
 kms++/src/pixelformats.cpp     | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/kms++/inc/kms++/pixelformats.h b/kms++/inc/kms++/pixelformats.h
index 6f2671b..35261a9 100644
--- a/kms++/inc/kms++/pixelformats.h
+++ b/kms++/inc/kms++/pixelformats.h
@@ -31,6 +31,8 @@ enum class PixelFormat : uint32_t {
 	YVYU = MakeFourCC("YVYU"),
 	VYUY = MakeFourCC("VYUY"),
 
+	Y210 = MakeFourCC("Y210"),
+
 	XRGB8888 = MakeFourCC("XR24"),
 	XBGR8888 = MakeFourCC("XB24"),
 	RGBX8888 = MakeFourCC("RX24"),
diff --git a/kms++/src/pixelformats.cpp b/kms++/src/pixelformats.cpp
index 7afbf09..89eece4 100644
--- a/kms++/src/pixelformats.cpp
+++ b/kms++/src/pixelformats.cpp
@@ -28,6 +28,12 @@ static const map<PixelFormat, PixelFormatInfo> format_info_array = {
 				     1,
 				     { { 16, 2, 1 } },
 			     } },
+	{ PixelFormat::Y210, {
+				     PixelColorType::YUV,
+				     1,
+				     { { 32, 2, 1 } },
+			     } },
+
 	/* YUV semi-planar */
 	{ PixelFormat::NV12, {
 				     PixelColorType::YUV,
-- 
2.34.1

[PATCH kms++ 3/4] kms++util: Add endian.h

[Tomi Valkeinen]

Add simple endianness supporting write function, and, for now, only one
shortcut helper, write16le().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 kms++util/inc/kms++util/endian.h | 46 ++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 kms++util/inc/kms++util/endian.h

diff --git a/kms++util/inc/kms++util/endian.h b/kms++util/inc/kms++util/endian.h
new file mode 100644
index 0000000..ea09065
--- /dev/null
+++ b/kms++util/inc/kms++util/endian.h
@@ -0,0 +1,46 @@
+#pragma once
+
+#include <type_traits>
+#include <byteswap.h>
+#include <stdint.h>
+
+static_assert((__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) ||
+	      (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__),
+	      "Unable to detect endianness");
+
+enum class endian {
+	little = __ORDER_LITTLE_ENDIAN__,
+	big = __ORDER_BIG_ENDIAN__,
+	native = __BYTE_ORDER__
+};
+
+template<typename T>
+constexpr T byteswap(T value) noexcept
+{
+	static_assert(std::is_integral<T>(), "Type is not integral");
+	static_assert(sizeof(T) == 2 ||
+		      sizeof(T) == 4 ||
+		      sizeof(T) == 8,
+		      "Illegal value size");
+
+	switch (sizeof(T)) {
+		case 2: return bswap_16(value);
+		case 4: return bswap_32(value);
+		case 8: return bswap_64(value);
+	}
+}
+
+template<endian E, typename T>
+static void write_endian(T val, T* dst)
+{
+	if constexpr (E != endian::native)
+		val = byteswap(val);
+
+	*dst = val;
+}
+
+[[maybe_unused]]
+static void write16le(uint16_t val, uint16_t* dst)
+{
+	write_endian<endian::little, uint16_t>(val, dst);
+}
-- 
2.34.1

[PATCH kms++ 4/4] kms++util: Add Y210 drawing support

[Tomi Valkeinen]

Add support for drawing Y210 pixels.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 kms++util/src/drawing.cpp | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp
index 79e0d90..7e1b40b 100644
--- a/kms++util/src/drawing.cpp
+++ b/kms++util/src/drawing.cpp
@@ -3,6 +3,7 @@
 
 #include <kms++/kms++.h>
 #include <kms++util/kms++util.h>
+#include <kms++util/endian.h>
 
 using namespace std;
 
@@ -179,6 +180,32 @@ static void draw_yuv422_packed_macropixel(IFramebuffer& buf, unsigned x, unsigne
 	}
 }
 
+static void draw_y2xx_packed_macropixel(IFramebuffer& buf, unsigned x, unsigned y,
+					  YUV yuv1, YUV yuv2)
+{
+	const uint32_t macro_size = 4;
+	uint16_t* p = (uint16_t*)(buf.map(0) + buf.stride(0) * y + x * macro_size);
+
+	switch (buf.format()) {
+	case PixelFormat::Y210: {
+		// XXX naive shift left, similar to 10-bit funcs in class RGB
+		uint16_t y0 = yuv1.y << 2;
+		uint16_t y1 = yuv2.y << 2;
+		uint16_t cb = ((yuv1.u  << 2) + (yuv2.u << 2)) / 2;
+		uint16_t cr = ((yuv1.v  << 2) + (yuv2.v << 2)) / 2;
+
+		write16le(y0, &p[0]);
+		write16le(cb, &p[1]);
+		write16le(y1, &p[2]);
+		write16le(cr, &p[3]);
+		break;
+	}
+
+	default:
+		throw std::invalid_argument("invalid pixelformat");
+	}
+}
+
 static void draw_yuv422_semiplanar_macropixel(IFramebuffer& buf, unsigned x, unsigned y,
 					      YUV yuv1, YUV yuv2)
 {
@@ -257,6 +284,10 @@ void draw_yuv422_macropixel(IFramebuffer& buf, unsigned x, unsigned y, YUV yuv1,
 		draw_yuv422_packed_macropixel(buf, x, y, yuv1, yuv2);
 		break;
 
+	case PixelFormat::Y210:
+		draw_y2xx_packed_macropixel(buf, x, y, yuv1, yuv2);
+		break;
+
 	case PixelFormat::NV16:
 	case PixelFormat::NV61:
 		draw_yuv422_semiplanar_macropixel(buf, x, y, yuv1, yuv2);
-- 
2.34.1

Re: [PATCH kms++ 0/4] Support Y210

[Tomi Valkeinen]

On 02/12/2022 15:16, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> Hi,
> 
> These kms++ patches add support for Y210 format.
> 
> I didn't find a super clear description of the byte order for Y210
> anywhere.  If someone knows what it is supposed to be, I'd appreciate
> verifying the code =).

Actually, this:

https://learn.microsoft.com/en-us/windows/win32/medfound/10-bit-and-16-bit-yuv-video-formats#overview

indicates that the 10-bit values should be shifted by 6. So the drawing 
code in this series is probably not right.

  Tomi

Re: [PATCH kms++ 0/4] Support Y210

[Geert Uytterhoeven]

Hi Tomi,

On Fri, Dec 2, 2022 at 4:32 PM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
> On 02/12/2022 15:16, Tomi Valkeinen wrote:
> > These kms++ patches add support for Y210 format.
> >
> > I didn't find a super clear description of the byte order for Y210
> > anywhere.  If someone knows what it is supposed to be, I'd appreciate
> > verifying the code =).
>
> Actually, this:
>
> https://learn.microsoft.com/en-us/windows/win32/medfound/10-bit-and-16-bit-yuv-video-formats#overview
>
> indicates that the 10-bit values should be shifted by 6. So the drawing
> code in this series is probably not right.

https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_fourcc.h#L212
seems to confirm that?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH kms++ 0/4] Support Y210

[Tomi Valkeinen]

On 02/12/2022 17:56, Geert Uytterhoeven wrote:
> Hi Tomi,
> 
> On Fri, Dec 2, 2022 at 4:32 PM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>> On 02/12/2022 15:16, Tomi Valkeinen wrote:
>>> These kms++ patches add support for Y210 format.
>>>
>>> I didn't find a super clear description of the byte order for Y210
>>> anywhere.  If someone knows what it is supposed to be, I'd appreciate
>>> verifying the code =).
>>
>> Actually, this:
>>
>> https://learn.microsoft.com/en-us/windows/win32/medfound/10-bit-and-16-bit-yuv-video-formats#overview
>>
>> indicates that the 10-bit values should be shifted by 6. So the drawing
>> code in this series is probably not right.
> 
> https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_fourcc.h#L212
> seems to confirm that?

Hmm, indeed. I've looked at that before but I couldn't decipher it, but 
now the text makes sense. Although afaics, it doesn't say that each 
component is a 16-bit little endian. And I don't get the "little endian 
per 2 Y pixels".

So if I get this right...

The first byte in memory is the two lowest bits of Y0 in bits 6 and 7. 
The rest of the first byte is zero padding.

The second byte is the 8 highest bits of Y0.

Then following the same method, Cb, Y1, Cr.

  Tomi

Re: [PATCH kms++ 0/4] Support Y210

[Laurent Pinchart]

On Fri, Dec 02, 2022 at 06:06:01PM +0200, Tomi Valkeinen wrote:
> On 02/12/2022 17:56, Geert Uytterhoeven wrote:
> > On Fri, Dec 2, 2022 at 4:32 PM Tomi Valkeinen wrote:
> >> On 02/12/2022 15:16, Tomi Valkeinen wrote:
> >>> These kms++ patches add support for Y210 format.
> >>>
> >>> I didn't find a super clear description of the byte order for Y210
> >>> anywhere.  If someone knows what it is supposed to be, I'd appreciate
> >>> verifying the code =).
> >>
> >> Actually, this:
> >>
> >> https://learn.microsoft.com/en-us/windows/win32/medfound/10-bit-and-16-bit-yuv-video-formats#overview
> >>
> >> indicates that the 10-bit values should be shifted by 6. So the drawing
> >> code in this series is probably not right.
> > 
> > https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_fourcc.h#L212
> > seems to confirm that?
> 
> Hmm, indeed. I've looked at that before but I couldn't decipher it, but 
> now the text makes sense. Although afaics, it doesn't say that each 
> component is a 16-bit little endian. And I don't get the "little endian 
> per 2 Y pixels".
> 
> So if I get this right...
> 
> The first byte in memory is the two lowest bits of Y0 in bits 6 and 7. 
> The rest of the first byte is zero padding.
> 
> The second byte is the 8 highest bits of Y0.
> 
> Then following the same method, Cb, Y1, Cr.

I would have said in bits 7 and 6 for the first byte, but I assume we
mean the same. Apart from that, your interpretation is the same as mind.
However, if your aim is to support the 10bpp YUV 4:2:2 format of the
VSP, I'm not sure this is correct. Assuming you plan to set F2B=1 to
align bits towards the MSB, it's not clear if the endianness can be
configured. The four permutations of the packed YUYV components are
achievable as far as I can tell (see figure 60.12 in the documentation),
but I didn't immediately find a place where the endianness of each
16-bit value could be configured. Figure 60.15 mentions "example in case
of big endian", I don't know if it's an example, or the only possible
option. You may want to try both.

-- 
Regards,

Laurent Pinchart

Re: [PATCH kms++ 1/4] kms++: PixelFormats: Fix formatting

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Dec 02, 2022 at 03:16:55PM +0200, Tomi Valkeinen wrote:
> Fix formatting for some pixel formats.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  kms++/src/pixelformats.cpp | 60 +++++++-------------------------------
>  1 file changed, 10 insertions(+), 50 deletions(-)
> 
> diff --git a/kms++/src/pixelformats.cpp b/kms++/src/pixelformats.cpp
> index d739efd..7afbf09 100644
> --- a/kms++/src/pixelformats.cpp
> +++ b/kms++/src/pixelformats.cpp
> @@ -32,107 +32,67 @@ static const map<PixelFormat, PixelFormatInfo> format_info_array = {
>  	{ PixelFormat::NV12, {
>  				     PixelColorType::YUV,
>  				     2,
> -				     { {
> -					       8,
> -					       1,
> -					       1,
> -				       },
> +				     { { 8, 1, 1 },
>  				       { 8, 2, 2 } },
>  			     } },

While at it I would probably have reformated everything as

	{ PixelFormat::NV12, {
		PixelColorType::YUV,
		2,
		{
			{ 8, 1, 1 },
			{ 8, 2, 2 }
		},
	} },

or

	{ PixelFormat::NV12, {
		PixelColorType::YUV,
		2,
		{ { 8, 1, 1 }, { 8, 2, 2 } },
	} },

to make the table more compact (and I believe more readable), but that's
entirely up to you.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	{ PixelFormat::NV21, {
>  				     PixelColorType::YUV,
>  				     2,
> -				     { {
> -					       8,
> -					       1,
> -					       1,
> -				       },
> +				     { { 8, 1, 1 },
>  				       { 8, 2, 2 } },
>  			     } },
>  	{ PixelFormat::NV16, {
>  				     PixelColorType::YUV,
>  				     2,
> -				     { {
> -					       8,
> -					       1,
> -					       1,
> -				       },
> +				     { { 8, 1, 1 },
>  				       { 8, 2, 1 } },
>  			     } },
>  	{ PixelFormat::NV61, {
>  				     PixelColorType::YUV,
>  				     2,
> -				     { {
> -					       8,
> -					       1,
> -					       1,
> -				       },
> +				     { { 8, 1, 1 },
>  				       { 8, 2, 1 } },
>  			     } },
>  	/* YUV planar */
>  	{ PixelFormat::YUV420, {
>  				       PixelColorType::YUV,
>  				       3,
> -				       { {
> -						 8,
> -						 1,
> -						 1,
> -					 },
> +				       { { 8, 1, 1 },
>  					 { 8, 2, 2 },
>  					 { 8, 2, 2 } },
>  			       } },
>  	{ PixelFormat::YVU420, {
>  				       PixelColorType::YUV,
>  				       3,
> -				       { {
> -						 8,
> -						 1,
> -						 1,
> -					 },
> +				       { { 8, 1, 1 },
>  					 { 8, 2, 2 },
>  					 { 8, 2, 2 } },
>  			       } },
>  	{ PixelFormat::YUV422, {
>  				       PixelColorType::YUV,
>  				       3,
> -				       { {
> -						 8,
> -						 1,
> -						 1,
> -					 },
> +				       { { 8, 1, 1 },
>  					 { 8, 2, 1 },
>  					 { 8, 2, 1 } },
>  			       } },
>  	{ PixelFormat::YVU422, {
>  				       PixelColorType::YUV,
>  				       3,
> -				       { {
> -						 8,
> -						 1,
> -						 1,
> -					 },
> +				       { { 8, 1, 1 },
>  					 { 8, 2, 1 },
>  					 { 8, 2, 1 } },
>  			       } },
>  	{ PixelFormat::YUV444, {
>  				       PixelColorType::YUV,
>  				       3,
> -				       { {
> -						 8,
> -						 1,
> -						 1,
> -					 },
> +				       { { 8, 1, 1 },
>  					 { 8, 1, 1 },
>  					 { 8, 1, 1 } },
>  			       } },
>  	{ PixelFormat::YVU444, {
>  				       PixelColorType::YUV,
>  				       3,
> -				       { {
> -						 8,
> -						 1,
> -						 1,
> -					 },
> +				       { { 8, 1, 1 },
>  					 { 8, 1, 1 },
>  					 { 8, 1, 1 } },
>  			       } },

-- 
Regards,

Laurent Pinchart

Re: [PATCH kms++ 3/4] kms++util: Add endian.h

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Dec 02, 2022 at 03:16:57PM +0200, Tomi Valkeinen wrote:
> Add simple endianness supporting write function, and, for now, only one
> shortcut helper, write16le().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  kms++util/inc/kms++util/endian.h | 46 ++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 kms++util/inc/kms++util/endian.h
> 
> diff --git a/kms++util/inc/kms++util/endian.h b/kms++util/inc/kms++util/endian.h
> new file mode 100644
> index 0000000..ea09065
> --- /dev/null
> +++ b/kms++util/inc/kms++util/endian.h
> @@ -0,0 +1,46 @@
> +#pragma once
> +
> +#include <type_traits>
> +#include <byteswap.h>
> +#include <stdint.h>
> +
> +static_assert((__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) ||
> +	      (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__),
> +	      "Unable to detect endianness");
> +
> +enum class endian {
> +	little = __ORDER_LITTLE_ENDIAN__,
> +	big = __ORDER_BIG_ENDIAN__,
> +	native = __BYTE_ORDER__
> +};
> +
> +template<typename T>
> +constexpr T byteswap(T value) noexcept
> +{
> +	static_assert(std::is_integral<T>(), "Type is not integral");
> +	static_assert(sizeof(T) == 2 ||
> +		      sizeof(T) == 4 ||
> +		      sizeof(T) == 8,
> +		      "Illegal value size");
> +
> +	switch (sizeof(T)) {
> +		case 2: return bswap_16(value);
> +		case 4: return bswap_32(value);
> +		case 8: return bswap_64(value);
> +	}
> +}
> +
> +template<endian E, typename T>
> +static void write_endian(T val, T* dst)

I would have swapped the parameters, common APIs have the destination
first and the source second. Same below, and up to you.

> +{
> +	if constexpr (E != endian::native)
> +		val = byteswap(val);
> +
> +	*dst = val;
> +}
> +
> +[[maybe_unused]]
> +static void write16le(uint16_t val, uint16_t* dst)

I wonder if writing

using write16le = write_endian<endian::little, uint16_t>;

would compile.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +{
> +	write_endian<endian::little, uint16_t>(val, dst);
> +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH kms++ 2/4] kms++: PixelFormats: Add Y210 format

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Dec 02, 2022 at 03:16:56PM +0200, Tomi Valkeinen wrote:
> Add Y210 pixel format.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  kms++/inc/kms++/pixelformats.h | 2 ++
>  kms++/src/pixelformats.cpp     | 6 ++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/kms++/inc/kms++/pixelformats.h b/kms++/inc/kms++/pixelformats.h
> index 6f2671b..35261a9 100644
> --- a/kms++/inc/kms++/pixelformats.h
> +++ b/kms++/inc/kms++/pixelformats.h
> @@ -31,6 +31,8 @@ enum class PixelFormat : uint32_t {
>  	YVYU = MakeFourCC("YVYU"),
>  	VYUY = MakeFourCC("VYUY"),
>  
> +	Y210 = MakeFourCC("Y210"),
> +
>  	XRGB8888 = MakeFourCC("XR24"),
>  	XBGR8888 = MakeFourCC("XB24"),
>  	RGBX8888 = MakeFourCC("RX24"),
> diff --git a/kms++/src/pixelformats.cpp b/kms++/src/pixelformats.cpp
> index 7afbf09..89eece4 100644
> --- a/kms++/src/pixelformats.cpp
> +++ b/kms++/src/pixelformats.cpp
> @@ -28,6 +28,12 @@ static const map<PixelFormat, PixelFormatInfo> format_info_array = {
>  				     1,
>  				     { { 16, 2, 1 } },
>  			     } },
> +	{ PixelFormat::Y210, {
> +				     PixelColorType::YUV,
> +				     1,
> +				     { { 32, 2, 1 } },
> +			     } },
> +
>  	/* YUV semi-planar */
>  	{ PixelFormat::NV12, {
>  				     PixelColorType::YUV,

-- 
Regards,

Laurent Pinchart

Re: [PATCH kms++ 4/4] kms++util: Add Y210 drawing support

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Dec 02, 2022 at 03:16:58PM +0200, Tomi Valkeinen wrote:
> Add support for drawing Y210 pixels.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  kms++util/src/drawing.cpp | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp
> index 79e0d90..7e1b40b 100644
> --- a/kms++util/src/drawing.cpp
> +++ b/kms++util/src/drawing.cpp
> @@ -3,6 +3,7 @@
>  
>  #include <kms++/kms++.h>
>  #include <kms++util/kms++util.h>
> +#include <kms++util/endian.h>
>  
>  using namespace std;
>  
> @@ -179,6 +180,32 @@ static void draw_yuv422_packed_macropixel(IFramebuffer& buf, unsigned x, unsigne
>  	}
>  }
>  
> +static void draw_y2xx_packed_macropixel(IFramebuffer& buf, unsigned x, unsigned y,
> +					  YUV yuv1, YUV yuv2)
> +{
> +	const uint32_t macro_size = 4;
> +	uint16_t* p = (uint16_t*)(buf.map(0) + buf.stride(0) * y + x * macro_size);
> +
> +	switch (buf.format()) {
> +	case PixelFormat::Y210: {
> +		// XXX naive shift left, similar to 10-bit funcs in class RGB

As mentioned in replies to the cover letter, values should be shifted by
6 bits.

> +		uint16_t y0 = yuv1.y << 2;
> +		uint16_t y1 = yuv2.y << 2;
> +		uint16_t cb = ((yuv1.u  << 2) + (yuv2.u << 2)) / 2;
> +		uint16_t cr = ((yuv1.v  << 2) + (yuv2.v << 2)) / 2;
> +
> +		write16le(y0, &p[0]);
> +		write16le(cb, &p[1]);
> +		write16le(y1, &p[2]);
> +		write16le(cr, &p[3]);

If x is odd, won't this swap cb and cr ? draw_yuv422_packed_macropixel()
seems to have the same possible issue, so I assume callers always pass
an even x value. If so,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

And if not, draw_yuv422_packed_macropixel() will need to be fixed too,
so I'm fine with this patch and an additional fix to both functions on
top.

> +		break;
> +	}
> +
> +	default:
> +		throw std::invalid_argument("invalid pixelformat");
> +	}
> +}
> +
>  static void draw_yuv422_semiplanar_macropixel(IFramebuffer& buf, unsigned x, unsigned y,
>  					      YUV yuv1, YUV yuv2)
>  {
> @@ -257,6 +284,10 @@ void draw_yuv422_macropixel(IFramebuffer& buf, unsigned x, unsigned y, YUV yuv1,
>  		draw_yuv422_packed_macropixel(buf, x, y, yuv1, yuv2);
>  		break;
>  
> +	case PixelFormat::Y210:
> +		draw_y2xx_packed_macropixel(buf, x, y, yuv1, yuv2);
> +		break;
> +
>  	case PixelFormat::NV16:
>  	case PixelFormat::NV61:
>  		draw_yuv422_semiplanar_macropixel(buf, x, y, yuv1, yuv2);

-- 
Regards,

Laurent Pinchart

Re: [PATCH kms++ 3/4] kms++util: Add endian.h

[Tomi Valkeinen]

On 03/12/2022 01:56, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, Dec 02, 2022 at 03:16:57PM +0200, Tomi Valkeinen wrote:
>> Add simple endianness supporting write function, and, for now, only one
>> shortcut helper, write16le().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   kms++util/inc/kms++util/endian.h | 46 ++++++++++++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>   create mode 100644 kms++util/inc/kms++util/endian.h
>>
>> diff --git a/kms++util/inc/kms++util/endian.h b/kms++util/inc/kms++util/endian.h
>> new file mode 100644
>> index 0000000..ea09065
>> --- /dev/null
>> +++ b/kms++util/inc/kms++util/endian.h
>> @@ -0,0 +1,46 @@
>> +#pragma once
>> +
>> +#include <type_traits>
>> +#include <byteswap.h>
>> +#include <stdint.h>
>> +
>> +static_assert((__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) ||
>> +	      (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__),
>> +	      "Unable to detect endianness");
>> +
>> +enum class endian {
>> +	little = __ORDER_LITTLE_ENDIAN__,
>> +	big = __ORDER_BIG_ENDIAN__,
>> +	native = __BYTE_ORDER__
>> +};
>> +
>> +template<typename T>
>> +constexpr T byteswap(T value) noexcept
>> +{
>> +	static_assert(std::is_integral<T>(), "Type is not integral");
>> +	static_assert(sizeof(T) == 2 ||
>> +		      sizeof(T) == 4 ||
>> +		      sizeof(T) == 8,
>> +		      "Illegal value size");
>> +
>> +	switch (sizeof(T)) {
>> +		case 2: return bswap_16(value);
>> +		case 4: return bswap_32(value);
>> +		case 8: return bswap_64(value);
>> +	}
>> +}
>> +
>> +template<endian E, typename T>
>> +static void write_endian(T val, T* dst)
> 
> I would have swapped the parameters, common APIs have the destination
> first and the source second. Same below, and up to you.

True, I think that makes sense.

>> +{
>> +	if constexpr (E != endian::native)
>> +		val = byteswap(val);
>> +
>> +	*dst = val;
>> +}
>> +
>> +[[maybe_unused]]
>> +static void write16le(uint16_t val, uint16_t* dst)
> 
> I wonder if writing
> 
> using write16le = write_endian<endian::little, uint16_t>;
> 
> would compile.

No, using needs a type.

  Tomi

Re: [PATCH kms++ 4/4] kms++util: Add Y210 drawing support

[Tomi Valkeinen]

On 03/12/2022 02:01, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, Dec 02, 2022 at 03:16:58PM +0200, Tomi Valkeinen wrote:
>> Add support for drawing Y210 pixels.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   kms++util/src/drawing.cpp | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp
>> index 79e0d90..7e1b40b 100644
>> --- a/kms++util/src/drawing.cpp
>> +++ b/kms++util/src/drawing.cpp
>> @@ -3,6 +3,7 @@
>>   
>>   #include <kms++/kms++.h>
>>   #include <kms++util/kms++util.h>
>> +#include <kms++util/endian.h>
>>   
>>   using namespace std;
>>   
>> @@ -179,6 +180,32 @@ static void draw_yuv422_packed_macropixel(IFramebuffer& buf, unsigned x, unsigne
>>   	}
>>   }
>>   
>> +static void draw_y2xx_packed_macropixel(IFramebuffer& buf, unsigned x, unsigned y,
>> +					  YUV yuv1, YUV yuv2)
>> +{
>> +	const uint32_t macro_size = 4;
>> +	uint16_t* p = (uint16_t*)(buf.map(0) + buf.stride(0) * y + x * macro_size);
>> +
>> +	switch (buf.format()) {
>> +	case PixelFormat::Y210: {
>> +		// XXX naive shift left, similar to 10-bit funcs in class RGB
> 
> As mentioned in replies to the cover letter, values should be shifted by
> 6 bits.
> 
>> +		uint16_t y0 = yuv1.y << 2;
>> +		uint16_t y1 = yuv2.y << 2;
>> +		uint16_t cb = ((yuv1.u  << 2) + (yuv2.u << 2)) / 2;
>> +		uint16_t cr = ((yuv1.v  << 2) + (yuv2.v << 2)) / 2;
>> +
>> +		write16le(y0, &p[0]);
>> +		write16le(cb, &p[1]);
>> +		write16le(y1, &p[2]);
>> +		write16le(cr, &p[3]);
> 
> If x is odd, won't this swap cb and cr ? draw_yuv422_packed_macropixel()
> seems to have the same possible issue, so I assume callers always pass
> an even x value. If so,

Yes, a macro pixel always needs to be correctly aligned. With YUYV style 
macropixels it's even aligned. The test pattern drawing handles this 
correctly, but I'm not sure if all the funcs available to the users 
check it correctly.

  Tomi

Re: [PATCH kms++ 0/4] Support Y210

[Tomi Valkeinen]

On 03/12/2022 01:47, Laurent Pinchart wrote:
> On Fri, Dec 02, 2022 at 06:06:01PM +0200, Tomi Valkeinen wrote:
>> On 02/12/2022 17:56, Geert Uytterhoeven wrote:
>>> On Fri, Dec 2, 2022 at 4:32 PM Tomi Valkeinen wrote:
>>>> On 02/12/2022 15:16, Tomi Valkeinen wrote:
>>>>> These kms++ patches add support for Y210 format.
>>>>>
>>>>> I didn't find a super clear description of the byte order for Y210
>>>>> anywhere.  If someone knows what it is supposed to be, I'd appreciate
>>>>> verifying the code =).
>>>>
>>>> Actually, this:
>>>>
>>>> https://learn.microsoft.com/en-us/windows/win32/medfound/10-bit-and-16-bit-yuv-video-formats#overview
>>>>
>>>> indicates that the 10-bit values should be shifted by 6. So the drawing
>>>> code in this series is probably not right.
>>>
>>> https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm_fourcc.h#L212
>>> seems to confirm that?
>>
>> Hmm, indeed. I've looked at that before but I couldn't decipher it, but
>> now the text makes sense. Although afaics, it doesn't say that each
>> component is a 16-bit little endian. And I don't get the "little endian
>> per 2 Y pixels".
>>
>> So if I get this right...
>>
>> The first byte in memory is the two lowest bits of Y0 in bits 6 and 7.
>> The rest of the first byte is zero padding.
>>
>> The second byte is the 8 highest bits of Y0.
>>
>> Then following the same method, Cb, Y1, Cr.
> 
> I would have said in bits 7 and 6 for the first byte, but I assume we
> mean the same. Apart from that, your interpretation is the same as mind.
> However, if your aim is to support the 10bpp YUV 4:2:2 format of the
> VSP, I'm not sure this is correct. Assuming you plan to set F2B=1 to
> align bits towards the MSB, it's not clear if the endianness can be
> configured. The four permutations of the packed YUYV components are
> achievable as far as I can tell (see figure 60.12 in the documentation),
> but I didn't immediately find a place where the endianness of each
> 16-bit value could be configured. Figure 60.15 mentions "example in case
> of big endian", I don't know if it's an example, or the only possible
> option. You may want to try both.

The pixel format can be set using F2B and swap options in VI6_RPFn_DSWAP 
(Table 60.14). I have Y210 working (and I have had multiple other 
variations working too). And by "working" I mean that it looks good to 
me, but I can only see it via a web meeting as the board is at Kieran's.

The HW seems to be very versatile, and a lot of different variations can 
be supported, especially for RGB formats. So this has been a bit 
difficult as I didn't have any ready and validated test tool to draw 
Y210, but I hope I got it working now correctly in kms++.

I don't know what format for 10-bit YUV Renesas really needs, but the 
BSP commits said Y210, so that's what I added. Although afaik, the DU 
only handles 8-bits per component, so I'm not sure what's the benefit of 
10-bit formats (especially for RGB).

  Tomi