[PATCH v2 00/10] drm: Add support for low-color frame buffer formats

[PATCH v2 00/10] drm: Add support for low-color frame buffer formats

[Geert Uytterhoeven]

	Hi all,

A long outstanding issue with the DRM subsystem has been the lack of
support for low-color displays, as used typically on older desktop
systems, and on small embedded displays.

This patch series adds support for color-indexed frame buffer formats
with 2, 4, and 16 colors.  It has been tested on ARAnyM using a
work-in-progress Atari DRM driver supporting 2, 4, 16, and 256 colors,
with text console operation, fbtest, and modetest.

Overview:
  - Patch 1 introduces a helper, to be used by later patches in the
    series,
  - Patch 2 introduces a flag to indicate color-indexed formats,
  - Patches 3 and 4 correct calculations of bits per pixel for sub-byte
    pixel formats,
  - Patches 5 and 6 introduce the new C[124] formats,
  - Patch 7 fixes an untested code path,
  - Patch 8 documents the use of "red" for light-on-dark displays,
  - Patches 9 and 10 add more fourcc codes for light-on-dark and
    dark-on-light frame buffer formats, which may be useful for e.g. the
    ssd130x and repaper drivers.

Changes compared to v1[1]:
  - Reshuffle patches,
  - New patch "[PATCH v2 02/10] drm/fourcc: Add
    drm_format_info.is_color_indexed flag",
  - Improve pixel descriptions,
  - Require depth to match bpp in drm_mode_legacy_fb_format(),
  - Set .is_color_indexed flag.
  - Use drm_format_info_bpp() helper instead of deprecated .depth field
    or format-dependent calculations,
  - Use new .is_color_indexed field instead of checking against a list
    of formats,
  - Add Acked-by,
  - Replace FIXME by TODO comment,
  - New patch "[PATCH v2 08/10] [RFC] drm/fourcc: Document that
    single-channel "red" can be any color",
  - Add rationale for adding new formats,
  - Add D[248] for completeness.

Notes:
  - Using modetest with the C4 formats requires [2].
  - Using modetest with the default XR24 format requires [3].
  - As this was used on emulated hardware only, and I do not have Atari
    hardware, I do not have performance figures to compare with fbdev.
    I hope to do proper measuring with an Amiga DRM driver, eventually.

Thanks for your comments!

[1] "[PATCH 0/8] drm: Add support for low-color frame buffer formats"
    https://lore.kernel.org/r/20220215165226.2738568-1-geert@linux-m68k.org/
[2] "[PATCH libdrm 0/3] Add support for low-color frame buffer formats"
    https://lore.kernel.org/r/cover.1646683737.git.geert@linux-m68k.org
[3] "[PATCH RFC libdrm 0/2] Big-endian fixes"
    https://lore.kernel.org/r/cover.1646684158.git.geert@linux-m68k.org

Geert Uytterhoeven (10):
  drm/fourcc: Add drm_format_info_bpp() helper
  drm/fourcc: Add drm_format_info.is_color_indexed flag
  drm/client: Use actual bpp when allocating frame buffers
  drm/framebuffer: Use actual bpp for DRM_IOCTL_MODE_GETFB
  drm/fourcc: Add DRM_FORMAT_C[124]
  drm/fb-helper: Add support for DRM_FORMAT_C[124]
  [RFC] drm/gem-fb-helper: Use actual bpp for size calculations
  [RFC] drm/fourcc: Document that single-channel "red" can be any color
  [RFC] drm/fourcc: Add DRM_FORMAT_R[124]
  [RFC] drm/fourcc: Add DRM_FORMAT_D[1248]

 drivers/gpu/drm/drm_client.c                 |   4 +-
 drivers/gpu/drm/drm_fb_helper.c              | 101 ++++++++++++++-----
 drivers/gpu/drm/drm_fourcc.c                 |  55 +++++++++-
 drivers/gpu/drm/drm_framebuffer.c            |   2 +-
 drivers/gpu/drm/drm_gem_framebuffer_helper.c |  12 +--
 include/drm/drm_fourcc.h                     |   4 +
 include/uapi/drm/drm_fourcc.h                |  36 +++++--
 7 files changed, 169 insertions(+), 45 deletions(-)

-- 
2.25.1

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

[PATCH v2 00/10] drm: Add support for low-color frame buffer formats

[Geert Uytterhoeven]

	Hi all,

A long outstanding issue with the DRM subsystem has been the lack of
support for low-color displays, as used typically on older desktop
systems, and on small embedded displays.

This patch series adds support for color-indexed frame buffer formats
with 2, 4, and 16 colors.  It has been tested on ARAnyM using a
work-in-progress Atari DRM driver supporting 2, 4, 16, and 256 colors,
with text console operation, fbtest, and modetest.

Overview:
  - Patch 1 introduces a helper, to be used by later patches in the
    series,
  - Patch 2 introduces a flag to indicate color-indexed formats,
  - Patches 3 and 4 correct calculations of bits per pixel for sub-byte
    pixel formats,
  - Patches 5 and 6 introduce the new C[124] formats,
  - Patch 7 fixes an untested code path,
  - Patch 8 documents the use of "red" for light-on-dark displays,
  - Patches 9 and 10 add more fourcc codes for light-on-dark and
    dark-on-light frame buffer formats, which may be useful for e.g. the
    ssd130x and repaper drivers.

Changes compared to v1[1]:
  - Reshuffle patches,
  - New patch "[PATCH v2 02/10] drm/fourcc: Add
    drm_format_info.is_color_indexed flag",
  - Improve pixel descriptions,
  - Require depth to match bpp in drm_mode_legacy_fb_format(),
  - Set .is_color_indexed flag.
  - Use drm_format_info_bpp() helper instead of deprecated .depth field
    or format-dependent calculations,
  - Use new .is_color_indexed field instead of checking against a list
    of formats,
  - Add Acked-by,
  - Replace FIXME by TODO comment,
  - New patch "[PATCH v2 08/10] [RFC] drm/fourcc: Document that
    single-channel "red" can be any color",
  - Add rationale for adding new formats,
  - Add D[248] for completeness.

Notes:
  - Using modetest with the C4 formats requires [2].
  - Using modetest with the default XR24 format requires [3].
  - As this was used on emulated hardware only, and I do not have Atari
    hardware, I do not have performance figures to compare with fbdev.
    I hope to do proper measuring with an Amiga DRM driver, eventually.

Thanks for your comments!

[1] "[PATCH 0/8] drm: Add support for low-color frame buffer formats"
    https://lore.kernel.org/r/20220215165226.2738568-1-geert@linux-m68k.org/
[2] "[PATCH libdrm 0/3] Add support for low-color frame buffer formats"
    https://lore.kernel.org/r/cover.1646683737.git.geert@linux-m68k.org
[3] "[PATCH RFC libdrm 0/2] Big-endian fixes"
    https://lore.kernel.org/r/cover.1646684158.git.geert@linux-m68k.org

Geert Uytterhoeven (10):
  drm/fourcc: Add drm_format_info_bpp() helper
  drm/fourcc: Add drm_format_info.is_color_indexed flag
  drm/client: Use actual bpp when allocating frame buffers
  drm/framebuffer: Use actual bpp for DRM_IOCTL_MODE_GETFB
  drm/fourcc: Add DRM_FORMAT_C[124]
  drm/fb-helper: Add support for DRM_FORMAT_C[124]
  [RFC] drm/gem-fb-helper: Use actual bpp for size calculations
  [RFC] drm/fourcc: Document that single-channel "red" can be any color
  [RFC] drm/fourcc: Add DRM_FORMAT_R[124]
  [RFC] drm/fourcc: Add DRM_FORMAT_D[1248]

 drivers/gpu/drm/drm_client.c                 |   4 +-
 drivers/gpu/drm/drm_fb_helper.c              | 101 ++++++++++++++-----
 drivers/gpu/drm/drm_fourcc.c                 |  55 +++++++++-
 drivers/gpu/drm/drm_framebuffer.c            |   2 +-
 drivers/gpu/drm/drm_gem_framebuffer_helper.c |  12 +--
 include/drm/drm_fourcc.h                     |   4 +
 include/uapi/drm/drm_fourcc.h                |  36 +++++--
 7 files changed, 169 insertions(+), 45 deletions(-)

-- 
2.25.1

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

[PATCH v2 01/10] drm/fourcc: Add drm_format_info_bpp() helper

[Geert Uytterhoeven]

Add a helper to retrieve the actual number of bits per pixel for a
plane, taking into account the number of characters and pixels per
block for tiled formats.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - Move up.
---
 drivers/gpu/drm/drm_fourcc.c | 19 +++++++++++++++++++
 include/drm/drm_fourcc.h     |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 07741b678798b0f1..cf48ea0b2cb70ba8 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info,
 }
 EXPORT_SYMBOL(drm_format_info_block_height);
 
+/**
+ * drm_format_info_bpp - number of bits per pixel
+ * @info: pixel format info
+ * @plane: plane index
+ *
+ * Returns:
+ * The actual number of bits per pixel, depending on the plane index.
+ */
+unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane)
+{
+	if (!info || plane < 0 || plane >= info->num_planes)
+		return 0;
+
+	return info->char_per_block[plane] * 8 /
+	       (drm_format_info_block_width(info, plane) *
+		drm_format_info_block_height(info, plane));
+}
+EXPORT_SYMBOL(drm_format_info_bpp);
+
 /**
  * drm_format_info_min_pitch - computes the minimum required pitch in bytes
  * @info: pixel format info
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 22aa64d07c7905e2..3800a7ad7f0cda7a 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -313,6 +313,7 @@ unsigned int drm_format_info_block_width(const struct drm_format_info *info,
 					 int plane);
 unsigned int drm_format_info_block_height(const struct drm_format_info *info,
 					  int plane);
+unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane);
 uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
 				   int plane, unsigned int buffer_width);
 
-- 
2.25.1

[PATCH v2 01/10] drm/fourcc: Add drm_format_info_bpp() helper

[Geert Uytterhoeven]

Add a helper to retrieve the actual number of bits per pixel for a
plane, taking into account the number of characters and pixels per
block for tiled formats.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - Move up.
---
 drivers/gpu/drm/drm_fourcc.c | 19 +++++++++++++++++++
 include/drm/drm_fourcc.h     |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 07741b678798b0f1..cf48ea0b2cb70ba8 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info,
 }
 EXPORT_SYMBOL(drm_format_info_block_height);
 
+/**
+ * drm_format_info_bpp - number of bits per pixel
+ * @info: pixel format info
+ * @plane: plane index
+ *
+ * Returns:
+ * The actual number of bits per pixel, depending on the plane index.
+ */
+unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane)
+{
+	if (!info || plane < 0 || plane >= info->num_planes)
+		return 0;
+
+	return info->char_per_block[plane] * 8 /
+	       (drm_format_info_block_width(info, plane) *
+		drm_format_info_block_height(info, plane));
+}
+EXPORT_SYMBOL(drm_format_info_bpp);
+
 /**
  * drm_format_info_min_pitch - computes the minimum required pitch in bytes
  * @info: pixel format info
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 22aa64d07c7905e2..3800a7ad7f0cda7a 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -313,6 +313,7 @@ unsigned int drm_format_info_block_width(const struct drm_format_info *info,
 					 int plane);
 unsigned int drm_format_info_block_height(const struct drm_format_info *info,
 					  int plane);
+unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane);
 uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
 				   int plane, unsigned int buffer_width);
 
-- 
2.25.1

Re: [PATCH v2 01/10] drm/fourcc: Add drm_format_info_bpp() helper

[Javier Martinez Canillas]

Hello Geert,

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Add a helper to retrieve the actual number of bits per pixel for a
> plane, taking into account the number of characters and pixels per
> block for tiled formats.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 01/10] drm/fourcc: Add drm_format_info_bpp() helper

[Javier Martinez Canillas]

Hello Geert,

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Add a helper to retrieve the actual number of bits per pixel for a
> plane, taking into account the number of characters and pixels per
> block for tiled formats.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 02/10] drm/fourcc: Add drm_format_info.is_color_indexed flag

[Geert Uytterhoeven]

Add a flag to struct drm_format_info to indicate if a format is
color-indexed, similar to the existing .is_yuv flag.

This way generic code and drivers can just check this flag, instead of
checking against a list of fourcc formats.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - New.
---
 drivers/gpu/drm/drm_fourcc.c | 2 +-
 include/drm/drm_fourcc.h     | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index cf48ea0b2cb70ba8..6c76bd821d17e7c7 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -132,7 +132,7 @@ EXPORT_SYMBOL(drm_driver_legacy_fb_format);
 const struct drm_format_info *__drm_format_info(u32 format)
 {
 	static const struct drm_format_info formats[] = {
-		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
 		{ .format = DRM_FORMAT_R8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R10,		.depth = 10, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R12,		.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 3800a7ad7f0cda7a..532ae78ca747e6c4 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -138,6 +138,9 @@ struct drm_format_info {
 
 	/** @is_yuv: Is it a YUV format? */
 	bool is_yuv;
+
+	/** @is_color_indexed: Is it a color-indexed format? */
+	bool is_color_indexed;
 };
 
 /**
-- 
2.25.1

[PATCH v2 02/10] drm/fourcc: Add drm_format_info.is_color_indexed flag

[Geert Uytterhoeven]

Add a flag to struct drm_format_info to indicate if a format is
color-indexed, similar to the existing .is_yuv flag.

This way generic code and drivers can just check this flag, instead of
checking against a list of fourcc formats.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - New.
---
 drivers/gpu/drm/drm_fourcc.c | 2 +-
 include/drm/drm_fourcc.h     | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index cf48ea0b2cb70ba8..6c76bd821d17e7c7 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -132,7 +132,7 @@ EXPORT_SYMBOL(drm_driver_legacy_fb_format);
 const struct drm_format_info *__drm_format_info(u32 format)
 {
 	static const struct drm_format_info formats[] = {
-		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
 		{ .format = DRM_FORMAT_R8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R10,		.depth = 10, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R12,		.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 3800a7ad7f0cda7a..532ae78ca747e6c4 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -138,6 +138,9 @@ struct drm_format_info {
 
 	/** @is_yuv: Is it a YUV format? */
 	bool is_yuv;
+
+	/** @is_color_indexed: Is it a color-indexed format? */
+	bool is_color_indexed;
 };
 
 /**
-- 
2.25.1

Re: [PATCH v2 02/10] drm/fourcc: Add drm_format_info.is_color_indexed flag

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Add a flag to struct drm_format_info to indicate if a format is
> color-indexed, similar to the existing .is_yuv flag.
> 
> This way generic code and drivers can just check this flag, instead of
> checking against a list of fourcc formats.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 02/10] drm/fourcc: Add drm_format_info.is_color_indexed flag

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Add a flag to struct drm_format_info to indicate if a format is
> color-indexed, similar to the existing .is_yuv flag.
> 
> This way generic code and drivers can just check this flag, instead of
> checking against a list of fourcc formats.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 03/10] drm/client: Use actual bpp when allocating frame buffers

[Geert Uytterhoeven]

When allocating a frame buffer, the number of bits per pixel needed is
derived from the deprecated drm_format_info.cpp[] field.  While this
works for formats using less than 8 bits per pixel, it does lead to a
large overallocation.

Reduce memory consumption by using the actual number of bits per pixel
instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
---
v2:
  - Add Acked-by.
---
 drivers/gpu/drm/drm_client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index ce45e380f4a2028f..c6a279e3de95591a 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -264,7 +264,7 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
 
 	dumb_args.width = width;
 	dumb_args.height = height;
-	dumb_args.bpp = info->cpp[0] * 8;
+	dumb_args.bpp = drm_format_info_bpp(info, 0);
 	ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
 	if (ret)
 		goto err_delete;
@@ -372,7 +372,7 @@ static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,
 	int ret;
 
 	info = drm_format_info(format);
-	fb_req.bpp = info->cpp[0] * 8;
+	fb_req.bpp = drm_format_info_bpp(info, 0);
 	fb_req.depth = info->depth;
 	fb_req.width = width;
 	fb_req.height = height;
-- 
2.25.1

[PATCH v2 03/10] drm/client: Use actual bpp when allocating frame buffers

[Geert Uytterhoeven]

When allocating a frame buffer, the number of bits per pixel needed is
derived from the deprecated drm_format_info.cpp[] field.  While this
works for formats using less than 8 bits per pixel, it does lead to a
large overallocation.

Reduce memory consumption by using the actual number of bits per pixel
instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
---
v2:
  - Add Acked-by.
---
 drivers/gpu/drm/drm_client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index ce45e380f4a2028f..c6a279e3de95591a 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -264,7 +264,7 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
 
 	dumb_args.width = width;
 	dumb_args.height = height;
-	dumb_args.bpp = info->cpp[0] * 8;
+	dumb_args.bpp = drm_format_info_bpp(info, 0);
 	ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
 	if (ret)
 		goto err_delete;
@@ -372,7 +372,7 @@ static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,
 	int ret;
 
 	info = drm_format_info(format);
-	fb_req.bpp = info->cpp[0] * 8;
+	fb_req.bpp = drm_format_info_bpp(info, 0);
 	fb_req.depth = info->depth;
 	fb_req.width = width;
 	fb_req.height = height;
-- 
2.25.1

Re: [PATCH v2 03/10] drm/client: Use actual bpp when allocating frame buffers

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> When allocating a frame buffer, the number of bits per pixel needed is
> derived from the deprecated drm_format_info.cpp[] field.  While this
> works for formats using less than 8 bits per pixel, it does lead to a
> large overallocation.
> 
> Reduce memory consumption by using the actual number of bits per pixel
> instead.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 03/10] drm/client: Use actual bpp when allocating frame buffers

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> When allocating a frame buffer, the number of bits per pixel needed is
> derived from the deprecated drm_format_info.cpp[] field.  While this
> works for formats using less than 8 bits per pixel, it does lead to a
> large overallocation.
> 
> Reduce memory consumption by using the actual number of bits per pixel
> instead.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 04/10] drm/framebuffer: Use actual bpp for DRM_IOCTL_MODE_GETFB

[Geert Uytterhoeven]

When userspace queries the properties of a frame buffer, the number of
bits per pixel is derived from the deprecated drm_format_info.cpp[]
field, which does not take into account block sizes.

Fix this by using the actual number of bits per pixel instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - No changes.
---
 drivers/gpu/drm/drm_framebuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 07f5abc875e97b96..4b9d7b01cb99c03d 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -530,7 +530,7 @@ int drm_mode_getfb(struct drm_device *dev,
 	r->height = fb->height;
 	r->width = fb->width;
 	r->depth = fb->format->depth;
-	r->bpp = fb->format->cpp[0] * 8;
+	r->bpp = drm_format_info_bpp(fb->format, 0);
 	r->pitch = fb->pitches[0];
 
 	/* GET_FB() is an unprivileged ioctl so we must not return a
-- 
2.25.1

[PATCH v2 04/10] drm/framebuffer: Use actual bpp for DRM_IOCTL_MODE_GETFB

[Geert Uytterhoeven]

When userspace queries the properties of a frame buffer, the number of
bits per pixel is derived from the deprecated drm_format_info.cpp[]
field, which does not take into account block sizes.

Fix this by using the actual number of bits per pixel instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - No changes.
---
 drivers/gpu/drm/drm_framebuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 07f5abc875e97b96..4b9d7b01cb99c03d 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -530,7 +530,7 @@ int drm_mode_getfb(struct drm_device *dev,
 	r->height = fb->height;
 	r->width = fb->width;
 	r->depth = fb->format->depth;
-	r->bpp = fb->format->cpp[0] * 8;
+	r->bpp = drm_format_info_bpp(fb->format, 0);
 	r->pitch = fb->pitches[0];
 
 	/* GET_FB() is an unprivileged ioctl so we must not return a
-- 
2.25.1

Re: [PATCH v2 04/10] drm/framebuffer: Use actual bpp for DRM_IOCTL_MODE_GETFB

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> When userspace queries the properties of a frame buffer, the number of
> bits per pixel is derived from the deprecated drm_format_info.cpp[]
> field, which does not take into account block sizes.
> 
> Fix this by using the actual number of bits per pixel instead.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 04/10] drm/framebuffer: Use actual bpp for DRM_IOCTL_MODE_GETFB

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> When userspace queries the properties of a frame buffer, the number of
> bits per pixel is derived from the deprecated drm_format_info.cpp[]
> field, which does not take into account block sizes.
> 
> Fix this by using the actual number of bits per pixel instead.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Introduce fourcc codes for color-indexed frame buffer formats with two,
four, and sixteen colors, and provide a mapping from bit per pixel and
depth to fourcc codes.

As the number of bits per pixel is less than eight, these rely on proper
block handling for the calculation of bits per pixel and pitch.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - Improve pixel descriptions,
  - Require depth to match bpp in drm_mode_legacy_fb_format(),
  - Set .is_color_indexed flag.
---
 drivers/gpu/drm/drm_fourcc.c  | 21 +++++++++++++++++++++
 include/uapi/drm/drm_fourcc.h |  5 ++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 6c76bd821d17e7c7..29f4fe199c4ddcf0 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -43,6 +43,21 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
 	uint32_t fmt = DRM_FORMAT_INVALID;
 
 	switch (bpp) {
+	case 1:
+		if (depth == 1)
+			fmt = DRM_FORMAT_C1;
+		break;
+
+	case 2:
+		if (depth == 2)
+			fmt = DRM_FORMAT_C2;
+		break;
+
+	case 4:
+		if (depth == 4)
+			fmt = DRM_FORMAT_C4;
+		break;
+
 	case 8:
 		if (depth == 8)
 			fmt = DRM_FORMAT_C8;
@@ -132,6 +147,12 @@ EXPORT_SYMBOL(drm_driver_legacy_fb_format);
 const struct drm_format_info *__drm_format_info(u32 format)
 {
 	static const struct drm_format_info formats[] = {
+		{ .format = DRM_FORMAT_C1,		.depth = 1,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+		{ .format = DRM_FORMAT_C2,		.depth = 2,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+		{ .format = DRM_FORMAT_C4,		.depth = 4,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
 		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
 		{ .format = DRM_FORMAT_R8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R10,		.depth = 10, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index fc0c1454d2757d5d..457ed39cc48f08e1 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -99,7 +99,10 @@ extern "C" {
 #define DRM_FORMAT_INVALID	0
 
 /* color index */
-#define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
+#define DRM_FORMAT_C1		fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
+#define DRM_FORMAT_C2		fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
+#define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
+#define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
 
 /* 8 bpp Red */
 #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
-- 
2.25.1

[PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Introduce fourcc codes for color-indexed frame buffer formats with two,
four, and sixteen colors, and provide a mapping from bit per pixel and
depth to fourcc codes.

As the number of bits per pixel is less than eight, these rely on proper
block handling for the calculation of bits per pixel and pitch.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - Improve pixel descriptions,
  - Require depth to match bpp in drm_mode_legacy_fb_format(),
  - Set .is_color_indexed flag.
---
 drivers/gpu/drm/drm_fourcc.c  | 21 +++++++++++++++++++++
 include/uapi/drm/drm_fourcc.h |  5 ++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 6c76bd821d17e7c7..29f4fe199c4ddcf0 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -43,6 +43,21 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
 	uint32_t fmt = DRM_FORMAT_INVALID;
 
 	switch (bpp) {
+	case 1:
+		if (depth == 1)
+			fmt = DRM_FORMAT_C1;
+		break;
+
+	case 2:
+		if (depth == 2)
+			fmt = DRM_FORMAT_C2;
+		break;
+
+	case 4:
+		if (depth == 4)
+			fmt = DRM_FORMAT_C4;
+		break;
+
 	case 8:
 		if (depth == 8)
 			fmt = DRM_FORMAT_C8;
@@ -132,6 +147,12 @@ EXPORT_SYMBOL(drm_driver_legacy_fb_format);
 const struct drm_format_info *__drm_format_info(u32 format)
 {
 	static const struct drm_format_info formats[] = {
+		{ .format = DRM_FORMAT_C1,		.depth = 1,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+		{ .format = DRM_FORMAT_C2,		.depth = 2,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+		{ .format = DRM_FORMAT_C4,		.depth = 4,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
 		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
 		{ .format = DRM_FORMAT_R8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R10,		.depth = 10, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index fc0c1454d2757d5d..457ed39cc48f08e1 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -99,7 +99,10 @@ extern "C" {
 #define DRM_FORMAT_INVALID	0
 
 /* color index */
-#define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
+#define DRM_FORMAT_C1		fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
+#define DRM_FORMAT_C2		fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
+#define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
+#define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
 
 /* 8 bpp Red */
 #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
-- 
2.25.1

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3581 bytes --]

On Mon,  7 Mar 2022 21:52:40 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Introduce fourcc codes for color-indexed frame buffer formats with two,
> four, and sixteen colors, and provide a mapping from bit per pixel and
> depth to fourcc codes.
> 
> As the number of bits per pixel is less than eight, these rely on proper
> block handling for the calculation of bits per pixel and pitch.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> v2:
>   - Improve pixel descriptions,
>   - Require depth to match bpp in drm_mode_legacy_fb_format(),
>   - Set .is_color_indexed flag.
> ---
>  drivers/gpu/drm/drm_fourcc.c  | 21 +++++++++++++++++++++
>  include/uapi/drm/drm_fourcc.h |  5 ++++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 6c76bd821d17e7c7..29f4fe199c4ddcf0 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -43,6 +43,21 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
>  	uint32_t fmt = DRM_FORMAT_INVALID;
>  
>  	switch (bpp) {
> +	case 1:
> +		if (depth == 1)
> +			fmt = DRM_FORMAT_C1;
> +		break;
> +
> +	case 2:
> +		if (depth == 2)
> +			fmt = DRM_FORMAT_C2;
> +		break;
> +
> +	case 4:
> +		if (depth == 4)
> +			fmt = DRM_FORMAT_C4;
> +		break;
> +
>  	case 8:
>  		if (depth == 8)
>  			fmt = DRM_FORMAT_C8;
> @@ -132,6 +147,12 @@ EXPORT_SYMBOL(drm_driver_legacy_fb_format);
>  const struct drm_format_info *__drm_format_info(u32 format)
>  {
>  	static const struct drm_format_info formats[] = {
> +		{ .format = DRM_FORMAT_C1,		.depth = 1,  .num_planes = 1,
> +		  .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
> +		{ .format = DRM_FORMAT_C2,		.depth = 2,  .num_planes = 1,
> +		  .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
> +		{ .format = DRM_FORMAT_C4,		.depth = 4,  .num_planes = 1,
> +		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
>  		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
>  		{ .format = DRM_FORMAT_R8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
>  		{ .format = DRM_FORMAT_R10,		.depth = 10, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index fc0c1454d2757d5d..457ed39cc48f08e1 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -99,7 +99,10 @@ extern "C" {
>  #define DRM_FORMAT_INVALID	0
>  
>  /* color index */
> -#define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> +#define DRM_FORMAT_C1		fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> +#define DRM_FORMAT_C2		fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> +#define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> +#define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
>  
>  /* 8 bpp Red */
>  #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */

Hi Geert,

this patch looks good to me, so

Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3581 bytes --]

On Mon,  7 Mar 2022 21:52:40 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Introduce fourcc codes for color-indexed frame buffer formats with two,
> four, and sixteen colors, and provide a mapping from bit per pixel and
> depth to fourcc codes.
> 
> As the number of bits per pixel is less than eight, these rely on proper
> block handling for the calculation of bits per pixel and pitch.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> v2:
>   - Improve pixel descriptions,
>   - Require depth to match bpp in drm_mode_legacy_fb_format(),
>   - Set .is_color_indexed flag.
> ---
>  drivers/gpu/drm/drm_fourcc.c  | 21 +++++++++++++++++++++
>  include/uapi/drm/drm_fourcc.h |  5 ++++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 6c76bd821d17e7c7..29f4fe199c4ddcf0 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -43,6 +43,21 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
>  	uint32_t fmt = DRM_FORMAT_INVALID;
>  
>  	switch (bpp) {
> +	case 1:
> +		if (depth == 1)
> +			fmt = DRM_FORMAT_C1;
> +		break;
> +
> +	case 2:
> +		if (depth == 2)
> +			fmt = DRM_FORMAT_C2;
> +		break;
> +
> +	case 4:
> +		if (depth == 4)
> +			fmt = DRM_FORMAT_C4;
> +		break;
> +
>  	case 8:
>  		if (depth == 8)
>  			fmt = DRM_FORMAT_C8;
> @@ -132,6 +147,12 @@ EXPORT_SYMBOL(drm_driver_legacy_fb_format);
>  const struct drm_format_info *__drm_format_info(u32 format)
>  {
>  	static const struct drm_format_info formats[] = {
> +		{ .format = DRM_FORMAT_C1,		.depth = 1,  .num_planes = 1,
> +		  .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
> +		{ .format = DRM_FORMAT_C2,		.depth = 2,  .num_planes = 1,
> +		  .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
> +		{ .format = DRM_FORMAT_C4,		.depth = 4,  .num_planes = 1,
> +		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
>  		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
>  		{ .format = DRM_FORMAT_R8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
>  		{ .format = DRM_FORMAT_R10,		.depth = 10, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index fc0c1454d2757d5d..457ed39cc48f08e1 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -99,7 +99,10 @@ extern "C" {
>  #define DRM_FORMAT_INVALID	0
>  
>  /* color index */
> -#define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> +#define DRM_FORMAT_C1		fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> +#define DRM_FORMAT_C2		fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> +#define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> +#define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
>  
>  /* 8 bpp Red */
>  #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */

Hi Geert,

this patch looks good to me, so

Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Introduce fourcc codes for color-indexed frame buffer formats with two,
> four, and sixteen colors, and provide a mapping from bit per pixel and
> depth to fourcc codes.
> 
> As the number of bits per pixel is less than eight, these rely on proper
> block handling for the calculation of bits per pixel and pitch.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Introduce fourcc codes for color-indexed frame buffer formats with two,
> four, and sixteen colors, and provide a mapping from bit per pixel and
> depth to fourcc codes.
> 
> As the number of bits per pixel is less than eight, these rely on proper
> block handling for the calculation of bits per pixel and pitch.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Introduce fourcc codes for color-indexed frame buffer formats with two,
> four, and sixteen colors, and provide a mapping from bit per pixel and
> depth to fourcc codes.
>
> As the number of bits per pixel is less than eight, these rely on proper
> block handling for the calculation of bits per pixel and pitch.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -99,7 +99,10 @@ extern "C" {
>  #define DRM_FORMAT_INVALID     0
>
>  /* color index */
> -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
>
>  /* 8 bpp Red */
>  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */

After replying to Ilia's comment[1], I realized the CFB drawing
operations use native byte and bit ordering, unless
FBINFO_FOREIGN_ENDIAN is set.
While Amiga, Atari, and Sun-3 use big-endian bit ordering,
e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
(sh7760fb configures ordering to match host order).
BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
guess they are broken on big-endian.
Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
on little-endian.

Hence the above should become:

    #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
[7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
    #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
[7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
    #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
[7:0] C1:C0 4:4 two pixels/byte */

The same changes should be made for DRM_FORMAT_[RD][124].

The fbdev emulation code should gain support for these with and without
DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?

[1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
[2] See p.30 of the VIDC datasheet
    http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
[3] See p.1178 of the SH7660 datasheet
    https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf

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 v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Introduce fourcc codes for color-indexed frame buffer formats with two,
> four, and sixteen colors, and provide a mapping from bit per pixel and
> depth to fourcc codes.
>
> As the number of bits per pixel is less than eight, these rely on proper
> block handling for the calculation of bits per pixel and pitch.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -99,7 +99,10 @@ extern "C" {
>  #define DRM_FORMAT_INVALID     0
>
>  /* color index */
> -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
>
>  /* 8 bpp Red */
>  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */

After replying to Ilia's comment[1], I realized the CFB drawing
operations use native byte and bit ordering, unless
FBINFO_FOREIGN_ENDIAN is set.
While Amiga, Atari, and Sun-3 use big-endian bit ordering,
e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
(sh7760fb configures ordering to match host order).
BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
guess they are broken on big-endian.
Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
on little-endian.

Hence the above should become:

    #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
[7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
    #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
[7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
    #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
[7:0] C1:C0 4:4 two pixels/byte */

The same changes should be made for DRM_FORMAT_[RD][124].

The fbdev emulation code should gain support for these with and without
DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?

[1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
[2] See p.30 of the VIDC datasheet
    http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
[3] See p.1178 of the SH7660 datasheet
    https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf

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 v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3794 bytes --]

On Mon, 14 Mar 2022 14:30:18 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Introduce fourcc codes for color-indexed frame buffer formats with two,
> > four, and sixteen colors, and provide a mapping from bit per pixel and
> > depth to fourcc codes.
> >
> > As the number of bits per pixel is less than eight, these rely on proper
> > block handling for the calculation of bits per pixel and pitch.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>  
> 
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -99,7 +99,10 @@ extern "C" {
> >  #define DRM_FORMAT_INVALID     0
> >
> >  /* color index */
> > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> >
> >  /* 8 bpp Red */
> >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */  
> 
> After replying to Ilia's comment[1], I realized the CFB drawing
> operations use native byte and bit ordering, unless
> FBINFO_FOREIGN_ENDIAN is set.
> While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> (sh7760fb configures ordering to match host order).
> BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> guess they are broken on big-endian.
> Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> on little-endian.
> 
> Hence the above should become:
> 
>     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
>     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
>     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> [7:0] C1:C0 4:4 two pixels/byte */
> 
> The same changes should be made for DRM_FORMAT_[RD][124].
> 
> The fbdev emulation code should gain support for these with and without
> DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> 
> [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> [2] See p.30 of the VIDC datasheet
>     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> [3] See p.1178 of the SH7660 datasheet
>     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf

Hi Geert,

why would CPU endianess affect the order of bits in a byte?

Do you mean that bit 0 one machine is (1 << 0), and on another machine
bit 0 is (1 << 7)?

In C, we have only one way to address bits of a byte and that is with
arithmetic. You cannot take the address of a bit any other way, can you?

Can we standardise on "bit n of a byte is addressed as (1 << n)"?

I don't mind in which order the pixels are inside a byte, as long as it
doesn't change by CPU endianess. If you need both directions, then use
two different drm_fourcc codes that do not change their meaning by CPU
endianess. Just like we have XRGB and BGRX formats.

I would not like to see DRM_FORMAT_BIG_ENDIAN used for this, it would
conflate a whole new concept into the mess that is little- vs.
big-endian.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3794 bytes --]

On Mon, 14 Mar 2022 14:30:18 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Introduce fourcc codes for color-indexed frame buffer formats with two,
> > four, and sixteen colors, and provide a mapping from bit per pixel and
> > depth to fourcc codes.
> >
> > As the number of bits per pixel is less than eight, these rely on proper
> > block handling for the calculation of bits per pixel and pitch.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>  
> 
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -99,7 +99,10 @@ extern "C" {
> >  #define DRM_FORMAT_INVALID     0
> >
> >  /* color index */
> > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> >
> >  /* 8 bpp Red */
> >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */  
> 
> After replying to Ilia's comment[1], I realized the CFB drawing
> operations use native byte and bit ordering, unless
> FBINFO_FOREIGN_ENDIAN is set.
> While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> (sh7760fb configures ordering to match host order).
> BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> guess they are broken on big-endian.
> Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> on little-endian.
> 
> Hence the above should become:
> 
>     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
>     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
>     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> [7:0] C1:C0 4:4 two pixels/byte */
> 
> The same changes should be made for DRM_FORMAT_[RD][124].
> 
> The fbdev emulation code should gain support for these with and without
> DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> 
> [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> [2] See p.30 of the VIDC datasheet
>     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> [3] See p.1178 of the SH7660 datasheet
>     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf

Hi Geert,

why would CPU endianess affect the order of bits in a byte?

Do you mean that bit 0 one machine is (1 << 0), and on another machine
bit 0 is (1 << 7)?

In C, we have only one way to address bits of a byte and that is with
arithmetic. You cannot take the address of a bit any other way, can you?

Can we standardise on "bit n of a byte is addressed as (1 << n)"?

I don't mind in which order the pixels are inside a byte, as long as it
doesn't change by CPU endianess. If you need both directions, then use
two different drm_fourcc codes that do not change their meaning by CPU
endianess. Just like we have XRGB and BGRX formats.

I would not like to see DRM_FORMAT_BIG_ENDIAN used for this, it would
conflate a whole new concept into the mess that is little- vs.
big-endian.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Pekka,

On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Mon, 14 Mar 2022 14:30:18 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > Introduce fourcc codes for color-indexed frame buffer formats with two,
> > > four, and sixteen colors, and provide a mapping from bit per pixel and
> > > depth to fourcc codes.
> > >
> > > As the number of bits per pixel is less than eight, these rely on proper
> > > block handling for the calculation of bits per pixel and pitch.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -99,7 +99,10 @@ extern "C" {
> > >  #define DRM_FORMAT_INVALID     0
> > >
> > >  /* color index */
> > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > >
> > >  /* 8 bpp Red */
> > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
> >
> > After replying to Ilia's comment[1], I realized the CFB drawing
> > operations use native byte and bit ordering, unless
> > FBINFO_FOREIGN_ENDIAN is set.
> > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > (sh7760fb configures ordering to match host order).
> > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > guess they are broken on big-endian.
> > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > on little-endian.
> >
> > Hence the above should become:
> >
> >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > [7:0] C1:C0 4:4 two pixels/byte */
> >
> > The same changes should be made for DRM_FORMAT_[RD][124].
> >
> > The fbdev emulation code should gain support for these with and without
> > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> >
> > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > [2] See p.30 of the VIDC datasheet
> >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > [3] See p.1178 of the SH7660 datasheet
> >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf
>
> why would CPU endianess affect the order of bits in a byte?

It doesn't, but see below.

> Do you mean that bit 0 one machine is (1 << 0), and on another machine
> bit 0 is (1 << 7)?

No, I mean that in case of multiple pixels per byte, the display
hardware pumps out pixels to the CRTC starting from either the MSB
or the LSB of the first display byte.  Which order depends on the
display hardware, not on the CPU.

> In C, we have only one way to address bits of a byte and that is with
> arithmetic. You cannot take the address of a bit any other way, can you?
>
> Can we standardise on "bit n of a byte is addressed as (1 << n)"?

BIT(n) in Linux works the same for little- and big-endian CPUs.
But display hardware may use a different bit order.

> I don't mind in which order the pixels are inside a byte, as long as it
> doesn't change by CPU endianess. If you need both directions, then use
> two different drm_fourcc codes that do not change their meaning by CPU
> endianess. Just like we have XRGB and BGRX formats.

OK.

> I would not like to see DRM_FORMAT_BIG_ENDIAN used for this, it would
> conflate a whole new concept into the mess that is little- vs.
> big-endian.

OK.

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 v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Pekka,

On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Mon, 14 Mar 2022 14:30:18 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > Introduce fourcc codes for color-indexed frame buffer formats with two,
> > > four, and sixteen colors, and provide a mapping from bit per pixel and
> > > depth to fourcc codes.
> > >
> > > As the number of bits per pixel is less than eight, these rely on proper
> > > block handling for the calculation of bits per pixel and pitch.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -99,7 +99,10 @@ extern "C" {
> > >  #define DRM_FORMAT_INVALID     0
> > >
> > >  /* color index */
> > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > >
> > >  /* 8 bpp Red */
> > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
> >
> > After replying to Ilia's comment[1], I realized the CFB drawing
> > operations use native byte and bit ordering, unless
> > FBINFO_FOREIGN_ENDIAN is set.
> > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > (sh7760fb configures ordering to match host order).
> > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > guess they are broken on big-endian.
> > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > on little-endian.
> >
> > Hence the above should become:
> >
> >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > [7:0] C1:C0 4:4 two pixels/byte */
> >
> > The same changes should be made for DRM_FORMAT_[RD][124].
> >
> > The fbdev emulation code should gain support for these with and without
> > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> >
> > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > [2] See p.30 of the VIDC datasheet
> >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > [3] See p.1178 of the SH7660 datasheet
> >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf
>
> why would CPU endianess affect the order of bits in a byte?

It doesn't, but see below.

> Do you mean that bit 0 one machine is (1 << 0), and on another machine
> bit 0 is (1 << 7)?

No, I mean that in case of multiple pixels per byte, the display
hardware pumps out pixels to the CRTC starting from either the MSB
or the LSB of the first display byte.  Which order depends on the
display hardware, not on the CPU.

> In C, we have only one way to address bits of a byte and that is with
> arithmetic. You cannot take the address of a bit any other way, can you?
>
> Can we standardise on "bit n of a byte is addressed as (1 << n)"?

BIT(n) in Linux works the same for little- and big-endian CPUs.
But display hardware may use a different bit order.

> I don't mind in which order the pixels are inside a byte, as long as it
> doesn't change by CPU endianess. If you need both directions, then use
> two different drm_fourcc codes that do not change their meaning by CPU
> endianess. Just like we have XRGB and BGRX formats.

OK.

> I would not like to see DRM_FORMAT_BIG_ENDIAN used for this, it would
> conflate a whole new concept into the mess that is little- vs.
> big-endian.

OK.

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 v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Finn Thain]

Hi Geert,

On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:

> On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Mon, 14 Mar 2022 14:30:18 +0100
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > Introduce fourcc codes for color-indexed frame buffer formats with 
> > > > two, four, and sixteen colors, and provide a mapping from bit per 
> > > > pixel and depth to fourcc codes.
> > > >
> > > > As the number of bits per pixel is less than eight, these rely on 
> > > > proper block handling for the calculation of bits per pixel and 
> > > > pitch.
> > > >
> > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > >
> > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > @@ -99,7 +99,10 @@ extern "C" {
> > > >  #define DRM_FORMAT_INVALID     0
> > > >
> > > >  /* color index */
> > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > >
> > > >  /* 8 bpp Red */
> > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
> > >
> > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > operations use native byte and bit ordering, unless
> > > FBINFO_FOREIGN_ENDIAN is set.
> > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > (sh7760fb configures ordering to match host order).
> > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > guess they are broken on big-endian.
> > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > on little-endian.
> > >
> > > Hence the above should become:
> > >
> > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > [7:0] C1:C0 4:4 two pixels/byte */
> > >
> > > The same changes should be made for DRM_FORMAT_[RD][124].
> > >
> > > The fbdev emulation code should gain support for these with and without
> > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > >
> > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > [2] See p.30 of the VIDC datasheet
> > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > [3] See p.1178 of the SH7660 datasheet
> > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf
> >
> > why would CPU endianess affect the order of bits in a byte?
> 
> It doesn't, but see below.
> 
> > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > bit 0 is (1 << 7)?
> 
> No, I mean that in case of multiple pixels per byte, the display
> hardware pumps out pixels to the CRTC starting from either the MSB
> or the LSB of the first display byte.  Which order depends on the
> display hardware, not on the CPU.
> 
> > In C, we have only one way to address bits of a byte and that is with
> > arithmetic. You cannot take the address of a bit any other way, can you?
> >
> > Can we standardise on "bit n of a byte is addressed as (1 << n)"?
> 
> BIT(n) in Linux works the same for little- and big-endian CPUs.
> But display hardware may use a different bit order.
> 

Perhaps some of this confusion could be avoided if you describe the 
problem in terms of the sequence of scan-out of pixels, rather than in 
terms of the serialization of bits. The significance of bits within each 
pixel and the ordering of pixels within each memory word are independent, 
right?

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Finn Thain]

Hi Geert,

On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:

> On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Mon, 14 Mar 2022 14:30:18 +0100
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > Introduce fourcc codes for color-indexed frame buffer formats with 
> > > > two, four, and sixteen colors, and provide a mapping from bit per 
> > > > pixel and depth to fourcc codes.
> > > >
> > > > As the number of bits per pixel is less than eight, these rely on 
> > > > proper block handling for the calculation of bits per pixel and 
> > > > pitch.
> > > >
> > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > >
> > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > @@ -99,7 +99,10 @@ extern "C" {
> > > >  #define DRM_FORMAT_INVALID     0
> > > >
> > > >  /* color index */
> > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > >
> > > >  /* 8 bpp Red */
> > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
> > >
> > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > operations use native byte and bit ordering, unless
> > > FBINFO_FOREIGN_ENDIAN is set.
> > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > (sh7760fb configures ordering to match host order).
> > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > guess they are broken on big-endian.
> > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > on little-endian.
> > >
> > > Hence the above should become:
> > >
> > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > [7:0] C1:C0 4:4 two pixels/byte */
> > >
> > > The same changes should be made for DRM_FORMAT_[RD][124].
> > >
> > > The fbdev emulation code should gain support for these with and without
> > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > >
> > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > [2] See p.30 of the VIDC datasheet
> > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > [3] See p.1178 of the SH7660 datasheet
> > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf
> >
> > why would CPU endianess affect the order of bits in a byte?
> 
> It doesn't, but see below.
> 
> > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > bit 0 is (1 << 7)?
> 
> No, I mean that in case of multiple pixels per byte, the display
> hardware pumps out pixels to the CRTC starting from either the MSB
> or the LSB of the first display byte.  Which order depends on the
> display hardware, not on the CPU.
> 
> > In C, we have only one way to address bits of a byte and that is with
> > arithmetic. You cannot take the address of a bit any other way, can you?
> >
> > Can we standardise on "bit n of a byte is addressed as (1 << n)"?
> 
> BIT(n) in Linux works the same for little- and big-endian CPUs.
> But display hardware may use a different bit order.
> 

Perhaps some of this confusion could be avoided if you describe the 
problem in terms of the sequence of scan-out of pixels, rather than in 
terms of the serialization of bits. The significance of bits within each 
pixel and the ordering of pixels within each memory word are independent, 
right?

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5099 bytes --]

On Tue, 15 Mar 2022 09:15:08 +1100 (AEDT)
Finn Thain <fthain@linux-m68k.org> wrote:

> Hi Geert,
> 
> On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:
> 
> > On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > On Mon, 14 Mar 2022 14:30:18 +0100
> > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > > Introduce fourcc codes for color-indexed frame buffer formats with 
> > > > > two, four, and sixteen colors, and provide a mapping from bit per 
> > > > > pixel and depth to fourcc codes.
> > > > >
> > > > > As the number of bits per pixel is less than eight, these rely on 
> > > > > proper block handling for the calculation of bits per pixel and 
> > > > > pitch.
> > > > >
> > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>  
> > > >  
> > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > @@ -99,7 +99,10 @@ extern "C" {
> > > > >  #define DRM_FORMAT_INVALID     0
> > > > >
> > > > >  /* color index */
> > > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > > >
> > > > >  /* 8 bpp Red */
> > > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */  
> > > >
> > > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > > operations use native byte and bit ordering, unless
> > > > FBINFO_FOREIGN_ENDIAN is set.
> > > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > > (sh7760fb configures ordering to match host order).
> > > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > > guess they are broken on big-endian.
> > > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > > on little-endian.
> > > >
> > > > Hence the above should become:
> > > >
> > > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > > [7:0] C1:C0 4:4 two pixels/byte */
> > > >
> > > > The same changes should be made for DRM_FORMAT_[RD][124].
> > > >
> > > > The fbdev emulation code should gain support for these with and without
> > > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > > >
> > > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > > [2] See p.30 of the VIDC datasheet
> > > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > > [3] See p.1178 of the SH7660 datasheet
> > > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf  
> > >
> > > why would CPU endianess affect the order of bits in a byte?  
> > 
> > It doesn't, but see below.
> >   
> > > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > > bit 0 is (1 << 7)?  
> > 
> > No, I mean that in case of multiple pixels per byte, the display
> > hardware pumps out pixels to the CRTC starting from either the MSB
> > or the LSB of the first display byte.  Which order depends on the
> > display hardware, not on the CPU.
> >   
> > > In C, we have only one way to address bits of a byte and that is with
> > > arithmetic. You cannot take the address of a bit any other way, can you?
> > >
> > > Can we standardise on "bit n of a byte is addressed as (1 << n)"?  
> > 
> > BIT(n) in Linux works the same for little- and big-endian CPUs.
> > But display hardware may use a different bit order.
> >   
> 
> Perhaps some of this confusion could be avoided if you describe the 
> problem in terms of the sequence of scan-out of pixels, rather than in 
> terms of the serialization of bits. The significance of bits within each 
> pixel and the ordering of pixels within each memory word are independent, 
> right?

Yes, that might help.

Also, when drm_fourcc.h is describing pixel formats, it needs to
consider only how a little-endian CPU accesses them. That's how pixel
data in memory is described. Display hardware plays no part in that.
It is the driver's job to expose the pixel formats that match display
hardware behaviour.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5099 bytes --]

On Tue, 15 Mar 2022 09:15:08 +1100 (AEDT)
Finn Thain <fthain@linux-m68k.org> wrote:

> Hi Geert,
> 
> On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:
> 
> > On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > On Mon, 14 Mar 2022 14:30:18 +0100
> > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > > Introduce fourcc codes for color-indexed frame buffer formats with 
> > > > > two, four, and sixteen colors, and provide a mapping from bit per 
> > > > > pixel and depth to fourcc codes.
> > > > >
> > > > > As the number of bits per pixel is less than eight, these rely on 
> > > > > proper block handling for the calculation of bits per pixel and 
> > > > > pitch.
> > > > >
> > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>  
> > > >  
> > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > @@ -99,7 +99,10 @@ extern "C" {
> > > > >  #define DRM_FORMAT_INVALID     0
> > > > >
> > > > >  /* color index */
> > > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > > >
> > > > >  /* 8 bpp Red */
> > > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */  
> > > >
> > > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > > operations use native byte and bit ordering, unless
> > > > FBINFO_FOREIGN_ENDIAN is set.
> > > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > > (sh7760fb configures ordering to match host order).
> > > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > > guess they are broken on big-endian.
> > > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > > on little-endian.
> > > >
> > > > Hence the above should become:
> > > >
> > > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > > [7:0] C1:C0 4:4 two pixels/byte */
> > > >
> > > > The same changes should be made for DRM_FORMAT_[RD][124].
> > > >
> > > > The fbdev emulation code should gain support for these with and without
> > > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > > >
> > > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > > [2] See p.30 of the VIDC datasheet
> > > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > > [3] See p.1178 of the SH7660 datasheet
> > > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf  
> > >
> > > why would CPU endianess affect the order of bits in a byte?  
> > 
> > It doesn't, but see below.
> >   
> > > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > > bit 0 is (1 << 7)?  
> > 
> > No, I mean that in case of multiple pixels per byte, the display
> > hardware pumps out pixels to the CRTC starting from either the MSB
> > or the LSB of the first display byte.  Which order depends on the
> > display hardware, not on the CPU.
> >   
> > > In C, we have only one way to address bits of a byte and that is with
> > > arithmetic. You cannot take the address of a bit any other way, can you?
> > >
> > > Can we standardise on "bit n of a byte is addressed as (1 << n)"?  
> > 
> > BIT(n) in Linux works the same for little- and big-endian CPUs.
> > But display hardware may use a different bit order.
> >   
> 
> Perhaps some of this confusion could be avoided if you describe the 
> problem in terms of the sequence of scan-out of pixels, rather than in 
> terms of the serialization of bits. The significance of bits within each 
> pixel and the ordering of pixels within each memory word are independent, 
> right?

Yes, that might help.

Also, when drm_fourcc.h is describing pixel formats, it needs to
consider only how a little-endian CPU accesses them. That's how pixel
data in memory is described. Display hardware plays no part in that.
It is the driver's job to expose the pixel formats that match display
hardware behaviour.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Pekka,

On Tue, Mar 15, 2022 at 8:33 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Tue, 15 Mar 2022 09:15:08 +1100 (AEDT)
> Finn Thain <fthain@linux-m68k.org> wrote:
> > On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:
> > > On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > > On Mon, 14 Mar 2022 14:30:18 +0100
> > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > Introduce fourcc codes for color-indexed frame buffer formats with
> > > > > > two, four, and sixteen colors, and provide a mapping from bit per
> > > > > > pixel and depth to fourcc codes.
> > > > > >
> > > > > > As the number of bits per pixel is less than eight, these rely on
> > > > > > proper block handling for the calculation of bits per pixel and
> > > > > > pitch.
> > > > > >
> > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > >
> > > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > > @@ -99,7 +99,10 @@ extern "C" {
> > > > > >  #define DRM_FORMAT_INVALID     0
> > > > > >
> > > > > >  /* color index */
> > > > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > > > >
> > > > > >  /* 8 bpp Red */
> > > > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
> > > > >
> > > > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > > > operations use native byte and bit ordering, unless
> > > > > FBINFO_FOREIGN_ENDIAN is set.
> > > > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > > > (sh7760fb configures ordering to match host order).
> > > > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > > > guess they are broken on big-endian.
> > > > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > > > on little-endian.
> > > > >
> > > > > Hence the above should become:
> > > > >
> > > > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > > > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > > > [7:0] C1:C0 4:4 two pixels/byte */
> > > > >
> > > > > The same changes should be made for DRM_FORMAT_[RD][124].
> > > > >
> > > > > The fbdev emulation code should gain support for these with and without
> > > > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > > > >
> > > > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > > > [2] See p.30 of the VIDC datasheet
> > > > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > > > [3] See p.1178 of the SH7660 datasheet
> > > > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf
> > > >
> > > > why would CPU endianess affect the order of bits in a byte?
> > >
> > > It doesn't, but see below.
> > >
> > > > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > > > bit 0 is (1 << 7)?
> > >
> > > No, I mean that in case of multiple pixels per byte, the display
> > > hardware pumps out pixels to the CRTC starting from either the MSB
> > > or the LSB of the first display byte.  Which order depends on the
> > > display hardware, not on the CPU.
> > >
> > > > In C, we have only one way to address bits of a byte and that is with
> > > > arithmetic. You cannot take the address of a bit any other way, can you?
> > > >
> > > > Can we standardise on "bit n of a byte is addressed as (1 << n)"?
> > >
> > > BIT(n) in Linux works the same for little- and big-endian CPUs.
> > > But display hardware may use a different bit order.
> >
> > Perhaps some of this confusion could be avoided if you describe the
> > problem in terms of the sequence of scan-out of pixels, rather than in
> > terms of the serialization of bits. The significance of bits within each
> > pixel and the ordering of pixels within each memory word are independent,
> > right?
>
> Yes, that might help.

Display:

     P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15

    P15 P14 P13 P12 P11 P10  P9  P8  P7  P6  P5  P4  P3  P2  P1  P0

Memory:

  1 bpp (MSB first):

              bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
              ---- ---- ---- ---- ---- ---- ---- ----
      byte 0:   P0   P1   P2   P3   P4   P5   P6   P7
      byte 1:   P8   P9  P10  P11  P12  P13  P14  P15

  1 bpp (LSB first):

              bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
              ---- ---- ---- ---- ---- ---- ---- ----
      byte 0:   P7   P6   P5   P4   P3   P2   P1   P0
      byte 1:  P15  P14  P13  P12  P11  P10   P9   P8

  2 bpp (MSB first):

              bits7-6 bits5-4 bits3-2 bits1-0
              ------- ------- ------- -------
      byte 0:    P0      P1      P2      P3
      byte 1:    P4      P5      P6      P7
      byte 2:    P8      P9     P10     P11
      byte 3:   P12     P13     P14     P15

  2 bpp (LSB first):

              bits7-6 bits5-4 bits3-2 bits1-0
              ------- ------- ------- -------
      byte 0:    P3      P2      P1      P0
      byte 1:    P7      P6      P5      P4
      byte 2:   P11     P10      P9      P8
      byte 3:   P15     P14     P13     P12

  4 bpp (MSB first):

              bits7-4 bits3-0
              ------- -------
      byte 0:    P0      P1
      byte 1:    P2      P3
      byte 2:    P4      P5
      byte 3:    P6      P7
      byte 4:    P8      P9
      byte 5:   P10     P11
      byte 6:   P12     P13
      byte 7:   P14     P15

  4 bpp (LSB first):

              bits7-4 bits3-0
              ------- -------
      byte 0:    P1      P0
      byte 1:    P3      P2
      byte 2:    P5      P4
      byte 3:    P7      P6
      byte 4:    P9      P8
      byte 5:   P11     P10
      byte 6:   P13     P12
      byte 7:   P15     P14

> Also, when drm_fourcc.h is describing pixel formats, it needs to
> consider only how a little-endian CPU accesses them. That's how pixel
> data in memory is described. Display hardware plays no part in that.
> It is the driver's job to expose the pixel formats that match display
> hardware behaviour.

But if the "CPU format" does not match the "display support",
all pixel data must be converted?

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 v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Pekka,

On Tue, Mar 15, 2022 at 8:33 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Tue, 15 Mar 2022 09:15:08 +1100 (AEDT)
> Finn Thain <fthain@linux-m68k.org> wrote:
> > On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:
> > > On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > > On Mon, 14 Mar 2022 14:30:18 +0100
> > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > Introduce fourcc codes for color-indexed frame buffer formats with
> > > > > > two, four, and sixteen colors, and provide a mapping from bit per
> > > > > > pixel and depth to fourcc codes.
> > > > > >
> > > > > > As the number of bits per pixel is less than eight, these rely on
> > > > > > proper block handling for the calculation of bits per pixel and
> > > > > > pitch.
> > > > > >
> > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > >
> > > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > > @@ -99,7 +99,10 @@ extern "C" {
> > > > > >  #define DRM_FORMAT_INVALID     0
> > > > > >
> > > > > >  /* color index */
> > > > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > > > >
> > > > > >  /* 8 bpp Red */
> > > > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
> > > > >
> > > > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > > > operations use native byte and bit ordering, unless
> > > > > FBINFO_FOREIGN_ENDIAN is set.
> > > > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > > > (sh7760fb configures ordering to match host order).
> > > > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > > > guess they are broken on big-endian.
> > > > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > > > on little-endian.
> > > > >
> > > > > Hence the above should become:
> > > > >
> > > > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > > > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > > > [7:0] C1:C0 4:4 two pixels/byte */
> > > > >
> > > > > The same changes should be made for DRM_FORMAT_[RD][124].
> > > > >
> > > > > The fbdev emulation code should gain support for these with and without
> > > > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > > > >
> > > > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > > > [2] See p.30 of the VIDC datasheet
> > > > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > > > [3] See p.1178 of the SH7660 datasheet
> > > > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf
> > > >
> > > > why would CPU endianess affect the order of bits in a byte?
> > >
> > > It doesn't, but see below.
> > >
> > > > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > > > bit 0 is (1 << 7)?
> > >
> > > No, I mean that in case of multiple pixels per byte, the display
> > > hardware pumps out pixels to the CRTC starting from either the MSB
> > > or the LSB of the first display byte.  Which order depends on the
> > > display hardware, not on the CPU.
> > >
> > > > In C, we have only one way to address bits of a byte and that is with
> > > > arithmetic. You cannot take the address of a bit any other way, can you?
> > > >
> > > > Can we standardise on "bit n of a byte is addressed as (1 << n)"?
> > >
> > > BIT(n) in Linux works the same for little- and big-endian CPUs.
> > > But display hardware may use a different bit order.
> >
> > Perhaps some of this confusion could be avoided if you describe the
> > problem in terms of the sequence of scan-out of pixels, rather than in
> > terms of the serialization of bits. The significance of bits within each
> > pixel and the ordering of pixels within each memory word are independent,
> > right?
>
> Yes, that might help.

Display:

     P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15

    P15 P14 P13 P12 P11 P10  P9  P8  P7  P6  P5  P4  P3  P2  P1  P0

Memory:

  1 bpp (MSB first):

              bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
              ---- ---- ---- ---- ---- ---- ---- ----
      byte 0:   P0   P1   P2   P3   P4   P5   P6   P7
      byte 1:   P8   P9  P10  P11  P12  P13  P14  P15

  1 bpp (LSB first):

              bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
              ---- ---- ---- ---- ---- ---- ---- ----
      byte 0:   P7   P6   P5   P4   P3   P2   P1   P0
      byte 1:  P15  P14  P13  P12  P11  P10   P9   P8

  2 bpp (MSB first):

              bits7-6 bits5-4 bits3-2 bits1-0
              ------- ------- ------- -------
      byte 0:    P0      P1      P2      P3
      byte 1:    P4      P5      P6      P7
      byte 2:    P8      P9     P10     P11
      byte 3:   P12     P13     P14     P15

  2 bpp (LSB first):

              bits7-6 bits5-4 bits3-2 bits1-0
              ------- ------- ------- -------
      byte 0:    P3      P2      P1      P0
      byte 1:    P7      P6      P5      P4
      byte 2:   P11     P10      P9      P8
      byte 3:   P15     P14     P13     P12

  4 bpp (MSB first):

              bits7-4 bits3-0
              ------- -------
      byte 0:    P0      P1
      byte 1:    P2      P3
      byte 2:    P4      P5
      byte 3:    P6      P7
      byte 4:    P8      P9
      byte 5:   P10     P11
      byte 6:   P12     P13
      byte 7:   P14     P15

  4 bpp (LSB first):

              bits7-4 bits3-0
              ------- -------
      byte 0:    P1      P0
      byte 1:    P3      P2
      byte 2:    P5      P4
      byte 3:    P7      P6
      byte 4:    P9      P8
      byte 5:   P11     P10
      byte 6:   P13     P12
      byte 7:   P15     P14

> Also, when drm_fourcc.h is describing pixel formats, it needs to
> consider only how a little-endian CPU accesses them. That's how pixel
> data in memory is described. Display hardware plays no part in that.
> It is the driver's job to expose the pixel formats that match display
> hardware behaviour.

But if the "CPU format" does not match the "display support",
all pixel data must be converted?

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 v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

On Tue, Mar 15, 2022 at 8:51 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Also, when drm_fourcc.h is describing pixel formats, it needs to
> > consider only how a little-endian CPU accesses them. That's how pixel
> > data in memory is described. Display hardware plays no part in that.
> > It is the driver's job to expose the pixel formats that match display
> > hardware behaviour.
>
> But if the "CPU format" does not match the "display support",

s/support/format/

> all pixel data must be converted?

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 v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

On Tue, Mar 15, 2022 at 8:51 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Also, when drm_fourcc.h is describing pixel formats, it needs to
> > consider only how a little-endian CPU accesses them. That's how pixel
> > data in memory is described. Display hardware plays no part in that.
> > It is the driver's job to expose the pixel formats that match display
> > hardware behaviour.
>
> But if the "CPU format" does not match the "display support",

s/support/format/

> all pixel data must be converted?

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 v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 8596 bytes --]

On Tue, 15 Mar 2022 08:51:31 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Pekka,
> 
> On Tue, Mar 15, 2022 at 8:33 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Tue, 15 Mar 2022 09:15:08 +1100 (AEDT)
> > Finn Thain <fthain@linux-m68k.org> wrote:  
> > > On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:  
> > > > On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > > > On Mon, 14 Mar 2022 14:30:18 +0100
> > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > > > > Introduce fourcc codes for color-indexed frame buffer formats with
> > > > > > > two, four, and sixteen colors, and provide a mapping from bit per
> > > > > > > pixel and depth to fourcc codes.
> > > > > > >
> > > > > > > As the number of bits per pixel is less than eight, these rely on
> > > > > > > proper block handling for the calculation of bits per pixel and
> > > > > > > pitch.
> > > > > > >
> > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>  
> > > > > >  
> > > > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > > > @@ -99,7 +99,10 @@ extern "C" {
> > > > > > >  #define DRM_FORMAT_INVALID     0
> > > > > > >
> > > > > > >  /* color index */
> > > > > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > > > > >
> > > > > > >  /* 8 bpp Red */
> > > > > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */  
> > > > > >
> > > > > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > > > > operations use native byte and bit ordering, unless
> > > > > > FBINFO_FOREIGN_ENDIAN is set.
> > > > > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > > > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > > > > (sh7760fb configures ordering to match host order).
> > > > > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > > > > guess they are broken on big-endian.
> > > > > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > > > > on little-endian.
> > > > > >
> > > > > > Hence the above should become:
> > > > > >
> > > > > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > > > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > > > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > > > > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > > > > [7:0] C1:C0 4:4 two pixels/byte */
> > > > > >
> > > > > > The same changes should be made for DRM_FORMAT_[RD][124].
> > > > > >
> > > > > > The fbdev emulation code should gain support for these with and without
> > > > > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > > > > >
> > > > > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > > > > [2] See p.30 of the VIDC datasheet
> > > > > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > > > > [3] See p.1178 of the SH7660 datasheet
> > > > > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf  
> > > > >
> > > > > why would CPU endianess affect the order of bits in a byte?  
> > > >
> > > > It doesn't, but see below.
> > > >  
> > > > > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > > > > bit 0 is (1 << 7)?  
> > > >
> > > > No, I mean that in case of multiple pixels per byte, the display
> > > > hardware pumps out pixels to the CRTC starting from either the MSB
> > > > or the LSB of the first display byte.  Which order depends on the
> > > > display hardware, not on the CPU.
> > > >  
> > > > > In C, we have only one way to address bits of a byte and that is with
> > > > > arithmetic. You cannot take the address of a bit any other way, can you?
> > > > >
> > > > > Can we standardise on "bit n of a byte is addressed as (1 << n)"?  
> > > >
> > > > BIT(n) in Linux works the same for little- and big-endian CPUs.
> > > > But display hardware may use a different bit order.  
> > >
> > > Perhaps some of this confusion could be avoided if you describe the
> > > problem in terms of the sequence of scan-out of pixels, rather than in
> > > terms of the serialization of bits. The significance of bits within each
> > > pixel and the ordering of pixels within each memory word are independent,
> > > right?  
> >
> > Yes, that might help.  
> 
> Display:
> 
>      P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15
> 
>     P15 P14 P13 P12 P11 P10  P9  P8  P7  P6  P5  P4  P3  P2  P1  P0

Hi Geert,

does this mean the display hardware emits even rows from left to right
and odd rows from right to left?

I suppose that would practically eliminate the horizontal blanking
period in CRT timings. If so, I think that might be best represented as
a new format modifier.

I'm guessing P stands for "pixel".

> 
> Memory:
> 
>   1 bpp (MSB first):
> 
>               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
>               ---- ---- ---- ---- ---- ---- ---- ----
>       byte 0:   P0   P1   P2   P3   P4   P5   P6   P7
>       byte 1:   P8   P9  P10  P11  P12  P13  P14  P15
> 
>   1 bpp (LSB first):
> 
>               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
>               ---- ---- ---- ---- ---- ---- ---- ----
>       byte 0:   P7   P6   P5   P4   P3   P2   P1   P0
>       byte 1:  P15  P14  P13  P12  P11  P10   P9   P8
> 
>   2 bpp (MSB first):
> 
>               bits7-6 bits5-4 bits3-2 bits1-0
>               ------- ------- ------- -------
>       byte 0:    P0      P1      P2      P3
>       byte 1:    P4      P5      P6      P7
>       byte 2:    P8      P9     P10     P11
>       byte 3:   P12     P13     P14     P15
> 
>   2 bpp (LSB first):
> 
>               bits7-6 bits5-4 bits3-2 bits1-0
>               ------- ------- ------- -------
>       byte 0:    P3      P2      P1      P0
>       byte 1:    P7      P6      P5      P4
>       byte 2:   P11     P10      P9      P8
>       byte 3:   P15     P14     P13     P12
> 
>   4 bpp (MSB first):
> 
>               bits7-4 bits3-0
>               ------- -------
>       byte 0:    P0      P1
>       byte 1:    P2      P3
>       byte 2:    P4      P5
>       byte 3:    P6      P7
>       byte 4:    P8      P9
>       byte 5:   P10     P11
>       byte 6:   P12     P13
>       byte 7:   P14     P15
> 
>   4 bpp (LSB first):
> 
>               bits7-4 bits3-0
>               ------- -------
>       byte 0:    P1      P0
>       byte 1:    P3      P2
>       byte 2:    P5      P4
>       byte 3:    P7      P6
>       byte 4:    P9      P8
>       byte 5:   P11     P10
>       byte 6:   P13     P12
>       byte 7:   P15     P14

I think I can guess what you meant there, and it looks understandable
to me. These tables are actually very clear, and leave only one thing
undefined: when multiple bits form a pixel, in which order do the bits
form the value. I recall you said fbdev allows for both orderings but
only one order is ever used if I understood right.

> > Also, when drm_fourcc.h is describing pixel formats, it needs to
> > consider only how a little-endian CPU accesses them. That's how pixel
> > data in memory is described. Display hardware plays no part in that.
> > It is the driver's job to expose the pixel formats that match display
> > hardware behaviour.  
> 
> But if the "CPU format" does not match the "display support",
> all pixel data must be converted?

Of course. If the driver author does not want to convert pixel data in
flight, then the author should not let the driver expose a format that
needs conversion.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 8596 bytes --]

On Tue, 15 Mar 2022 08:51:31 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Pekka,
> 
> On Tue, Mar 15, 2022 at 8:33 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Tue, 15 Mar 2022 09:15:08 +1100 (AEDT)
> > Finn Thain <fthain@linux-m68k.org> wrote:  
> > > On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:  
> > > > On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > > > On Mon, 14 Mar 2022 14:30:18 +0100
> > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > > > > Introduce fourcc codes for color-indexed frame buffer formats with
> > > > > > > two, four, and sixteen colors, and provide a mapping from bit per
> > > > > > > pixel and depth to fourcc codes.
> > > > > > >
> > > > > > > As the number of bits per pixel is less than eight, these rely on
> > > > > > > proper block handling for the calculation of bits per pixel and
> > > > > > > pitch.
> > > > > > >
> > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>  
> > > > > >  
> > > > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > > > @@ -99,7 +99,10 @@ extern "C" {
> > > > > > >  #define DRM_FORMAT_INVALID     0
> > > > > > >
> > > > > > >  /* color index */
> > > > > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > > > > >
> > > > > > >  /* 8 bpp Red */
> > > > > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */  
> > > > > >
> > > > > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > > > > operations use native byte and bit ordering, unless
> > > > > > FBINFO_FOREIGN_ENDIAN is set.
> > > > > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > > > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > > > > (sh7760fb configures ordering to match host order).
> > > > > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > > > > guess they are broken on big-endian.
> > > > > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > > > > on little-endian.
> > > > > >
> > > > > > Hence the above should become:
> > > > > >
> > > > > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > > > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > > > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > > > > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > > > > [7:0] C1:C0 4:4 two pixels/byte */
> > > > > >
> > > > > > The same changes should be made for DRM_FORMAT_[RD][124].
> > > > > >
> > > > > > The fbdev emulation code should gain support for these with and without
> > > > > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > > > > >
> > > > > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > > > > [2] See p.30 of the VIDC datasheet
> > > > > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > > > > [3] See p.1178 of the SH7660 datasheet
> > > > > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf  
> > > > >
> > > > > why would CPU endianess affect the order of bits in a byte?  
> > > >
> > > > It doesn't, but see below.
> > > >  
> > > > > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > > > > bit 0 is (1 << 7)?  
> > > >
> > > > No, I mean that in case of multiple pixels per byte, the display
> > > > hardware pumps out pixels to the CRTC starting from either the MSB
> > > > or the LSB of the first display byte.  Which order depends on the
> > > > display hardware, not on the CPU.
> > > >  
> > > > > In C, we have only one way to address bits of a byte and that is with
> > > > > arithmetic. You cannot take the address of a bit any other way, can you?
> > > > >
> > > > > Can we standardise on "bit n of a byte is addressed as (1 << n)"?  
> > > >
> > > > BIT(n) in Linux works the same for little- and big-endian CPUs.
> > > > But display hardware may use a different bit order.  
> > >
> > > Perhaps some of this confusion could be avoided if you describe the
> > > problem in terms of the sequence of scan-out of pixels, rather than in
> > > terms of the serialization of bits. The significance of bits within each
> > > pixel and the ordering of pixels within each memory word are independent,
> > > right?  
> >
> > Yes, that might help.  
> 
> Display:
> 
>      P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15
> 
>     P15 P14 P13 P12 P11 P10  P9  P8  P7  P6  P5  P4  P3  P2  P1  P0

Hi Geert,

does this mean the display hardware emits even rows from left to right
and odd rows from right to left?

I suppose that would practically eliminate the horizontal blanking
period in CRT timings. If so, I think that might be best represented as
a new format modifier.

I'm guessing P stands for "pixel".

> 
> Memory:
> 
>   1 bpp (MSB first):
> 
>               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
>               ---- ---- ---- ---- ---- ---- ---- ----
>       byte 0:   P0   P1   P2   P3   P4   P5   P6   P7
>       byte 1:   P8   P9  P10  P11  P12  P13  P14  P15
> 
>   1 bpp (LSB first):
> 
>               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
>               ---- ---- ---- ---- ---- ---- ---- ----
>       byte 0:   P7   P6   P5   P4   P3   P2   P1   P0
>       byte 1:  P15  P14  P13  P12  P11  P10   P9   P8
> 
>   2 bpp (MSB first):
> 
>               bits7-6 bits5-4 bits3-2 bits1-0
>               ------- ------- ------- -------
>       byte 0:    P0      P1      P2      P3
>       byte 1:    P4      P5      P6      P7
>       byte 2:    P8      P9     P10     P11
>       byte 3:   P12     P13     P14     P15
> 
>   2 bpp (LSB first):
> 
>               bits7-6 bits5-4 bits3-2 bits1-0
>               ------- ------- ------- -------
>       byte 0:    P3      P2      P1      P0
>       byte 1:    P7      P6      P5      P4
>       byte 2:   P11     P10      P9      P8
>       byte 3:   P15     P14     P13     P12
> 
>   4 bpp (MSB first):
> 
>               bits7-4 bits3-0
>               ------- -------
>       byte 0:    P0      P1
>       byte 1:    P2      P3
>       byte 2:    P4      P5
>       byte 3:    P6      P7
>       byte 4:    P8      P9
>       byte 5:   P10     P11
>       byte 6:   P12     P13
>       byte 7:   P14     P15
> 
>   4 bpp (LSB first):
> 
>               bits7-4 bits3-0
>               ------- -------
>       byte 0:    P1      P0
>       byte 1:    P3      P2
>       byte 2:    P5      P4
>       byte 3:    P7      P6
>       byte 4:    P9      P8
>       byte 5:   P11     P10
>       byte 6:   P13     P12
>       byte 7:   P15     P14

I think I can guess what you meant there, and it looks understandable
to me. These tables are actually very clear, and leave only one thing
undefined: when multiple bits form a pixel, in which order do the bits
form the value. I recall you said fbdev allows for both orderings but
only one order is ever used if I understood right.

> > Also, when drm_fourcc.h is describing pixel formats, it needs to
> > consider only how a little-endian CPU accesses them. That's how pixel
> > data in memory is described. Display hardware plays no part in that.
> > It is the driver's job to expose the pixel formats that match display
> > hardware behaviour.  
> 
> But if the "CPU format" does not match the "display support",
> all pixel data must be converted?

Of course. If the driver author does not want to convert pixel data in
flight, then the author should not let the driver expose a format that
needs conversion.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Pekka,

On Tue, Mar 15, 2022 at 9:46 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Tue, 15 Mar 2022 08:51:31 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Mar 15, 2022 at 8:33 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > On Tue, 15 Mar 2022 09:15:08 +1100 (AEDT)
> > > Finn Thain <fthain@linux-m68k.org> wrote:
> > > > On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:
> > > > > On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > > > > On Mon, 14 Mar 2022 14:30:18 +0100
> > > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > > Introduce fourcc codes for color-indexed frame buffer formats with
> > > > > > > > two, four, and sixteen colors, and provide a mapping from bit per
> > > > > > > > pixel and depth to fourcc codes.
> > > > > > > >
> > > > > > > > As the number of bits per pixel is less than eight, these rely on
> > > > > > > > proper block handling for the calculation of bits per pixel and
> > > > > > > > pitch.
> > > > > > > >
> > > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > >
> > > > > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > > > > @@ -99,7 +99,10 @@ extern "C" {
> > > > > > > >  #define DRM_FORMAT_INVALID     0
> > > > > > > >
> > > > > > > >  /* color index */
> > > > > > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > > > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > > > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > > > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > > > > > >
> > > > > > > >  /* 8 bpp Red */
> > > > > > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
> > > > > > >
> > > > > > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > > > > > operations use native byte and bit ordering, unless
> > > > > > > FBINFO_FOREIGN_ENDIAN is set.
> > > > > > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > > > > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > > > > > (sh7760fb configures ordering to match host order).
> > > > > > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > > > > > guess they are broken on big-endian.
> > > > > > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > > > > > on little-endian.
> > > > > > >
> > > > > > > Hence the above should become:
> > > > > > >
> > > > > > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > > > > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > > > > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > > > > > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > > > > > [7:0] C1:C0 4:4 two pixels/byte */
> > > > > > >
> > > > > > > The same changes should be made for DRM_FORMAT_[RD][124].
> > > > > > >
> > > > > > > The fbdev emulation code should gain support for these with and without
> > > > > > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > > > > > [2] See p.30 of the VIDC datasheet
> > > > > > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > > > > > [3] See p.1178 of the SH7660 datasheet
> > > > > > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf
> > > > > >
> > > > > > why would CPU endianess affect the order of bits in a byte?
> > > > >
> > > > > It doesn't, but see below.
> > > > >
> > > > > > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > > > > > bit 0 is (1 << 7)?
> > > > >
> > > > > No, I mean that in case of multiple pixels per byte, the display
> > > > > hardware pumps out pixels to the CRTC starting from either the MSB
> > > > > or the LSB of the first display byte.  Which order depends on the
> > > > > display hardware, not on the CPU.
> > > > >
> > > > > > In C, we have only one way to address bits of a byte and that is with
> > > > > > arithmetic. You cannot take the address of a bit any other way, can you?
> > > > > >
> > > > > > Can we standardise on "bit n of a byte is addressed as (1 << n)"?
> > > > >
> > > > > BIT(n) in Linux works the same for little- and big-endian CPUs.
> > > > > But display hardware may use a different bit order.
> > > >
> > > > Perhaps some of this confusion could be avoided if you describe the
> > > > problem in terms of the sequence of scan-out of pixels, rather than in
> > > > terms of the serialization of bits. The significance of bits within each
> > > > pixel and the ordering of pixels within each memory word are independent,
> > > > right?
> > >
> > > Yes, that might help.
> >
> > Display:
> >
> >      P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15
> >
> >     P15 P14 P13 P12 P11 P10  P9  P8  P7  P6  P5  P4  P3  P2  P1  P0
>
> Hi Geert,
>
> does this mean the display hardware emits even rows from left to right
> and odd rows from right to left?

No, it means I should have my morning coffee first, and remove all
temporary cruft before pressing send :-(

The above paragraph should have read:

    Display (16 pixels):

        P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15

> I'm guessing P stands for "pixel".

Exactly.

> > Memory:
> >
> >   1 bpp (MSB first):
> >
> >               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
> >               ---- ---- ---- ---- ---- ---- ---- ----
> >       byte 0:   P0   P1   P2   P3   P4   P5   P6   P7
> >       byte 1:   P8   P9  P10  P11  P12  P13  P14  P15
> >
> >   1 bpp (LSB first):
> >
> >               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
> >               ---- ---- ---- ---- ---- ---- ---- ----
> >       byte 0:   P7   P6   P5   P4   P3   P2   P1   P0
> >       byte 1:  P15  P14  P13  P12  P11  P10   P9   P8
> >
> >   2 bpp (MSB first):
> >
> >               bits7-6 bits5-4 bits3-2 bits1-0
> >               ------- ------- ------- -------
> >       byte 0:    P0      P1      P2      P3
> >       byte 1:    P4      P5      P6      P7
> >       byte 2:    P8      P9     P10     P11
> >       byte 3:   P12     P13     P14     P15
> >
> >   2 bpp (LSB first):
> >
> >               bits7-6 bits5-4 bits3-2 bits1-0
> >               ------- ------- ------- -------
> >       byte 0:    P3      P2      P1      P0
> >       byte 1:    P7      P6      P5      P4
> >       byte 2:   P11     P10      P9      P8
> >       byte 3:   P15     P14     P13     P12
> >
> >   4 bpp (MSB first):
> >
> >               bits7-4 bits3-0
> >               ------- -------
> >       byte 0:    P0      P1
> >       byte 1:    P2      P3
> >       byte 2:    P4      P5
> >       byte 3:    P6      P7
> >       byte 4:    P8      P9
> >       byte 5:   P10     P11
> >       byte 6:   P12     P13
> >       byte 7:   P14     P15
> >
> >   4 bpp (LSB first):
> >
> >               bits7-4 bits3-0
> >               ------- -------
> >       byte 0:    P1      P0
> >       byte 1:    P3      P2
> >       byte 2:    P5      P4
> >       byte 3:    P7      P6
> >       byte 4:    P9      P8
> >       byte 5:   P11     P10
> >       byte 6:   P13     P12
> >       byte 7:   P15     P14
>
> I think I can guess what you meant there, and it looks understandable
> to me. These tables are actually very clear, and leave only one thing
> undefined: when multiple bits form a pixel, in which order do the bits
> form the value. I recall you said fbdev allows for both orderings but
> only one order is ever used if I understood right.

Indeed.  The third ordering is the ordering of the bits in a pixel.
As fb_bitfield.msb_right is always false, no hardware ever supported by
fbdev used the other ordering, so we only have to care about:

   1 bpp: P = [ bitN ]
   2 bpp: P = [ bitN bitN-1 ]
   4 bpp: P = [ bitN bitN-1 bitN-2 bitN-3 ]

> > > Also, when drm_fourcc.h is describing pixel formats, it needs to
> > > consider only how a little-endian CPU accesses them. That's how pixel
> > > data in memory is described. Display hardware plays no part in that.
> > > It is the driver's job to expose the pixel formats that match display
> > > hardware behaviour.
> >
> > But if the "CPU format" does not match the "display support",
> > all pixel data must be converted?
>
> Of course. If the driver author does not want to convert pixel data in
> flight, then the author should not let the driver expose a format that
> needs conversion.

... in which case we need a DRM fourcc code for the format?

BTW, Atari and Amiga use bitplanes for bpp <= 8, so they need
conversion anyway.

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 v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Pekka,

On Tue, Mar 15, 2022 at 9:46 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Tue, 15 Mar 2022 08:51:31 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Mar 15, 2022 at 8:33 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > On Tue, 15 Mar 2022 09:15:08 +1100 (AEDT)
> > > Finn Thain <fthain@linux-m68k.org> wrote:
> > > > On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:
> > > > > On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > > > > On Mon, 14 Mar 2022 14:30:18 +0100
> > > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > > Introduce fourcc codes for color-indexed frame buffer formats with
> > > > > > > > two, four, and sixteen colors, and provide a mapping from bit per
> > > > > > > > pixel and depth to fourcc codes.
> > > > > > > >
> > > > > > > > As the number of bits per pixel is less than eight, these rely on
> > > > > > > > proper block handling for the calculation of bits per pixel and
> > > > > > > > pitch.
> > > > > > > >
> > > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > >
> > > > > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > > > > @@ -99,7 +99,10 @@ extern "C" {
> > > > > > > >  #define DRM_FORMAT_INVALID     0
> > > > > > > >
> > > > > > > >  /* color index */
> > > > > > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > > > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > > > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > > > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > > > > > >
> > > > > > > >  /* 8 bpp Red */
> > > > > > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
> > > > > > >
> > > > > > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > > > > > operations use native byte and bit ordering, unless
> > > > > > > FBINFO_FOREIGN_ENDIAN is set.
> > > > > > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > > > > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > > > > > (sh7760fb configures ordering to match host order).
> > > > > > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > > > > > guess they are broken on big-endian.
> > > > > > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > > > > > on little-endian.
> > > > > > >
> > > > > > > Hence the above should become:
> > > > > > >
> > > > > > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > > > > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > > > > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > > > > > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > > > > > [7:0] C1:C0 4:4 two pixels/byte */
> > > > > > >
> > > > > > > The same changes should be made for DRM_FORMAT_[RD][124].
> > > > > > >
> > > > > > > The fbdev emulation code should gain support for these with and without
> > > > > > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > > > > > [2] See p.30 of the VIDC datasheet
> > > > > > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > > > > > [3] See p.1178 of the SH7660 datasheet
> > > > > > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf
> > > > > >
> > > > > > why would CPU endianess affect the order of bits in a byte?
> > > > >
> > > > > It doesn't, but see below.
> > > > >
> > > > > > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > > > > > bit 0 is (1 << 7)?
> > > > >
> > > > > No, I mean that in case of multiple pixels per byte, the display
> > > > > hardware pumps out pixels to the CRTC starting from either the MSB
> > > > > or the LSB of the first display byte.  Which order depends on the
> > > > > display hardware, not on the CPU.
> > > > >
> > > > > > In C, we have only one way to address bits of a byte and that is with
> > > > > > arithmetic. You cannot take the address of a bit any other way, can you?
> > > > > >
> > > > > > Can we standardise on "bit n of a byte is addressed as (1 << n)"?
> > > > >
> > > > > BIT(n) in Linux works the same for little- and big-endian CPUs.
> > > > > But display hardware may use a different bit order.
> > > >
> > > > Perhaps some of this confusion could be avoided if you describe the
> > > > problem in terms of the sequence of scan-out of pixels, rather than in
> > > > terms of the serialization of bits. The significance of bits within each
> > > > pixel and the ordering of pixels within each memory word are independent,
> > > > right?
> > >
> > > Yes, that might help.
> >
> > Display:
> >
> >      P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15
> >
> >     P15 P14 P13 P12 P11 P10  P9  P8  P7  P6  P5  P4  P3  P2  P1  P0
>
> Hi Geert,
>
> does this mean the display hardware emits even rows from left to right
> and odd rows from right to left?

No, it means I should have my morning coffee first, and remove all
temporary cruft before pressing send :-(

The above paragraph should have read:

    Display (16 pixels):

        P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15

> I'm guessing P stands for "pixel".

Exactly.

> > Memory:
> >
> >   1 bpp (MSB first):
> >
> >               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
> >               ---- ---- ---- ---- ---- ---- ---- ----
> >       byte 0:   P0   P1   P2   P3   P4   P5   P6   P7
> >       byte 1:   P8   P9  P10  P11  P12  P13  P14  P15
> >
> >   1 bpp (LSB first):
> >
> >               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
> >               ---- ---- ---- ---- ---- ---- ---- ----
> >       byte 0:   P7   P6   P5   P4   P3   P2   P1   P0
> >       byte 1:  P15  P14  P13  P12  P11  P10   P9   P8
> >
> >   2 bpp (MSB first):
> >
> >               bits7-6 bits5-4 bits3-2 bits1-0
> >               ------- ------- ------- -------
> >       byte 0:    P0      P1      P2      P3
> >       byte 1:    P4      P5      P6      P7
> >       byte 2:    P8      P9     P10     P11
> >       byte 3:   P12     P13     P14     P15
> >
> >   2 bpp (LSB first):
> >
> >               bits7-6 bits5-4 bits3-2 bits1-0
> >               ------- ------- ------- -------
> >       byte 0:    P3      P2      P1      P0
> >       byte 1:    P7      P6      P5      P4
> >       byte 2:   P11     P10      P9      P8
> >       byte 3:   P15     P14     P13     P12
> >
> >   4 bpp (MSB first):
> >
> >               bits7-4 bits3-0
> >               ------- -------
> >       byte 0:    P0      P1
> >       byte 1:    P2      P3
> >       byte 2:    P4      P5
> >       byte 3:    P6      P7
> >       byte 4:    P8      P9
> >       byte 5:   P10     P11
> >       byte 6:   P12     P13
> >       byte 7:   P14     P15
> >
> >   4 bpp (LSB first):
> >
> >               bits7-4 bits3-0
> >               ------- -------
> >       byte 0:    P1      P0
> >       byte 1:    P3      P2
> >       byte 2:    P5      P4
> >       byte 3:    P7      P6
> >       byte 4:    P9      P8
> >       byte 5:   P11     P10
> >       byte 6:   P13     P12
> >       byte 7:   P15     P14
>
> I think I can guess what you meant there, and it looks understandable
> to me. These tables are actually very clear, and leave only one thing
> undefined: when multiple bits form a pixel, in which order do the bits
> form the value. I recall you said fbdev allows for both orderings but
> only one order is ever used if I understood right.

Indeed.  The third ordering is the ordering of the bits in a pixel.
As fb_bitfield.msb_right is always false, no hardware ever supported by
fbdev used the other ordering, so we only have to care about:

   1 bpp: P = [ bitN ]
   2 bpp: P = [ bitN bitN-1 ]
   4 bpp: P = [ bitN bitN-1 bitN-2 bitN-3 ]

> > > Also, when drm_fourcc.h is describing pixel formats, it needs to
> > > consider only how a little-endian CPU accesses them. That's how pixel
> > > data in memory is described. Display hardware plays no part in that.
> > > It is the driver's job to expose the pixel formats that match display
> > > hardware behaviour.
> >
> > But if the "CPU format" does not match the "display support",
> > all pixel data must be converted?
>
> Of course. If the driver author does not want to convert pixel data in
> flight, then the author should not let the driver expose a format that
> needs conversion.

... in which case we need a DRM fourcc code for the format?

BTW, Atari and Amiga use bitplanes for bpp <= 8, so they need
conversion anyway.

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 v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 10709 bytes --]

On Tue, 15 Mar 2022 09:57:23 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Pekka,
> 
> On Tue, Mar 15, 2022 at 9:46 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Tue, 15 Mar 2022 08:51:31 +0100
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > On Tue, Mar 15, 2022 at 8:33 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > > On Tue, 15 Mar 2022 09:15:08 +1100 (AEDT)
> > > > Finn Thain <fthain@linux-m68k.org> wrote:  
> > > > > On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:  
> > > > > > On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > > > > > On Mon, 14 Mar 2022 14:30:18 +0100
> > > > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > > > > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > > > > > > Introduce fourcc codes for color-indexed frame buffer formats with
> > > > > > > > > two, four, and sixteen colors, and provide a mapping from bit per
> > > > > > > > > pixel and depth to fourcc codes.
> > > > > > > > >
> > > > > > > > > As the number of bits per pixel is less than eight, these rely on
> > > > > > > > > proper block handling for the calculation of bits per pixel and
> > > > > > > > > pitch.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>  
> > > > > > > >  
> > > > > > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > > > > > @@ -99,7 +99,10 @@ extern "C" {
> > > > > > > > >  #define DRM_FORMAT_INVALID     0
> > > > > > > > >
> > > > > > > > >  /* color index */
> > > > > > > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > > > > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > > > > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > > > > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > > > > > > >
> > > > > > > > >  /* 8 bpp Red */
> > > > > > > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */  
> > > > > > > >
> > > > > > > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > > > > > > operations use native byte and bit ordering, unless
> > > > > > > > FBINFO_FOREIGN_ENDIAN is set.
> > > > > > > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > > > > > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > > > > > > (sh7760fb configures ordering to match host order).
> > > > > > > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > > > > > > guess they are broken on big-endian.
> > > > > > > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > > > > > > on little-endian.
> > > > > > > >
> > > > > > > > Hence the above should become:
> > > > > > > >
> > > > > > > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > > > > > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > > > > > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > > > > > > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > > > > > > [7:0] C1:C0 4:4 two pixels/byte */
> > > > > > > >
> > > > > > > > The same changes should be made for DRM_FORMAT_[RD][124].
> > > > > > > >
> > > > > > > > The fbdev emulation code should gain support for these with and without
> > > > > > > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > > > > > > >
> > > > > > > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > > > > > > [2] See p.30 of the VIDC datasheet
> > > > > > > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > > > > > > [3] See p.1178 of the SH7660 datasheet
> > > > > > > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf  
> > > > > > >
> > > > > > > why would CPU endianess affect the order of bits in a byte?  
> > > > > >
> > > > > > It doesn't, but see below.
> > > > > >  
> > > > > > > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > > > > > > bit 0 is (1 << 7)?  
> > > > > >
> > > > > > No, I mean that in case of multiple pixels per byte, the display
> > > > > > hardware pumps out pixels to the CRTC starting from either the MSB
> > > > > > or the LSB of the first display byte.  Which order depends on the
> > > > > > display hardware, not on the CPU.
> > > > > >  
> > > > > > > In C, we have only one way to address bits of a byte and that is with
> > > > > > > arithmetic. You cannot take the address of a bit any other way, can you?
> > > > > > >
> > > > > > > Can we standardise on "bit n of a byte is addressed as (1 << n)"?  
> > > > > >
> > > > > > BIT(n) in Linux works the same for little- and big-endian CPUs.
> > > > > > But display hardware may use a different bit order.  
> > > > >
> > > > > Perhaps some of this confusion could be avoided if you describe the
> > > > > problem in terms of the sequence of scan-out of pixels, rather than in
> > > > > terms of the serialization of bits. The significance of bits within each
> > > > > pixel and the ordering of pixels within each memory word are independent,
> > > > > right?  
> > > >
> > > > Yes, that might help.  
> > >
> > > Display:
> > >
> > >      P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15
> > >
> > >     P15 P14 P13 P12 P11 P10  P9  P8  P7  P6  P5  P4  P3  P2  P1  P0  
> >
> > Hi Geert,
> >
> > does this mean the display hardware emits even rows from left to right
> > and odd rows from right to left?  
> 
> No, it means I should have my morning coffee first, and remove all
> temporary cruft before pressing send :-(
> 
> The above paragraph should have read:
> 
>     Display (16 pixels):
> 
>         P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15
> 
> > I'm guessing P stands for "pixel".  
> 
> Exactly.
> 
> > > Memory:
> > >
> > >   1 bpp (MSB first):
> > >
> > >               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
> > >               ---- ---- ---- ---- ---- ---- ---- ----
> > >       byte 0:   P0   P1   P2   P3   P4   P5   P6   P7
> > >       byte 1:   P8   P9  P10  P11  P12  P13  P14  P15
> > >
> > >   1 bpp (LSB first):
> > >
> > >               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
> > >               ---- ---- ---- ---- ---- ---- ---- ----
> > >       byte 0:   P7   P6   P5   P4   P3   P2   P1   P0
> > >       byte 1:  P15  P14  P13  P12  P11  P10   P9   P8
> > >
> > >   2 bpp (MSB first):
> > >
> > >               bits7-6 bits5-4 bits3-2 bits1-0
> > >               ------- ------- ------- -------
> > >       byte 0:    P0      P1      P2      P3
> > >       byte 1:    P4      P5      P6      P7
> > >       byte 2:    P8      P9     P10     P11
> > >       byte 3:   P12     P13     P14     P15
> > >
> > >   2 bpp (LSB first):
> > >
> > >               bits7-6 bits5-4 bits3-2 bits1-0
> > >               ------- ------- ------- -------
> > >       byte 0:    P3      P2      P1      P0
> > >       byte 1:    P7      P6      P5      P4
> > >       byte 2:   P11     P10      P9      P8
> > >       byte 3:   P15     P14     P13     P12
> > >
> > >   4 bpp (MSB first):
> > >
> > >               bits7-4 bits3-0
> > >               ------- -------
> > >       byte 0:    P0      P1
> > >       byte 1:    P2      P3
> > >       byte 2:    P4      P5
> > >       byte 3:    P6      P7
> > >       byte 4:    P8      P9
> > >       byte 5:   P10     P11
> > >       byte 6:   P12     P13
> > >       byte 7:   P14     P15
> > >
> > >   4 bpp (LSB first):
> > >
> > >               bits7-4 bits3-0
> > >               ------- -------
> > >       byte 0:    P1      P0
> > >       byte 1:    P3      P2
> > >       byte 2:    P5      P4
> > >       byte 3:    P7      P6
> > >       byte 4:    P9      P8
> > >       byte 5:   P11     P10
> > >       byte 6:   P13     P12
> > >       byte 7:   P15     P14  
> >
> > I think I can guess what you meant there, and it looks understandable
> > to me. These tables are actually very clear, and leave only one thing
> > undefined: when multiple bits form a pixel, in which order do the bits
> > form the value. I recall you said fbdev allows for both orderings but
> > only one order is ever used if I understood right.  
> 
> Indeed.  The third ordering is the ordering of the bits in a pixel.
> As fb_bitfield.msb_right is always false, no hardware ever supported by
> fbdev used the other ordering, so we only have to care about:
> 
>    1 bpp: P = [ bitN ]
>    2 bpp: P = [ bitN bitN-1 ]
>    4 bpp: P = [ bitN bitN-1 bitN-2 bitN-3 ]

Excellent!

> > > > Also, when drm_fourcc.h is describing pixel formats, it needs to
> > > > consider only how a little-endian CPU accesses them. That's how pixel
> > > > data in memory is described. Display hardware plays no part in that.
> > > > It is the driver's job to expose the pixel formats that match display
> > > > hardware behaviour.  
> > >
> > > But if the "CPU format" does not match the "display support",
> > > all pixel data must be converted?  
> >
> > Of course. If the driver author does not want to convert pixel data in
> > flight, then the author should not let the driver expose a format that
> > needs conversion.  
> 
> ... in which case we need a DRM fourcc code for the format?

Yes. You can define any new formats you need as long as the format
definition does not depend on (is not affected/modified by) CPU
endianess or any other CPU or display hardware property. I believe this
is the convention used with drm_fourcc.

If the format wanted by display hardware depends on something, then you
need all relevant pixel formats defined and choose at build or driver
initialisation time which ones to expose.

> BTW, Atari and Amiga use bitplanes for bpp <= 8, so they need
> conversion anyway.

Right, that's probably the most reasonable approach. If you really
wanted to expose bitplanes, I could imagine that some new format
modifiers could achieve that.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 05/10] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 10709 bytes --]

On Tue, 15 Mar 2022 09:57:23 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Pekka,
> 
> On Tue, Mar 15, 2022 at 9:46 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Tue, 15 Mar 2022 08:51:31 +0100
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > On Tue, Mar 15, 2022 at 8:33 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > > On Tue, 15 Mar 2022 09:15:08 +1100 (AEDT)
> > > > Finn Thain <fthain@linux-m68k.org> wrote:  
> > > > > On Mon, 14 Mar 2022, Geert Uytterhoeven wrote:  
> > > > > > On Mon, Mar 14, 2022 at 4:05 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > > > > > On Mon, 14 Mar 2022 14:30:18 +0100
> > > > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > > > > > On Mon, Mar 7, 2022 at 9:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > > > > > > > Introduce fourcc codes for color-indexed frame buffer formats with
> > > > > > > > > two, four, and sixteen colors, and provide a mapping from bit per
> > > > > > > > > pixel and depth to fourcc codes.
> > > > > > > > >
> > > > > > > > > As the number of bits per pixel is less than eight, these rely on
> > > > > > > > > proper block handling for the calculation of bits per pixel and
> > > > > > > > > pitch.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>  
> > > > > > > >  
> > > > > > > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > > > > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > > > > > > @@ -99,7 +99,10 @@ extern "C" {
> > > > > > > > >  #define DRM_FORMAT_INVALID     0
> > > > > > > > >
> > > > > > > > >  /* color index */
> > > > > > > > > -#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > > > > > > > > +#define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > > > > +#define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
> > > > > > > > > +#define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
> > > > > > > > > +#define DRM_FORMAT_C8          fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
> > > > > > > > >
> > > > > > > > >  /* 8 bpp Red */
> > > > > > > > >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */  
> > > > > > > >
> > > > > > > > After replying to Ilia's comment[1], I realized the CFB drawing
> > > > > > > > operations use native byte and bit ordering, unless
> > > > > > > > FBINFO_FOREIGN_ENDIAN is set.
> > > > > > > > While Amiga, Atari, and Sun-3 use big-endian bit ordering,
> > > > > > > > e.g. Acorn VIDC[2] uses little endian, and SH7760[3] is configurable
> > > > > > > > (sh7760fb configures ordering to match host order).
> > > > > > > > BTW, ssd130{7fb,x}_update_rect() both assume little-endian, so I
> > > > > > > > guess they are broken on big-endian.
> > > > > > > > Fbtest uses big-endian bit ordering, so < 8 bpp is probably broken
> > > > > > > > on little-endian.
> > > > > > > >
> > > > > > > > Hence the above should become:
> > > > > > > >
> > > > > > > >     #define DRM_FORMAT_C1          fourcc_code('C', '1', ' ', ' ') /*
> > > > > > > > [7:0] C7:C6:C5:C4:C3:C2:C1:C0 1:1:1:1:1:1:1:1 eight pixels/byte */
> > > > > > > >     #define DRM_FORMAT_C2          fourcc_code('C', '2', ' ', ' ') /*
> > > > > > > > [7:0] C3:C2:C1:C0 2:2:2:2 four pixels/byte */
> > > > > > > >     #define DRM_FORMAT_C4          fourcc_code('C', '4', ' ', ' ') /*
> > > > > > > > [7:0] C1:C0 4:4 two pixels/byte */
> > > > > > > >
> > > > > > > > The same changes should be made for DRM_FORMAT_[RD][124].
> > > > > > > >
> > > > > > > > The fbdev emulation code should gain support for these with and without
> > > > > > > > DRM_FORMAT_BIG_ENDIAN, the latter perhaps only on big-endian platforms?
> > > > > > > >
> > > > > > > > [1] https://lore.kernel.org/r/CAKb7UvgEdm9U=+RyRwL0TGRfA_Qc7NbhCWoZOft2DKdXggtKYw@mail.gmail.com/
> > > > > > > > [2] See p.30 of the VIDC datasheet
> > > > > > > >     http://chrisacorns.computinghistory.org.uk/docs/Acorn/Misc/Acorn_VIDC_Datasheet.pdf
> > > > > > > > [3] See p.1178 of the SH7660 datasheet
> > > > > > > >     https://datasheet.octopart.com/HD6417760BL200AV-Renesas-datasheet-14105759.pdf  
> > > > > > >
> > > > > > > why would CPU endianess affect the order of bits in a byte?  
> > > > > >
> > > > > > It doesn't, but see below.
> > > > > >  
> > > > > > > Do you mean that bit 0 one machine is (1 << 0), and on another machine
> > > > > > > bit 0 is (1 << 7)?  
> > > > > >
> > > > > > No, I mean that in case of multiple pixels per byte, the display
> > > > > > hardware pumps out pixels to the CRTC starting from either the MSB
> > > > > > or the LSB of the first display byte.  Which order depends on the
> > > > > > display hardware, not on the CPU.
> > > > > >  
> > > > > > > In C, we have only one way to address bits of a byte and that is with
> > > > > > > arithmetic. You cannot take the address of a bit any other way, can you?
> > > > > > >
> > > > > > > Can we standardise on "bit n of a byte is addressed as (1 << n)"?  
> > > > > >
> > > > > > BIT(n) in Linux works the same for little- and big-endian CPUs.
> > > > > > But display hardware may use a different bit order.  
> > > > >
> > > > > Perhaps some of this confusion could be avoided if you describe the
> > > > > problem in terms of the sequence of scan-out of pixels, rather than in
> > > > > terms of the serialization of bits. The significance of bits within each
> > > > > pixel and the ordering of pixels within each memory word are independent,
> > > > > right?  
> > > >
> > > > Yes, that might help.  
> > >
> > > Display:
> > >
> > >      P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15
> > >
> > >     P15 P14 P13 P12 P11 P10  P9  P8  P7  P6  P5  P4  P3  P2  P1  P0  
> >
> > Hi Geert,
> >
> > does this mean the display hardware emits even rows from left to right
> > and odd rows from right to left?  
> 
> No, it means I should have my morning coffee first, and remove all
> temporary cruft before pressing send :-(
> 
> The above paragraph should have read:
> 
>     Display (16 pixels):
> 
>         P0  P1  P2  P3  P4  P5  P6  P7  P8  P9 P10 P11 P12 P13 P14 P15
> 
> > I'm guessing P stands for "pixel".  
> 
> Exactly.
> 
> > > Memory:
> > >
> > >   1 bpp (MSB first):
> > >
> > >               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
> > >               ---- ---- ---- ---- ---- ---- ---- ----
> > >       byte 0:   P0   P1   P2   P3   P4   P5   P6   P7
> > >       byte 1:   P8   P9  P10  P11  P12  P13  P14  P15
> > >
> > >   1 bpp (LSB first):
> > >
> > >               bit7 bit6 bit5 bit4 bit3 bit2 bit1 bit0
> > >               ---- ---- ---- ---- ---- ---- ---- ----
> > >       byte 0:   P7   P6   P5   P4   P3   P2   P1   P0
> > >       byte 1:  P15  P14  P13  P12  P11  P10   P9   P8
> > >
> > >   2 bpp (MSB first):
> > >
> > >               bits7-6 bits5-4 bits3-2 bits1-0
> > >               ------- ------- ------- -------
> > >       byte 0:    P0      P1      P2      P3
> > >       byte 1:    P4      P5      P6      P7
> > >       byte 2:    P8      P9     P10     P11
> > >       byte 3:   P12     P13     P14     P15
> > >
> > >   2 bpp (LSB first):
> > >
> > >               bits7-6 bits5-4 bits3-2 bits1-0
> > >               ------- ------- ------- -------
> > >       byte 0:    P3      P2      P1      P0
> > >       byte 1:    P7      P6      P5      P4
> > >       byte 2:   P11     P10      P9      P8
> > >       byte 3:   P15     P14     P13     P12
> > >
> > >   4 bpp (MSB first):
> > >
> > >               bits7-4 bits3-0
> > >               ------- -------
> > >       byte 0:    P0      P1
> > >       byte 1:    P2      P3
> > >       byte 2:    P4      P5
> > >       byte 3:    P6      P7
> > >       byte 4:    P8      P9
> > >       byte 5:   P10     P11
> > >       byte 6:   P12     P13
> > >       byte 7:   P14     P15
> > >
> > >   4 bpp (LSB first):
> > >
> > >               bits7-4 bits3-0
> > >               ------- -------
> > >       byte 0:    P1      P0
> > >       byte 1:    P3      P2
> > >       byte 2:    P5      P4
> > >       byte 3:    P7      P6
> > >       byte 4:    P9      P8
> > >       byte 5:   P11     P10
> > >       byte 6:   P13     P12
> > >       byte 7:   P15     P14  
> >
> > I think I can guess what you meant there, and it looks understandable
> > to me. These tables are actually very clear, and leave only one thing
> > undefined: when multiple bits form a pixel, in which order do the bits
> > form the value. I recall you said fbdev allows for both orderings but
> > only one order is ever used if I understood right.  
> 
> Indeed.  The third ordering is the ordering of the bits in a pixel.
> As fb_bitfield.msb_right is always false, no hardware ever supported by
> fbdev used the other ordering, so we only have to care about:
> 
>    1 bpp: P = [ bitN ]
>    2 bpp: P = [ bitN bitN-1 ]
>    4 bpp: P = [ bitN bitN-1 bitN-2 bitN-3 ]

Excellent!

> > > > Also, when drm_fourcc.h is describing pixel formats, it needs to
> > > > consider only how a little-endian CPU accesses them. That's how pixel
> > > > data in memory is described. Display hardware plays no part in that.
> > > > It is the driver's job to expose the pixel formats that match display
> > > > hardware behaviour.  
> > >
> > > But if the "CPU format" does not match the "display support",
> > > all pixel data must be converted?  
> >
> > Of course. If the driver author does not want to convert pixel data in
> > flight, then the author should not let the driver expose a format that
> > needs conversion.  
> 
> ... in which case we need a DRM fourcc code for the format?

Yes. You can define any new formats you need as long as the format
definition does not depend on (is not affected/modified by) CPU
endianess or any other CPU or display hardware property. I believe this
is the convention used with drm_fourcc.

If the format wanted by display hardware depends on something, then you
need all relevant pixel formats defined and choose at build or driver
initialisation time which ones to expose.

> BTW, Atari and Amiga use bitplanes for bpp <= 8, so they need
> conversion anyway.

Right, that's probably the most reasonable approach. If you really
wanted to expose bitplanes, I could imagine that some new format
modifiers could achieve that.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 06/10] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Add support for color-indexed frame buffer formats with two, four, and
sixteen colors to the DRM framebuffer helper functions:
  1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
  2. For color-indexed modes, the length of the color bitfields must be
     set to the color depth, else the logo code may pick a logo with too
     many colors.  Drop the incorrect DAC width comment, which
     originates from the i915 driver.
  3. Accept C[124] modes when validating or filling in struct
     fb_var_screeninfo, and use the correct number of bits per pixel.
  4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
     modes.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - Use drm_format_info_bpp() helper instead of deprecated .depth field
    or format-dependent calculations,
  - Use new .is_color_indexed field instead of checking against a list
    of formats.
---
 drivers/gpu/drm/drm_fb_helper.c | 101 ++++++++++++++++++++++++--------
 1 file changed, 75 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ed43b987d306afce..ba1c303a294d8c6f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -376,12 +376,31 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
 					   struct iosys_map *dst)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
-	unsigned int cpp = fb->format->cpp[0];
-	size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
-	void *src = fb_helper->fbdev->screen_buffer + offset;
-	size_t len = (clip->x2 - clip->x1) * cpp;
+	size_t offset = clip->y1 * fb->pitches[0];
+	size_t len = clip->x2 - clip->x1;
 	unsigned int y;
+	void *src;
 
+	switch (drm_format_info_bpp(fb->format, 0)) {
+	case 1:
+		offset += clip->x1 / 8;
+		len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
+		break;
+	case 2:
+		offset += clip->x1 / 4;
+		len = DIV_ROUND_UP(len + clip->x1 % 4, 4);
+		break;
+	case 4:
+		offset += clip->x1 / 2;
+		len = DIV_ROUND_UP(len + clip->x1 % 2, 2);
+		break;
+	default:
+		offset += clip->x1 * fb->format->cpp[0];
+		len *= fb->format->cpp[0];
+		break;
+	}
+
+	src = fb_helper->fbdev->screen_buffer + offset;
 	iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
 
 	for (y = clip->y1; y < clip->y2; y++) {
@@ -1231,19 +1250,23 @@ static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1,
 }
 
 static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
-					 u8 depth)
+					 const struct drm_format_info *format)
 {
-	switch (depth) {
-	case 8:
+	u8 depth = format->depth;
+
+	if (format->is_color_indexed) {
 		var->red.offset = 0;
 		var->green.offset = 0;
 		var->blue.offset = 0;
-		var->red.length = 8; /* 8bit DAC */
-		var->green.length = 8;
-		var->blue.length = 8;
+		var->red.length = depth;
+		var->green.length = depth;
+		var->blue.length = depth;
 		var->transp.offset = 0;
 		var->transp.length = 0;
-		break;
+		return;
+	}
+
+	switch (depth) {
 	case 15:
 		var->red.offset = 10;
 		var->green.offset = 5;
@@ -1298,7 +1321,9 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 {
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_framebuffer *fb = fb_helper->fb;
+	const struct drm_format_info *format = fb->format;
 	struct drm_device *dev = fb_helper->dev;
+	unsigned int bpp;
 
 	if (in_dbg_master())
 		return -EINVAL;
@@ -1308,22 +1333,33 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 		var->pixclock = 0;
 	}
 
-	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
-	    (drm_format_info_block_height(fb->format, 0) > 1))
-		return -EINVAL;
+	switch (format->format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+		/* supported format with sub-byte pixels */
+		break;
+
+	default:
+		if ((drm_format_info_block_width(format, 0) > 1) ||
+		    (drm_format_info_block_height(format, 0) > 1))
+			return -EINVAL;
+		break;
+	}
 
 	/*
 	 * Changes struct fb_var_screeninfo are currently not pushed back
 	 * to KMS, hence fail if different settings are requested.
 	 */
-	if (var->bits_per_pixel > fb->format->cpp[0] * 8 ||
+	bpp = drm_format_info_bpp(format, 0);
+	if (var->bits_per_pixel > bpp ||
 	    var->xres > fb->width || var->yres > fb->height ||
 	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
 		drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb "
 			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
 			  var->xres, var->yres, var->bits_per_pixel,
 			  var->xres_virtual, var->yres_virtual,
-			  fb->width, fb->height, fb->format->cpp[0] * 8);
+			  fb->width, fb->height, bpp);
 		return -EINVAL;
 	}
 
@@ -1338,13 +1374,13 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	    !var->blue.length    && !var->transp.length   &&
 	    !var->red.msb_right  && !var->green.msb_right &&
 	    !var->blue.msb_right && !var->transp.msb_right) {
-		drm_fb_helper_fill_pixel_fmt(var, fb->format->depth);
+		drm_fb_helper_fill_pixel_fmt(var, format);
 	}
 
 	/*
 	 * Likewise, bits_per_pixel should be rounded up to a supported value.
 	 */
-	var->bits_per_pixel = fb->format->cpp[0] * 8;
+	var->bits_per_pixel = bpp;
 
 	/*
 	 * drm fbdev emulation doesn't support changing the pixel format at all,
@@ -1680,11 +1716,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 }
 
 static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
-				   uint32_t depth)
+				   bool is_color_indexed)
 {
 	info->fix.type = FB_TYPE_PACKED_PIXELS;
-	info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
-		FB_VISUAL_TRUECOLOR;
+	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
+					    : info->fix.visual;
 	info->fix.mmio_start = 0;
 	info->fix.mmio_len = 0;
 	info->fix.type_aux = 0;
@@ -1701,19 +1737,31 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
 				   uint32_t fb_width, uint32_t fb_height)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
+	const struct drm_format_info *format = fb->format;
+
+	switch (format->format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+		/* supported format with sub-byte pixels */
+		break;
+
+	default:
+		WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
+			(drm_format_info_block_height(format, 0) > 1));
+		break;
+	}
 
-	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
-		(drm_format_info_block_height(fb->format, 0) > 1));
 	info->pseudo_palette = fb_helper->pseudo_palette;
 	info->var.xres_virtual = fb->width;
 	info->var.yres_virtual = fb->height;
-	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
+	info->var.bits_per_pixel = drm_format_info_bpp(format, 0);
 	info->var.accel_flags = FB_ACCELF_TEXT;
 	info->var.xoffset = 0;
 	info->var.yoffset = 0;
 	info->var.activate = FB_ACTIVATE_NOW;
 
-	drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth);
+	drm_fb_helper_fill_pixel_fmt(&info->var, format);
 
 	info->var.xres = fb_width;
 	info->var.yres = fb_height;
@@ -1738,7 +1786,8 @@ void drm_fb_helper_fill_info(struct fb_info *info,
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
 
-	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->depth);
+	drm_fb_helper_fill_fix(info, fb->pitches[0],
+			       fb->format->is_color_indexed);
 	drm_fb_helper_fill_var(info, fb_helper,
 			       sizes->fb_width, sizes->fb_height);
 
-- 
2.25.1

[PATCH v2 06/10] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Add support for color-indexed frame buffer formats with two, four, and
sixteen colors to the DRM framebuffer helper functions:
  1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
  2. For color-indexed modes, the length of the color bitfields must be
     set to the color depth, else the logo code may pick a logo with too
     many colors.  Drop the incorrect DAC width comment, which
     originates from the i915 driver.
  3. Accept C[124] modes when validating or filling in struct
     fb_var_screeninfo, and use the correct number of bits per pixel.
  4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
     modes.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - Use drm_format_info_bpp() helper instead of deprecated .depth field
    or format-dependent calculations,
  - Use new .is_color_indexed field instead of checking against a list
    of formats.
---
 drivers/gpu/drm/drm_fb_helper.c | 101 ++++++++++++++++++++++++--------
 1 file changed, 75 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ed43b987d306afce..ba1c303a294d8c6f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -376,12 +376,31 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
 					   struct iosys_map *dst)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
-	unsigned int cpp = fb->format->cpp[0];
-	size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
-	void *src = fb_helper->fbdev->screen_buffer + offset;
-	size_t len = (clip->x2 - clip->x1) * cpp;
+	size_t offset = clip->y1 * fb->pitches[0];
+	size_t len = clip->x2 - clip->x1;
 	unsigned int y;
+	void *src;
 
+	switch (drm_format_info_bpp(fb->format, 0)) {
+	case 1:
+		offset += clip->x1 / 8;
+		len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
+		break;
+	case 2:
+		offset += clip->x1 / 4;
+		len = DIV_ROUND_UP(len + clip->x1 % 4, 4);
+		break;
+	case 4:
+		offset += clip->x1 / 2;
+		len = DIV_ROUND_UP(len + clip->x1 % 2, 2);
+		break;
+	default:
+		offset += clip->x1 * fb->format->cpp[0];
+		len *= fb->format->cpp[0];
+		break;
+	}
+
+	src = fb_helper->fbdev->screen_buffer + offset;
 	iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
 
 	for (y = clip->y1; y < clip->y2; y++) {
@@ -1231,19 +1250,23 @@ static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1,
 }
 
 static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
-					 u8 depth)
+					 const struct drm_format_info *format)
 {
-	switch (depth) {
-	case 8:
+	u8 depth = format->depth;
+
+	if (format->is_color_indexed) {
 		var->red.offset = 0;
 		var->green.offset = 0;
 		var->blue.offset = 0;
-		var->red.length = 8; /* 8bit DAC */
-		var->green.length = 8;
-		var->blue.length = 8;
+		var->red.length = depth;
+		var->green.length = depth;
+		var->blue.length = depth;
 		var->transp.offset = 0;
 		var->transp.length = 0;
-		break;
+		return;
+	}
+
+	switch (depth) {
 	case 15:
 		var->red.offset = 10;
 		var->green.offset = 5;
@@ -1298,7 +1321,9 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 {
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_framebuffer *fb = fb_helper->fb;
+	const struct drm_format_info *format = fb->format;
 	struct drm_device *dev = fb_helper->dev;
+	unsigned int bpp;
 
 	if (in_dbg_master())
 		return -EINVAL;
@@ -1308,22 +1333,33 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 		var->pixclock = 0;
 	}
 
-	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
-	    (drm_format_info_block_height(fb->format, 0) > 1))
-		return -EINVAL;
+	switch (format->format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+		/* supported format with sub-byte pixels */
+		break;
+
+	default:
+		if ((drm_format_info_block_width(format, 0) > 1) ||
+		    (drm_format_info_block_height(format, 0) > 1))
+			return -EINVAL;
+		break;
+	}
 
 	/*
 	 * Changes struct fb_var_screeninfo are currently not pushed back
 	 * to KMS, hence fail if different settings are requested.
 	 */
-	if (var->bits_per_pixel > fb->format->cpp[0] * 8 ||
+	bpp = drm_format_info_bpp(format, 0);
+	if (var->bits_per_pixel > bpp ||
 	    var->xres > fb->width || var->yres > fb->height ||
 	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
 		drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb "
 			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
 			  var->xres, var->yres, var->bits_per_pixel,
 			  var->xres_virtual, var->yres_virtual,
-			  fb->width, fb->height, fb->format->cpp[0] * 8);
+			  fb->width, fb->height, bpp);
 		return -EINVAL;
 	}
 
@@ -1338,13 +1374,13 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	    !var->blue.length    && !var->transp.length   &&
 	    !var->red.msb_right  && !var->green.msb_right &&
 	    !var->blue.msb_right && !var->transp.msb_right) {
-		drm_fb_helper_fill_pixel_fmt(var, fb->format->depth);
+		drm_fb_helper_fill_pixel_fmt(var, format);
 	}
 
 	/*
 	 * Likewise, bits_per_pixel should be rounded up to a supported value.
 	 */
-	var->bits_per_pixel = fb->format->cpp[0] * 8;
+	var->bits_per_pixel = bpp;
 
 	/*
 	 * drm fbdev emulation doesn't support changing the pixel format at all,
@@ -1680,11 +1716,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 }
 
 static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
-				   uint32_t depth)
+				   bool is_color_indexed)
 {
 	info->fix.type = FB_TYPE_PACKED_PIXELS;
-	info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
-		FB_VISUAL_TRUECOLOR;
+	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
+					    : info->fix.visual;
 	info->fix.mmio_start = 0;
 	info->fix.mmio_len = 0;
 	info->fix.type_aux = 0;
@@ -1701,19 +1737,31 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
 				   uint32_t fb_width, uint32_t fb_height)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
+	const struct drm_format_info *format = fb->format;
+
+	switch (format->format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+		/* supported format with sub-byte pixels */
+		break;
+
+	default:
+		WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
+			(drm_format_info_block_height(format, 0) > 1));
+		break;
+	}
 
-	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
-		(drm_format_info_block_height(fb->format, 0) > 1));
 	info->pseudo_palette = fb_helper->pseudo_palette;
 	info->var.xres_virtual = fb->width;
 	info->var.yres_virtual = fb->height;
-	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
+	info->var.bits_per_pixel = drm_format_info_bpp(format, 0);
 	info->var.accel_flags = FB_ACCELF_TEXT;
 	info->var.xoffset = 0;
 	info->var.yoffset = 0;
 	info->var.activate = FB_ACTIVATE_NOW;
 
-	drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth);
+	drm_fb_helper_fill_pixel_fmt(&info->var, format);
 
 	info->var.xres = fb_width;
 	info->var.yres = fb_height;
@@ -1738,7 +1786,8 @@ void drm_fb_helper_fill_info(struct fb_info *info,
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
 
-	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->depth);
+	drm_fb_helper_fill_fix(info, fb->pitches[0],
+			       fb->format->is_color_indexed);
 	drm_fb_helper_fill_var(info, fb_helper,
 			       sizes->fb_width, sizes->fb_height);
 
-- 
2.25.1

Re: [PATCH v2 06/10] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Add support for color-indexed frame buffer formats with two, four, and
> sixteen colors to the DRM framebuffer helper functions:
>   1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
>   2. For color-indexed modes, the length of the color bitfields must be
>      set to the color depth, else the logo code may pick a logo with too
>      many colors.  Drop the incorrect DAC width comment, which
>      originates from the i915 driver.
>   3. Accept C[124] modes when validating or filling in struct
>      fb_var_screeninfo, and use the correct number of bits per pixel.
>   4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
>      modes.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

[snip]

>  static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> -				   uint32_t depth)
> +				   bool is_color_indexed)
>  {
>  	info->fix.type = FB_TYPE_PACKED_PIXELS;
> -	info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> -		FB_VISUAL_TRUECOLOR;
> +	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
> +					    : info->fix.visual;

Shouldn't this be the following instead ?

  +	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
  +					    : FB_VISUAL_TRUECOLOR;

Other than that the patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 06/10] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Add support for color-indexed frame buffer formats with two, four, and
> sixteen colors to the DRM framebuffer helper functions:
>   1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
>   2. For color-indexed modes, the length of the color bitfields must be
>      set to the color depth, else the logo code may pick a logo with too
>      many colors.  Drop the incorrect DAC width comment, which
>      originates from the i915 driver.
>   3. Accept C[124] modes when validating or filling in struct
>      fb_var_screeninfo, and use the correct number of bits per pixel.
>   4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
>      modes.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

[snip]

>  static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> -				   uint32_t depth)
> +				   bool is_color_indexed)
>  {
>  	info->fix.type = FB_TYPE_PACKED_PIXELS;
> -	info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> -		FB_VISUAL_TRUECOLOR;
> +	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
> +					    : info->fix.visual;

Shouldn't this be the following instead ?

  +	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
  +					    : FB_VISUAL_TRUECOLOR;

Other than that the patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 06/10] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Javier,

On Wed, Mar 9, 2022 at 2:10 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 3/7/22 21:52, Geert Uytterhoeven wrote:
> > Add support for color-indexed frame buffer formats with two, four, and
> > sixteen colors to the DRM framebuffer helper functions:
> >   1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
> >   2. For color-indexed modes, the length of the color bitfields must be
> >      set to the color depth, else the logo code may pick a logo with too
> >      many colors.  Drop the incorrect DAC width comment, which
> >      originates from the i915 driver.
> >   3. Accept C[124] modes when validating or filling in struct
> >      fb_var_screeninfo, and use the correct number of bits per pixel.
> >   4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
> >      modes.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> [snip]
>
> >  static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> > -                                uint32_t depth)
> > +                                bool is_color_indexed)
> >  {
> >       info->fix.type = FB_TYPE_PACKED_PIXELS;
> > -     info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> > -             FB_VISUAL_TRUECOLOR;
> > +     info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
> > +                                         : info->fix.visual;
>
> Shouldn't this be the following instead ?
>
>   +     info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
>   +                                         : FB_VISUAL_TRUECOLOR;

Indeed. Will fix in v3.
Thanks!

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 v2 06/10] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Javier,

On Wed, Mar 9, 2022 at 2:10 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 3/7/22 21:52, Geert Uytterhoeven wrote:
> > Add support for color-indexed frame buffer formats with two, four, and
> > sixteen colors to the DRM framebuffer helper functions:
> >   1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
> >   2. For color-indexed modes, the length of the color bitfields must be
> >      set to the color depth, else the logo code may pick a logo with too
> >      many colors.  Drop the incorrect DAC width comment, which
> >      originates from the i915 driver.
> >   3. Accept C[124] modes when validating or filling in struct
> >      fb_var_screeninfo, and use the correct number of bits per pixel.
> >   4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
> >      modes.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> [snip]
>
> >  static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> > -                                uint32_t depth)
> > +                                bool is_color_indexed)
> >  {
> >       info->fix.type = FB_TYPE_PACKED_PIXELS;
> > -     info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> > -             FB_VISUAL_TRUECOLOR;
> > +     info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
> > +                                         : info->fix.visual;
>
> Shouldn't this be the following instead ?
>
>   +     info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
>   +                                         : FB_VISUAL_TRUECOLOR;

Indeed. Will fix in v3.
Thanks!

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

[PATCH v2 RFC 07/10] drm/gem-fb-helper: Use actual bpp for size calculations

[Geert Uytterhoeven]

The AFBC helpers derive the number of bits per pixel from the deprecated
drm_format_info.cpp[] field, which does not take into account block
sizes.

Fix this by using the actual number of bits per pixel instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
RFC, as this code path was untested.

v2:
  - Replace FIXME by TODO comment.
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index 746fd8c738451247..e5b8443378d2294b 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -492,6 +492,8 @@ void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_directi
 }
 EXPORT_SYMBOL(drm_gem_fb_end_cpu_access);
 
+// TODO Drop this function and replace by drm_format_info_bpp() once all
+// DRM_FORMAT_* provide proper block info in drivers/gpu/drm/drm_fourcc.c
 static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
 				  const struct drm_mode_fb_cmd2 *mode_cmd)
 {
@@ -499,11 +501,6 @@ static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
 
 	info = drm_get_format_info(dev, mode_cmd);
 
-	/* use whatever a driver has set */
-	if (info->cpp[0])
-		return info->cpp[0] * 8;
-
-	/* guess otherwise */
 	switch (info->format) {
 	case DRM_FORMAT_YUV420_8BIT:
 		return 12;
@@ -512,11 +509,8 @@ static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
 	case DRM_FORMAT_VUY101010:
 		return 30;
 	default:
-		break;
+		return drm_format_info_bpp(info, 0);
 	}
-
-	/* all attempts failed */
-	return 0;
 }
 
 static int drm_gem_afbc_min_size(struct drm_device *dev,
-- 
2.25.1

[PATCH v2 RFC 07/10] drm/gem-fb-helper: Use actual bpp for size calculations

[Geert Uytterhoeven]

The AFBC helpers derive the number of bits per pixel from the deprecated
drm_format_info.cpp[] field, which does not take into account block
sizes.

Fix this by using the actual number of bits per pixel instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
RFC, as this code path was untested.

v2:
  - Replace FIXME by TODO comment.
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index 746fd8c738451247..e5b8443378d2294b 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -492,6 +492,8 @@ void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_directi
 }
 EXPORT_SYMBOL(drm_gem_fb_end_cpu_access);
 
+// TODO Drop this function and replace by drm_format_info_bpp() once all
+// DRM_FORMAT_* provide proper block info in drivers/gpu/drm/drm_fourcc.c
 static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
 				  const struct drm_mode_fb_cmd2 *mode_cmd)
 {
@@ -499,11 +501,6 @@ static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
 
 	info = drm_get_format_info(dev, mode_cmd);
 
-	/* use whatever a driver has set */
-	if (info->cpp[0])
-		return info->cpp[0] * 8;
-
-	/* guess otherwise */
 	switch (info->format) {
 	case DRM_FORMAT_YUV420_8BIT:
 		return 12;
@@ -512,11 +509,8 @@ static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
 	case DRM_FORMAT_VUY101010:
 		return 30;
 	default:
-		break;
+		return drm_format_info_bpp(info, 0);
 	}
-
-	/* all attempts failed */
-	return 0;
 }
 
 static int drm_gem_afbc_min_size(struct drm_device *dev,
-- 
2.25.1

Re: [PATCH v2 RFC 07/10] drm/gem-fb-helper: Use actual bpp for size calculations

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> The AFBC helpers derive the number of bits per pixel from the deprecated
> drm_format_info.cpp[] field, which does not take into account block
> sizes.
> 
> Fix this by using the actual number of bits per pixel instead.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> RFC, as this code path was untested.
>

Looks good to me but haven't tested either.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 RFC 07/10] drm/gem-fb-helper: Use actual bpp for size calculations

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> The AFBC helpers derive the number of bits per pixel from the deprecated
> drm_format_info.cpp[] field, which does not take into account block
> sizes.
> 
> Fix this by using the actual number of bits per pixel instead.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> RFC, as this code path was untested.
>

Looks good to me but haven't tested either.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 RFC 08/10] drm/fourcc: Document that single-channel "red" can be any color

[Geert Uytterhoeven]

Traditionally, the first channel has been called the "red" channel, but
the fourcc values for single-channel "red" formats can also be used for
other light-on-dark displays, like grayscale.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
RFC, as I have no immediate need for these formats.

v2:
  - New.
---
 include/uapi/drm/drm_fourcc.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 457ed39cc48f08e1..f0187cf20e4619d2 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,16 +104,16 @@ extern "C" {
 #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
 
-/* 8 bpp Red */
+/* 8 bpp Red (or generic light-on-dark) */
 #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
 
-/* 10 bpp Red */
+/* 10 bpp Red (or generic light-on-dark) */
 #define DRM_FORMAT_R10		fourcc_code('R', '1', '0', ' ') /* [15:0] x:R 6:10 little endian */
 
-/* 12 bpp Red */
+/* 12 bpp Red (or generic light-on-dark) */
 #define DRM_FORMAT_R12		fourcc_code('R', '1', '2', ' ') /* [15:0] x:R 4:12 little endian */
 
-/* 16 bpp Red */
+/* 16 bpp Red (or generic light-on-dark) */
 #define DRM_FORMAT_R16		fourcc_code('R', '1', '6', ' ') /* [15:0] R little endian */
 
 /* 16 bpp RG */
-- 
2.25.1

[PATCH v2 RFC 08/10] drm/fourcc: Document that single-channel "red" can be any color

[Geert Uytterhoeven]

Traditionally, the first channel has been called the "red" channel, but
the fourcc values for single-channel "red" formats can also be used for
other light-on-dark displays, like grayscale.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
RFC, as I have no immediate need for these formats.

v2:
  - New.
---
 include/uapi/drm/drm_fourcc.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 457ed39cc48f08e1..f0187cf20e4619d2 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,16 +104,16 @@ extern "C" {
 #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
 
-/* 8 bpp Red */
+/* 8 bpp Red (or generic light-on-dark) */
 #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
 
-/* 10 bpp Red */
+/* 10 bpp Red (or generic light-on-dark) */
 #define DRM_FORMAT_R10		fourcc_code('R', '1', '0', ' ') /* [15:0] x:R 6:10 little endian */
 
-/* 12 bpp Red */
+/* 12 bpp Red (or generic light-on-dark) */
 #define DRM_FORMAT_R12		fourcc_code('R', '1', '2', ' ') /* [15:0] x:R 4:12 little endian */
 
-/* 16 bpp Red */
+/* 16 bpp Red (or generic light-on-dark) */
 #define DRM_FORMAT_R16		fourcc_code('R', '1', '6', ' ') /* [15:0] R little endian */
 
 /* 16 bpp RG */
-- 
2.25.1

Re: [PATCH v2 RFC 08/10] drm/fourcc: Document that single-channel "red" can be any color

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2295 bytes --]

On Mon,  7 Mar 2022 21:52:43 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Traditionally, the first channel has been called the "red" channel, but
> the fourcc values for single-channel "red" formats can also be used for
> other light-on-dark displays, like grayscale.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> RFC, as I have no immediate need for these formats.
> 
> v2:
>   - New.
> ---
>  include/uapi/drm/drm_fourcc.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 457ed39cc48f08e1..f0187cf20e4619d2 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -104,16 +104,16 @@ extern "C" {
>  #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
>  #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
>  
> -/* 8 bpp Red */
> +/* 8 bpp Red (or generic light-on-dark) */
>  #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
>  
> -/* 10 bpp Red */
> +/* 10 bpp Red (or generic light-on-dark) */
>  #define DRM_FORMAT_R10		fourcc_code('R', '1', '0', ' ') /* [15:0] x:R 6:10 little endian */
>  
> -/* 12 bpp Red */
> +/* 12 bpp Red (or generic light-on-dark) */
>  #define DRM_FORMAT_R12		fourcc_code('R', '1', '2', ' ') /* [15:0] x:R 4:12 little endian */
>  
> -/* 16 bpp Red */
> +/* 16 bpp Red (or generic light-on-dark) */
>  #define DRM_FORMAT_R16		fourcc_code('R', '1', '6', ' ') /* [15:0] R little endian */
>  
>  /* 16 bpp RG */

Hi Geert,

this is a good idea. I just wonder whether light-on-dark is an accurate
description. To me, light-on-dark means things like bright characters
on a dark background, but that is a property of the image as a whole and
not a property of the pixel format. What I think you are after here is
that increasing channel value mean increasing brightness.

How to word that concisely...

Direct relationship to brightness (vs. inverse relationship to brightness)?

This does not imply a linear relationship, and I'd also use
"brightness" to denote that it is a qualitative factor relating to
human observation.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 RFC 08/10] drm/fourcc: Document that single-channel "red" can be any color

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2295 bytes --]

On Mon,  7 Mar 2022 21:52:43 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Traditionally, the first channel has been called the "red" channel, but
> the fourcc values for single-channel "red" formats can also be used for
> other light-on-dark displays, like grayscale.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> RFC, as I have no immediate need for these formats.
> 
> v2:
>   - New.
> ---
>  include/uapi/drm/drm_fourcc.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 457ed39cc48f08e1..f0187cf20e4619d2 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -104,16 +104,16 @@ extern "C" {
>  #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
>  #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
>  
> -/* 8 bpp Red */
> +/* 8 bpp Red (or generic light-on-dark) */
>  #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
>  
> -/* 10 bpp Red */
> +/* 10 bpp Red (or generic light-on-dark) */
>  #define DRM_FORMAT_R10		fourcc_code('R', '1', '0', ' ') /* [15:0] x:R 6:10 little endian */
>  
> -/* 12 bpp Red */
> +/* 12 bpp Red (or generic light-on-dark) */
>  #define DRM_FORMAT_R12		fourcc_code('R', '1', '2', ' ') /* [15:0] x:R 4:12 little endian */
>  
> -/* 16 bpp Red */
> +/* 16 bpp Red (or generic light-on-dark) */
>  #define DRM_FORMAT_R16		fourcc_code('R', '1', '6', ' ') /* [15:0] R little endian */
>  
>  /* 16 bpp RG */

Hi Geert,

this is a good idea. I just wonder whether light-on-dark is an accurate
description. To me, light-on-dark means things like bright characters
on a dark background, but that is a property of the image as a whole and
not a property of the pixel format. What I think you are after here is
that increasing channel value mean increasing brightness.

How to word that concisely...

Direct relationship to brightness (vs. inverse relationship to brightness)?

This does not imply a linear relationship, and I'd also use
"brightness" to denote that it is a qualitative factor relating to
human observation.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 RFC 08/10] drm/fourcc: Document that single-channel "red" can be any color

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Traditionally, the first channel has been called the "red" channel, but
> the fourcc values for single-channel "red" formats can also be used for
> other light-on-dark displays, like grayscale.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Yes, I learned that "Red" actually meant just a color channel
that may not be red in one of the thread about fourcc formats.

So I agree that would be good to have a comment about this.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 RFC 08/10] drm/fourcc: Document that single-channel "red" can be any color

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Traditionally, the first channel has been called the "red" channel, but
> the fourcc values for single-channel "red" formats can also be used for
> other light-on-dark displays, like grayscale.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Yes, I learned that "Red" actually meant just a color channel
that may not be red in one of the thread about fourcc formats.

So I agree that would be good to have a comment about this.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 RFC 09/10] drm/fourcc: Add DRM_FORMAT_R[124]

[Geert Uytterhoeven]

Introduce fourcc codes for single-channel light-on-dark frame buffer
formats with two, four, and sixteen intensity levels.

As the number of bits per pixel is less than eight, these rely on proper
block handling for the calculation of bits per pixel and pitch.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
RFC, as I have no immediate need for these formats.

v2:
  - Improve pixel descriptions.
---
 drivers/gpu/drm/drm_fourcc.c  |  6 ++++++
 include/uapi/drm/drm_fourcc.h | 11 ++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 29f4fe199c4ddcf0..05e65e9ab0c69c6a 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -154,6 +154,12 @@ const struct drm_format_info *__drm_format_info(u32 format)
 		{ .format = DRM_FORMAT_C4,		.depth = 4,  .num_planes = 1,
 		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
 		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+		{ .format = DRM_FORMAT_R1,		.depth = 1,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_R2,		.depth = 2,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_R4,		.depth = 4,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R10,		.depth = 10, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R12,		.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index f0187cf20e4619d2..def0a73620dc86d2 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,8 +104,17 @@ extern "C" {
 #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
 
+/* 1 bpp Red (or monochrome light-on-dark) */
+#define DRM_FORMAT_R1		fourcc_code('R', '1', ' ', ' ') /* [7:0] R0:R1:R2:R3:R4:R5:R6:R7 1:1:1:1:1:1:1:1 eight pixels/byte */
+
+/* 2 bpp Red (or generic light-on-dark) */
+#define DRM_FORMAT_R2		fourcc_code('R', '2', ' ', ' ') /* [7:0] R0:R1:R2:R3 2:2:2:2 four pixels/byte */
+
+/* 4 bpp Red (or generic light-on-dark) */
+#define DRM_FORMAT_R4		fourcc_code('R', '4', ' ', ' ') /* [7:0] R0:R1 4:4 two pixels/byte */
+
 /* 8 bpp Red (or generic light-on-dark) */
-#define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
+#define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R 8 one pixel/byte */
 
 /* 10 bpp Red (or generic light-on-dark) */
 #define DRM_FORMAT_R10		fourcc_code('R', '1', '0', ' ') /* [15:0] x:R 6:10 little endian */
-- 
2.25.1

[PATCH v2 RFC 09/10] drm/fourcc: Add DRM_FORMAT_R[124]

[Geert Uytterhoeven]

Introduce fourcc codes for single-channel light-on-dark frame buffer
formats with two, four, and sixteen intensity levels.

As the number of bits per pixel is less than eight, these rely on proper
block handling for the calculation of bits per pixel and pitch.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
RFC, as I have no immediate need for these formats.

v2:
  - Improve pixel descriptions.
---
 drivers/gpu/drm/drm_fourcc.c  |  6 ++++++
 include/uapi/drm/drm_fourcc.h | 11 ++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 29f4fe199c4ddcf0..05e65e9ab0c69c6a 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -154,6 +154,12 @@ const struct drm_format_info *__drm_format_info(u32 format)
 		{ .format = DRM_FORMAT_C4,		.depth = 4,  .num_planes = 1,
 		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
 		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+		{ .format = DRM_FORMAT_R1,		.depth = 1,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_R2,		.depth = 2,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_R4,		.depth = 4,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R10,		.depth = 10, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R12,		.depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index f0187cf20e4619d2..def0a73620dc86d2 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,8 +104,17 @@ extern "C" {
 #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
 
+/* 1 bpp Red (or monochrome light-on-dark) */
+#define DRM_FORMAT_R1		fourcc_code('R', '1', ' ', ' ') /* [7:0] R0:R1:R2:R3:R4:R5:R6:R7 1:1:1:1:1:1:1:1 eight pixels/byte */
+
+/* 2 bpp Red (or generic light-on-dark) */
+#define DRM_FORMAT_R2		fourcc_code('R', '2', ' ', ' ') /* [7:0] R0:R1:R2:R3 2:2:2:2 four pixels/byte */
+
+/* 4 bpp Red (or generic light-on-dark) */
+#define DRM_FORMAT_R4		fourcc_code('R', '4', ' ', ' ') /* [7:0] R0:R1 4:4 two pixels/byte */
+
 /* 8 bpp Red (or generic light-on-dark) */
-#define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
+#define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R 8 one pixel/byte */
 
 /* 10 bpp Red (or generic light-on-dark) */
 #define DRM_FORMAT_R10		fourcc_code('R', '1', '0', ' ') /* [15:0] x:R 6:10 little endian */
-- 
2.25.1

Re: [PATCH v2 RFC 09/10] drm/fourcc: Add DRM_FORMAT_R[124]

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Introduce fourcc codes for single-channel light-on-dark frame buffer
> formats with two, four, and sixteen intensity levels.
> 
> As the number of bits per pixel is less than eight, these rely on proper
> block handling for the calculation of bits per pixel and pitch.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

IIRC the agreement was that it was worth to have both Cx and Rx fourcc
formats separately, so this patch makes sense to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 RFC 09/10] drm/fourcc: Add DRM_FORMAT_R[124]

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> Introduce fourcc codes for single-channel light-on-dark frame buffer
> formats with two, four, and sixteen intensity levels.
> 
> As the number of bits per pixel is less than eight, these rely on proper
> block handling for the calculation of bits per pixel and pitch.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

IIRC the agreement was that it was worth to have both Cx and Rx fourcc
formats separately, so this patch makes sense to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 RFC 10/10] drm/fourcc: Add DRM_FORMAT_D[1248]

[Geert Uytterhoeven]

As Rn is light-on-dark, and Cn can be any colors, there are currently
no fourcc codes to describe dark-on-light displays.

Introduce fourcc codes for a single-channel dark-on-light frame buffer
format with two, four, sixteen, or 256 darkness levels.

As the number of bits per pixel may be less than eight, some of these
formats rely on proper block handling for the calculation of bits per
pixel and pitch.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
RFC, as I have no immediate need for these formats.

v2:
  - Add rationale for adding new formats,
  - Improve pixel descriptions,
  - Add D[248] for completeness.
---
 drivers/gpu/drm/drm_fourcc.c  |  7 +++++++
 include/uapi/drm/drm_fourcc.h | 12 ++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 05e65e9ab0c69c6a..e09331bb3bc73f21 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -154,6 +154,13 @@ const struct drm_format_info *__drm_format_info(u32 format)
 		{ .format = DRM_FORMAT_C4,		.depth = 4,  .num_planes = 1,
 		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
 		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+		{ .format = DRM_FORMAT_D1,		.depth = 1,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_D2,		.depth = 2,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_D4,		.depth = 4,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_D8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R1,		.depth = 1,  .num_planes = 1,
 		  .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R2,		.depth = 2,  .num_planes = 1,
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index def0a73620dc86d2..1d8d82b1742898c0 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,6 +104,18 @@ extern "C" {
 #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
 
+/* 1 bpp Darkness (monochrome dark-on-light) */
+#define DRM_FORMAT_D1		fourcc_code('D', '1', ' ', ' ') /* [7:0] D0:D1:D2:D3:D4:D5:D6:D7 1:1:1:1:1:1:1:1 eight pixels/byte */
+
+/* 2 bpp Darkness (generic dark-on-light) */
+#define DRM_FORMAT_D2		fourcc_code('D', '2', ' ', ' ') /* [7:0] D0:D1:D2:D3 2:2:2:2 four pixels/byte */
+
+/* 4 bpp Darkness (generic dark-on-light) */
+#define DRM_FORMAT_D4		fourcc_code('D', '4', ' ', ' ') /* [7:0] D0:D1 4:4 two pixels/byte */
+
+/* 8 bpp Darkness (generic dark-on-light) */
+#define DRM_FORMAT_D8		fourcc_code('D', '8', ' ', ' ') /* [7:0] D 8 one pixel/byte */
+
 /* 1 bpp Red (or monochrome light-on-dark) */
 #define DRM_FORMAT_R1		fourcc_code('R', '1', ' ', ' ') /* [7:0] R0:R1:R2:R3:R4:R5:R6:R7 1:1:1:1:1:1:1:1 eight pixels/byte */
 
-- 
2.25.1

[PATCH v2 RFC 10/10] drm/fourcc: Add DRM_FORMAT_D[1248]

[Geert Uytterhoeven]

As Rn is light-on-dark, and Cn can be any colors, there are currently
no fourcc codes to describe dark-on-light displays.

Introduce fourcc codes for a single-channel dark-on-light frame buffer
format with two, four, sixteen, or 256 darkness levels.

As the number of bits per pixel may be less than eight, some of these
formats rely on proper block handling for the calculation of bits per
pixel and pitch.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
RFC, as I have no immediate need for these formats.

v2:
  - Add rationale for adding new formats,
  - Improve pixel descriptions,
  - Add D[248] for completeness.
---
 drivers/gpu/drm/drm_fourcc.c  |  7 +++++++
 include/uapi/drm/drm_fourcc.h | 12 ++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 05e65e9ab0c69c6a..e09331bb3bc73f21 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -154,6 +154,13 @@ const struct drm_format_info *__drm_format_info(u32 format)
 		{ .format = DRM_FORMAT_C4,		.depth = 4,  .num_planes = 1,
 		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
 		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+		{ .format = DRM_FORMAT_D1,		.depth = 1,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_D2,		.depth = 2,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_D4,		.depth = 4,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_D8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R1,		.depth = 1,  .num_planes = 1,
 		  .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_R2,		.depth = 2,  .num_planes = 1,
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index def0a73620dc86d2..1d8d82b1742898c0 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,6 +104,18 @@ extern "C" {
 #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
 
+/* 1 bpp Darkness (monochrome dark-on-light) */
+#define DRM_FORMAT_D1		fourcc_code('D', '1', ' ', ' ') /* [7:0] D0:D1:D2:D3:D4:D5:D6:D7 1:1:1:1:1:1:1:1 eight pixels/byte */
+
+/* 2 bpp Darkness (generic dark-on-light) */
+#define DRM_FORMAT_D2		fourcc_code('D', '2', ' ', ' ') /* [7:0] D0:D1:D2:D3 2:2:2:2 four pixels/byte */
+
+/* 4 bpp Darkness (generic dark-on-light) */
+#define DRM_FORMAT_D4		fourcc_code('D', '4', ' ', ' ') /* [7:0] D0:D1 4:4 two pixels/byte */
+
+/* 8 bpp Darkness (generic dark-on-light) */
+#define DRM_FORMAT_D8		fourcc_code('D', '8', ' ', ' ') /* [7:0] D 8 one pixel/byte */
+
 /* 1 bpp Red (or monochrome light-on-dark) */
 #define DRM_FORMAT_R1		fourcc_code('R', '1', ' ', ' ') /* [7:0] R0:R1:R2:R3:R4:R5:R6:R7 1:1:1:1:1:1:1:1 eight pixels/byte */
 
-- 
2.25.1

Re: [PATCH v2 RFC 10/10] drm/fourcc: Add DRM_FORMAT_D[1248]

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> As Rn is light-on-dark, and Cn can be any colors, there are currently
> no fourcc codes to describe dark-on-light displays.
> 
> Introduce fourcc codes for a single-channel dark-on-light frame buffer
> format with two, four, sixteen, or 256 darkness levels.
> 
> As the number of bits per pixel may be less than eight, some of these
> formats rely on proper block handling for the calculation of bits per
> pixel and pitch.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 RFC 10/10] drm/fourcc: Add DRM_FORMAT_D[1248]

[Javier Martinez Canillas]

On 3/7/22 21:52, Geert Uytterhoeven wrote:
> As Rn is light-on-dark, and Cn can be any colors, there are currently
> no fourcc codes to describe dark-on-light displays.
> 
> Introduce fourcc codes for a single-channel dark-on-light frame buffer
> format with two, four, sixteen, or 256 darkness levels.
> 
> As the number of bits per pixel may be less than eight, some of these
> formats rely on proper block handling for the calculation of bits per
> pixel and pitch.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat