[PATCH 0/8] drm: Add support for low-color frame buffer formats

[PATCH 0/8] 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 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, with text console operation and
fbtest.

Overview:
  - Patches 1 and 2 give a working system, albeit with a too large pitch
    (line length),
  - Patches 3 and 4 reduce memory consumption by correcting the pitch
    in case bpp < 8,
  - Patches 5 and 6 are untested, but may become useful with DRM
    userspace,
  - Patches 7 and 8 add more fourcc codes for grayscale and monochrome
    frame buffer formats, which may be useful for e.g. the ssd130x and
    repaper drivers.

Notes:
  - I haven't looked yet into making modetest draw a correct image.
  - 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!

Geert Uytterhoeven (8):
  drm/fourcc: Add DRM_FORMAT_C[124]
  drm/fb-helper: Add support for DRM_FORMAT_C[124]
  drm/fourcc: Add drm_format_info_bpp() helper
  drm/client: Use actual bpp when allocating frame buffers
  drm/framebuffer: Use actual bpp for DRM_IOCTL_MODE_GETFB
  drm/gem-fb-helper: Use actual bpp for size calculations
  drm/fourcc: Add DRM_FORMAT_R[124]
  drm/fourcc: Add DRM_FORMAT_D1

 drivers/gpu/drm/drm_client.c                 |   4 +-
 drivers/gpu/drm/drm_fb_helper.c              | 120 ++++++++++++++-----
 drivers/gpu/drm/drm_fourcc.c                 |  45 +++++++
 drivers/gpu/drm/drm_framebuffer.c            |   2 +-
 drivers/gpu/drm/drm_gem_framebuffer_helper.c |  12 +-
 include/drm/drm_fourcc.h                     |   1 +
 include/uapi/drm/drm_fourcc.h                |  15 +++
 7 files changed, 160 insertions(+), 39 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 1/8] drm/fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Introduce fourcc codes for color-indexed frame buffer formats with two,
four, and sixteen color, and provide a suitable 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>
---
Do we want to keep the rounding down if depth < bpp, or insist on depth
== bpp? I don't think the rounding down will still be needed after
"[PATCH 4/8] drm/client: Use actual bpp when allocating frame buffers".
---
 drivers/gpu/drm/drm_fourcc.c  | 18 ++++++++++++++++++
 include/uapi/drm/drm_fourcc.h |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 07741b678798b0f1..60ce63d728b8e308 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -46,6 +46,18 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
 	case 8:
 		if (depth == 8)
 			fmt = DRM_FORMAT_C8;
+		fallthrough;
+	case 4:
+		if (depth == 4)
+			fmt = DRM_FORMAT_C4;
+		fallthrough;
+	case 2:
+		if (depth == 2)
+			fmt = DRM_FORMAT_C2;
+		fallthrough;
+	case 1:
+		if (depth == 1)
+			fmt = DRM_FORMAT_C1;
 		break;
 
 	case 16:
@@ -132,6 +144,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 },
+		{ .format = DRM_FORMAT_C2,		.depth = 2,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_C4,		.depth = 4,  .num_planes = 1,
+		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .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 },
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index fc0c1454d2757d5d..3f09174670b3cce6 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -99,6 +99,9 @@ extern "C" {
 #define DRM_FORMAT_INVALID	0
 
 /* color index */
+#define DRM_FORMAT_C1		fourcc_code('C', '1', ' ', ' ') /* [0] C */
+#define DRM_FORMAT_C2		fourcc_code('C', '2', ' ', ' ') /* [1:0] C */
+#define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [3:0] C */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
 
 /* 8 bpp Red */
-- 
2.25.1

[PATCH 2/8] 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 depths 1/2/4 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 supported
     color-indexed modes.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 120 +++++++++++++++++++++++++-------
 1 file changed, 93 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ed43b987d306afce..a4afed0de1570841 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -376,12 +376,34 @@ 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 (fb->format->depth) {
+	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 +1253,30 @@ 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)
-{
-	switch (depth) {
-	case 8:
+					 const struct drm_format_info *format)
+{
+	u8 depth = format->depth;
+
+	switch (format->format) {
+	// FIXME Perhaps
+	// #define DRM_FORMAT_C0 fourcc_code('C', '0', ' ', ' ')
+	// if ((format & fourcc_code(0xff, 0xf8, 0xff, 0xff) == DRM_FORMAT_C0) ...
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+	case DRM_FORMAT_C8:
 		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 +1331,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 +1343,34 @@ 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:
+		bpp = format->depth;
+		break;
+
+	default:
+		if ((drm_format_info_block_width(format, 0) > 1) ||
+		    (drm_format_info_block_height(format, 0) > 1))
+			return -EINVAL;
+
+		bpp = format->cpp[0] * 8;
+		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 ||
+	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 +1385,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 +1727,20 @@ 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)
+				   uint32_t format)
 {
 	info->fix.type = FB_TYPE_PACKED_PIXELS;
-	info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
-		FB_VISUAL_TRUECOLOR;
+	switch (format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+	case DRM_FORMAT_C8:
+		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
+		break;
+	default:
+		info->fix.visual = FB_VISUAL_TRUECOLOR;
+		break;
+	}
 	info->fix.mmio_start = 0;
 	info->fix.mmio_len = 0;
 	info->fix.type_aux = 0;
@@ -1701,19 +1757,29 @@ 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;
 
-	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;
+	switch (format->format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+		info->var.bits_per_pixel = format->depth;
+		break;
+
+	default:
+		WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
+			(drm_format_info_block_height(format, 0) > 1));
+		info->var.bits_per_pixel = format->cpp[0] * 8;
+	}
 	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 +1804,7 @@ 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->format);
 	drm_fb_helper_fill_var(info, fb_helper,
 			       sizes->fb_width, sizes->fb_height);
 
-- 
2.25.1

[PATCH 3/8] 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>
---
 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 60ce63d728b8e308..5c77ce10f53e3a64 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -388,6 +388,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 4/8] 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>
---
 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 5/8] 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>
---
Untested.
---
 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 6/8] 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>
---
Untested.

After adding the missing block info, probably the whole function can
just be dropped, in favor of drm_format_info_bpp()?
---
 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..7eca09fce095abbe 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -499,11 +499,8 @@ 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 */
+	// FIXME DRM_FORMAT_* should provide proper block info in
+	// FIXME drivers/gpu/drm/drm_fourcc.c
 	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 7/8] drm/fourcc: Add DRM_FORMAT_R[124]

[Geert Uytterhoeven]

Introduce fourcc codes for single-channel frame buffer formats with two,
four, and sixteen intensity levels.  Traditionally, the first channel
has been called the "red" channel, but the fourcc can also be used for
other light-on-dark displays.

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>
---
 drivers/gpu/drm/drm_fourcc.c  | 6 ++++++
 include/uapi/drm/drm_fourcc.h | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 5c77ce10f53e3a64..c12e48ecb1ab8aad 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -151,6 +151,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 },
 		{ .format = DRM_FORMAT_C8,		.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,
+		  .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 3f09174670b3cce6..8605a1acc6813e6c 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,6 +104,15 @@ extern "C" {
 #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [3:0] C */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
 
+/* 1 bpp Red */
+#define DRM_FORMAT_R1		fourcc_code('R', '1', ' ', ' ') /* [0] R */
+
+/* 2 bpp Red */
+#define DRM_FORMAT_R2		fourcc_code('R', '2', ' ', ' ') /* [1:0] R */
+
+/* 4 bpp Red */
+#define DRM_FORMAT_R4		fourcc_code('R', '4', ' ', ' ') /* [3:0] R */
+
 /* 8 bpp Red */
 #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
 
-- 
2.25.1

[PATCH 8/8] drm/fourcc: Add DRM_FORMAT_D1

[Geert Uytterhoeven]

Introduce a fourcc code for a single-channel frame buffer format with two
darkness levels.  This can be used for two-level dark-on-light displays.

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

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

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index c12e48ecb1ab8aad..d00ce5d8d1fb9dd3 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -151,6 +151,8 @@ 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 },
 		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .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_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 8605a1acc6813e6c..c15c6efcc65e5827 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,6 +104,9 @@ extern "C" {
 #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [3:0] C */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
 
+/* 1 bpp Darkness */
+#define DRM_FORMAT_D1		fourcc_code('D', '1', ' ', ' ') /* [0] D */
+
 /* 1 bpp Red */
 #define DRM_FORMAT_R1		fourcc_code('R', '1', ' ', ' ') /* [0] R */
 
-- 
2.25.1

Re: [PATCH 1/8] drm/fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

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

On Tue, 15 Feb 2022 17:52:19 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Introduce fourcc codes for color-indexed frame buffer formats with two,
> four, and sixteen color, and provide a suitable 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>
> ---
> Do we want to keep the rounding down if depth < bpp, or insist on depth
> == bpp? I don't think the rounding down will still be needed after
> "[PATCH 4/8] drm/client: Use actual bpp when allocating frame buffers".
> ---
>  drivers/gpu/drm/drm_fourcc.c  | 18 ++++++++++++++++++
>  include/uapi/drm/drm_fourcc.h |  3 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 07741b678798b0f1..60ce63d728b8e308 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -46,6 +46,18 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
>  	case 8:
>  		if (depth == 8)
>  			fmt = DRM_FORMAT_C8;
> +		fallthrough;
> +	case 4:
> +		if (depth == 4)
> +			fmt = DRM_FORMAT_C4;
> +		fallthrough;
> +	case 2:
> +		if (depth == 2)
> +			fmt = DRM_FORMAT_C2;
> +		fallthrough;
> +	case 1:
> +		if (depth == 1)
> +			fmt = DRM_FORMAT_C1;
>  		break;
>  
>  	case 16:
> @@ -132,6 +144,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 },
> +		{ .format = DRM_FORMAT_C2,		.depth = 2,  .num_planes = 1,
> +		  .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
> +		{ .format = DRM_FORMAT_C4,		.depth = 4,  .num_planes = 1,
> +		  .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
>  		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .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 },
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index fc0c1454d2757d5d..3f09174670b3cce6 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -99,6 +99,9 @@ extern "C" {
>  #define DRM_FORMAT_INVALID	0
>  
>  /* color index */
> +#define DRM_FORMAT_C1		fourcc_code('C', '1', ' ', ' ') /* [0] C */
> +#define DRM_FORMAT_C2		fourcc_code('C', '2', ' ', ' ') /* [1:0] C */
> +#define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [3:0] C */

Hi Geert,

generally this looks fine to me though I'm not familiar with the
code. The thing I'm missing here is a more precise description of the
new pixel formats.

>  #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */

This description of C8 is a little vague maybe, but presumably one
pixel being one byte, the address of pixel x is just &bytes[x].

C4, C2 and C1 should also specify the pixel order within the byte.
There is some precedent of that in with some YUV formats in this file. 

Maybe something like: 

C2 /* [7:0] c0:c1:c2:c3 2:2:2:2 four pixels per byte */

or the other way around, which ever your ordering is?


Thanks,
pq

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

Re: [PATCH 7/8] drm/fourcc: Add DRM_FORMAT_R[124]

[Pekka Paalanen]

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

On Tue, 15 Feb 2022 17:52:25 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Introduce fourcc codes for single-channel frame buffer formats with two,
> four, and sixteen intensity levels.  Traditionally, the first channel
> has been called the "red" channel, but the fourcc can also be used for
> other light-on-dark displays.
> 
> 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>
> ---
>  drivers/gpu/drm/drm_fourcc.c  | 6 ++++++
>  include/uapi/drm/drm_fourcc.h | 9 +++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 5c77ce10f53e3a64..c12e48ecb1ab8aad 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -151,6 +151,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 },
>  		{ .format = DRM_FORMAT_C8,		.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,
> +		  .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 3f09174670b3cce6..8605a1acc6813e6c 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -104,6 +104,15 @@ extern "C" {
>  #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [3:0] C */
>  #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
>  
> +/* 1 bpp Red */
> +#define DRM_FORMAT_R1		fourcc_code('R', '1', ' ', ' ') /* [0] R */
> +
> +/* 2 bpp Red */
> +#define DRM_FORMAT_R2		fourcc_code('R', '2', ' ', ' ') /* [1:0] R */
> +
> +/* 4 bpp Red */
> +#define DRM_FORMAT_R4		fourcc_code('R', '4', ' ', ' ') /* [3:0] R */
> +
>  /* 8 bpp Red */
>  #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
>  

Hi Geert,

I have the same comment here as for C1/C2/C4: these need to specify the
ordering inside a byte. Otherwise this reads as one byte of storage per
pixel, but using only 1/2/4 bits of each byte.

The idea of having Cx and Rx formats separately sounds good to me.


Thanks,
pq

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

Re: [PATCH 8/8] drm/fourcc: Add DRM_FORMAT_D1

[Pekka Paalanen]

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

On Tue, 15 Feb 2022 17:52:26 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Introduce a fourcc code for a single-channel frame buffer format with two
> darkness levels.  This can be used for two-level dark-on-light displays.
> 
> As the number of bits per pixel is less than eight, this relies on
> proper block handling for the calculation of bits per pixel and pitch.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/gpu/drm/drm_fourcc.c  | 2 ++
>  include/uapi/drm/drm_fourcc.h | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index c12e48ecb1ab8aad..d00ce5d8d1fb9dd3 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -151,6 +151,8 @@ 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 },
>  		{ .format = DRM_FORMAT_C8,		.depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> +		{ .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_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 8605a1acc6813e6c..c15c6efcc65e5827 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -104,6 +104,9 @@ extern "C" {
>  #define DRM_FORMAT_C4		fourcc_code('C', '4', ' ', ' ') /* [3:0] C */
>  #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
>  
> +/* 1 bpp Darkness */
> +#define DRM_FORMAT_D1		fourcc_code('D', '1', ' ', ' ') /* [0] D */
> +

Hi Geert,

the same comment here as for C1 and R1 formats, need to specify pixel
ordering inside a byte.

I think it would also be good to explain the rationale why C1 and R1
are not suitable for this case and we need yet another 1-bit format in
the commit message.

For posterity, of course. I roughly remember the discussions.

I also wonder if anyone would actually use D1. Should it be added
anyway? There is no rule that a pixel format must be used inside the
kernel AFAIK, but is there even a prospective userspace wanting this?

Exposing R1 and inverting bits while copying to hardware might be
enough?


Thanks,
pq

>  /* 1 bpp Red */
>  #define DRM_FORMAT_R1		fourcc_code('R', '1', ' ', ' ') /* [0] R */
>  


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

Re: [PATCH 8/8] drm/fourcc: Add DRM_FORMAT_D1

[Simon Ser]

Hi,

On Tuesday, February 15th, 2022 at 17:52, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Introduce a fourcc code for a single-channel frame buffer format with two
> darkness levels. This can be used for two-level dark-on-light displays.

The previous patches introduce C1 and R1. Do we really need D1? Can't
we just use R1 instead?

Simon

Re: [PATCH 8/8] drm/fourcc: Add DRM_FORMAT_D1

[Geert Uytterhoeven]

Hi Pekka,

On Thu, Feb 17, 2022 at 11:10 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Tue, 15 Feb 2022 17:52:26 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Introduce a fourcc code for a single-channel frame buffer format with two
> > darkness levels.  This can be used for two-level dark-on-light displays.
> >
> > As the number of bits per pixel is less than eight, this relies on
> > proper block handling for the calculation of bits per pixel and pitch.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -151,6 +151,8 @@ 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 },
> >               { .format = DRM_FORMAT_C8,              .depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> > +             { .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_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 8605a1acc6813e6c..c15c6efcc65e5827 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -104,6 +104,9 @@ extern "C" {
> >  #define DRM_FORMAT_C4                fourcc_code('C', '4', ' ', ' ') /* [3:0] C */
> >  #define DRM_FORMAT_C8                fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> >
> > +/* 1 bpp Darkness */
> > +#define DRM_FORMAT_D1                fourcc_code('D', '1', ' ', ' ') /* [0] D */
> > +
>
> the same comment here as for C1 and R1 formats, need to specify pixel
> ordering inside a byte.

Right, will do.

> I think it would also be good to explain the rationale why C1 and R1
> are not suitable for this case and we need yet another 1-bit format in
> the commit message.
>
> For posterity, of course. I roughly remember the discussions.

C1 is color-indexed, which can be any two colors.
R1 is light-on-dark.
D1 is dark-on-light.

> I also wonder if anyone would actually use D1. Should it be added
> anyway? There is no rule that a pixel format must be used inside the
> kernel AFAIK, but is there even a prospective userspace wanting this?
>
> Exposing R1 and inverting bits while copying to hardware might be
> enough?

That's an option.  The repaper driver does that:

    drm_fb_xrgb8888_to_gray8(buf, 0, cma_obj->vaddr, fb, &clip);
    repaper_gray8_to_mono_reversed(buf, fb->width, fb->height);

Can drm_framebuffer objects be backed by graphics memory, i.e.
can they be displayed without copying?

> >  /* 1 bpp Red */
> >  #define DRM_FORMAT_R1                fourcc_code('R', '1', ' ', ' ') /* [0] R */

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 8/8] drm/fourcc: Add DRM_FORMAT_D1

[Pekka Paalanen]

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

On Thu, 17 Feb 2022 11:42:29 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Pekka,
> 
> On Thu, Feb 17, 2022 at 11:10 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Tue, 15 Feb 2022 17:52:26 +0100
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > Introduce a fourcc code for a single-channel frame buffer format with two
> > > darkness levels.  This can be used for two-level dark-on-light displays.
> > >
> > > As the number of bits per pixel is less than eight, this relies on
> > > proper block handling for the calculation of bits per pixel and pitch.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>  
> 
> > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > @@ -151,6 +151,8 @@ 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 },
> > >               { .format = DRM_FORMAT_C8,              .depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
> > > +             { .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_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 8605a1acc6813e6c..c15c6efcc65e5827 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -104,6 +104,9 @@ extern "C" {
> > >  #define DRM_FORMAT_C4                fourcc_code('C', '4', ' ', ' ') /* [3:0] C */
> > >  #define DRM_FORMAT_C8                fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
> > >
> > > +/* 1 bpp Darkness */
> > > +#define DRM_FORMAT_D1                fourcc_code('D', '1', ' ', ' ') /* [0] D */
> > > +  
> >
> > the same comment here as for C1 and R1 formats, need to specify pixel
> > ordering inside a byte.  
> 
> Right, will do.

Btw. does endianess of anything have any effect on these pixel formats?

That's probably a weird question, but I recall Pixman (the pixel
handling library of the X server nowadays known as Xorg) having pixel
formats where CPU endianess affects whether the first pixel in a byte
is found at the MSB or LSB.

> > I think it would also be good to explain the rationale why C1 and R1
> > are not suitable for this case and we need yet another 1-bit format in
> > the commit message.
> >
> > For posterity, of course. I roughly remember the discussions.  
> 
> C1 is color-indexed, which can be any two colors.
> R1 is light-on-dark.
> D1 is dark-on-light.
> 
> > I also wonder if anyone would actually use D1. Should it be added
> > anyway? There is no rule that a pixel format must be used inside the
> > kernel AFAIK, but is there even a prospective userspace wanting this?
> >
> > Exposing R1 and inverting bits while copying to hardware might be
> > enough?  
> 
> That's an option.  The repaper driver does that:
> 
>     drm_fb_xrgb8888_to_gray8(buf, 0, cma_obj->vaddr, fb, &clip);
>     repaper_gray8_to_mono_reversed(buf, fb->width, fb->height);
> 
> Can drm_framebuffer objects be backed by graphics memory, i.e.
> can they be displayed without copying?

Yes, they can. That is actually their primary purpose. So the invert
bits approach only works with drivers that need to manually shovel the
data, but not with direct hardware scanout.

D1 might be useful on hardware that:
- can scanout the buffer directly, and
- does not have an optional inverter in its hardware pipeline, and
- does not benefit from a shadow buffer.

Do you happen to know any that fits the description?


Thanks,
pq

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

Re: [PATCH 8/8] drm/fourcc: Add DRM_FORMAT_D1

[Michel Dänzer]

On 2022-02-17 15:28, Pekka Paalanen wrote:
> On Thu, 17 Feb 2022 11:42:29 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
>> Hi Pekka,
>>
>> On Thu, Feb 17, 2022 at 11:10 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>> On Tue, 15 Feb 2022 17:52:26 +0100
>>> Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
>>>> Introduce a fourcc code for a single-channel frame buffer format with two
>>>> darkness levels.  This can be used for two-level dark-on-light displays.
>>>>
>>>> As the number of bits per pixel is less than eight, this relies on
>>>> proper block handling for the calculation of bits per pixel and pitch.
>>>>
>>>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>  
>>
>>>> --- a/drivers/gpu/drm/drm_fourcc.c
>>>> +++ b/drivers/gpu/drm/drm_fourcc.c
>>>> @@ -151,6 +151,8 @@ 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 },
>>>>               { .format = DRM_FORMAT_C8,              .depth = 8,  .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
>>>> +             { .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_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 8605a1acc6813e6c..c15c6efcc65e5827 100644
>>>> --- a/include/uapi/drm/drm_fourcc.h
>>>> +++ b/include/uapi/drm/drm_fourcc.h
>>>> @@ -104,6 +104,9 @@ extern "C" {
>>>>  #define DRM_FORMAT_C4                fourcc_code('C', '4', ' ', ' ') /* [3:0] C */
>>>>  #define DRM_FORMAT_C8                fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
>>>>
>>>> +/* 1 bpp Darkness */
>>>> +#define DRM_FORMAT_D1                fourcc_code('D', '1', ' ', ' ') /* [0] D */
>>>> +  
>>>
>>> the same comment here as for C1 and R1 formats, need to specify pixel
>>> ordering inside a byte.  
>>
>> Right, will do.
> 
> Btw. does endianess of anything have any effect on these pixel formats?
> 
> That's probably a weird question, but I recall Pixman (the pixel
> handling library of the X server nowadays known as Xorg) having pixel
> formats where CPU endianess affects whether the first pixel in a byte
> is found at the MSB or LSB.

Pixman probably has that for hysterical raisins inherited from the X code base. Conceptually, endianness is purely about the order of bytes in words, and is orthogonal to the order in which the bits of a byte/word are numbered.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [PATCH 2/8] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 9064 bytes --]

Hi Geert

Am 15.02.22 um 17:52 schrieb 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 depths 1/2/4 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 supported
>       color-indexed modes.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 120 +++++++++++++++++++++++++-------
>   1 file changed, 93 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index ed43b987d306afce..a4afed0de1570841 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -376,12 +376,34 @@ 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 (fb->format->depth) {

The depth field is deprecated. It's probably better to use 
fb->format->format and test against 4CC codes.

> +	case 1:
> +		offset += clip->x1 / 8;
> +		len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
> +		break;
> +

Style: no empty lines here.

> +	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;
> +

Can we handle case C8 like C[124]? Seems cleaner to me.

> +	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 +1253,30 @@ 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)
> -{
> -	switch (depth) {
> -	case 8:
> +					 const struct drm_format_info *format)
> +{
> +	u8 depth = format->depth;
> +
> +	switch (format->format) {
> +	// FIXME Perhaps
> +	// #define DRM_FORMAT_C0 fourcc_code('C', '0', ' ', ' ')

What is C0?

> +	// if ((format & fourcc_code(0xff, 0xf8, 0xff, 0xff) == DRM_FORMAT_C0) ...
> +	case DRM_FORMAT_C1:
> +	case DRM_FORMAT_C2:
> +	case DRM_FORMAT_C4:
> +	case DRM_FORMAT_C8:
>   		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 +1331,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 +1343,34 @@ 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:
> +		bpp = format->depth;
> +		break;

Added C8 here would be more consistent.

> +
> +	default:
> +		if ((drm_format_info_block_width(format, 0) > 1) ||
> +		    (drm_format_info_block_height(format, 0) > 1))
> +			return -EINVAL;
> +
> +		bpp = format->cpp[0] * 8;
> +		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 ||
> +	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 +1385,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 +1727,20 @@ 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)
> +				   uint32_t format)
>   {
>   	info->fix.type = FB_TYPE_PACKED_PIXELS;
> -	info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> -		FB_VISUAL_TRUECOLOR;
> +	switch (format) {
> +	case DRM_FORMAT_C1:
> +	case DRM_FORMAT_C2:
> +	case DRM_FORMAT_C4:
> +	case DRM_FORMAT_C8:
> +		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
> +		break;
> +	default:
> +		info->fix.visual = FB_VISUAL_TRUECOLOR;
> +		break;
> +	}
>   	info->fix.mmio_start = 0;
>   	info->fix.mmio_len = 0;
>   	info->fix.type_aux = 0;
> @@ -1701,19 +1757,29 @@ 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;
>   
> -	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;
> +	switch (format->format) {
> +	case DRM_FORMAT_C1:
> +	case DRM_FORMAT_C2:
> +	case DRM_FORMAT_C4:
> +		info->var.bits_per_pixel = format->depth;
> +		break;

C8.


The fbdev helpers look correct to me.  I'm not so sure about the usage 
of the format info; especially the depth field.  The docs say that the 
field is deprecated and should be 0. Maybe depth can be handled within 
fbdev?

Best regards
Thomas

> +
> +	default:
> +		WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
> +			(drm_format_info_block_height(format, 0) > 1));
> +		info->var.bits_per_pixel = format->cpp[0] * 8;
> +	}
>   	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 +1804,7 @@ 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->format);
>   	drm_fb_helper_fill_var(info, fb_helper,
>   			       sizes->fb_width, sizes->fb_height);
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

Re: [PATCH 4/8] drm/client: Use actual bpp when allocating frame buffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1699 bytes --]



Am 15.02.22 um 17:52 schrieb 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>

> ---
>   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;

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

Re: [PATCH 2/8] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Thomas,

Thanks for your review!

On Thu, Feb 17, 2022 at 3:57 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 15.02.22 um 17:52 schrieb 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 depths 1/2/4 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 supported
> >       color-indexed modes.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -376,12 +376,34 @@ 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 (fb->format->depth) {
>
> The depth field is deprecated. It's probably better to use
> fb->format->format and test against 4CC codes.

The reason I checked for depth instead of a 4CC code is that the only
thing that matters here is the number of bits per pixel.  Hence this
function won't need any changes to support R1, R2, R4, and D1 later.
When we get here, we already know that we are using a format that
is supported by the fbdev helper code, and thus passed the 4CC
checks elsewhere.

Alternatively, we could introduce drm_format_info_bpp() earlier in
the series, and use that?

>
> > +     case 1:
> > +             offset += clip->x1 / 8;
> > +             len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
> > +             break;
> > +
>
> Style: no empty lines here.

OK.

> > +     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;
> > +
>
> Can we handle case C8 like C[124]? Seems cleaner to me.

The cases above are purely to handle bpp < 8; they are not
about color-indexed vs. truecolor modes.
XRGB1111 mode would need to be handled above, too.

> > @@ -1231,19 +1253,30 @@ 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)
> > -{
> > -     switch (depth) {
> > -     case 8:
> > +                                      const struct drm_format_info *format)
> > +{
> > +     u8 depth = format->depth;
> > +
> > +     switch (format->format) {
> > +     // FIXME Perhaps
> > +     // #define DRM_FORMAT_C0 fourcc_code('C', '0', ' ', ' ')
>
> What is C0?

A non-existing color-indexed mode with zero colors ;-)
Introduced purely to make a check like in the comment below work.
What we really want to check here is if the mode is color-indexed
or not...

> > +     // if ((format & fourcc_code(0xff, 0xf8, 0xff, 0xff) == DRM_FORMAT_C0) ...
> > +     case DRM_FORMAT_C1:
> > +     case DRM_FORMAT_C2:
> > +     case DRM_FORMAT_C4:
> > +     case DRM_FORMAT_C8:
> >               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 +1331,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 +1343,34 @@ 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:
> > +             bpp = format->depth;
> > +             break;
>
> Added C8 here would be more consistent.

Again, this is not about color-indexed vs. truecolor, but about bpp.
drm_format_info_bpp()?

 > +
> > +     default:
> > +             if ((drm_format_info_block_width(format, 0) > 1) ||
> > +                 (drm_format_info_block_height(format, 0) > 1))
> > +                     return -EINVAL;
> > +
> > +             bpp = format->cpp[0] * 8;
> > +             break;
> > +     }

> > @@ -1680,11 +1727,20 @@ 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)
> > +                                uint32_t format)
> >   {
> >       info->fix.type = FB_TYPE_PACKED_PIXELS;
> > -     info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> > -             FB_VISUAL_TRUECOLOR;
> > +     switch (format) {

This one is about color-indexed vs. truecolor.

> > +     case DRM_FORMAT_C1:
> > +     case DRM_FORMAT_C2:
> > +     case DRM_FORMAT_C4:
> > +     case DRM_FORMAT_C8:
> > +             info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
> > +             break;
> > +     default:
> > +             info->fix.visual = FB_VISUAL_TRUECOLOR;
> > +             break;
> > +     }
> >       info->fix.mmio_start = 0;
> >       info->fix.mmio_len = 0;
> >       info->fix.type_aux = 0;
> > @@ -1701,19 +1757,29 @@ 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;
> >
> > -     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;
> > +     switch (format->format) {
> > +     case DRM_FORMAT_C1:
> > +     case DRM_FORMAT_C2:
> > +     case DRM_FORMAT_C4:
> > +             info->var.bits_per_pixel = format->depth;
> > +             break;
>
> C8.

Again, this is not about color-indexed vs. truecolor, but about bpp.
Here I do check the 4CC codes, as this controls which modes can be
handled by the fbdev emulation, and we do not want to let random
modes with depth or bpp < 8 pass.

> The fbdev helpers look correct to me.  I'm not so sure about the usage
> of the format info; especially the depth field.  The docs say that the
> field is deprecated and should be 0. Maybe depth can be handled within
> fbdev?

Perhaps. I don't know enough about DRM to know what the depth field
is used for.

Note that true fbdev supports all values of depth < bpp (e.g. a
32-color mode (depth = 5) where each pixel is stored in one byte).
I do not suggest adding support for that, though ;-)

> > +
> > +     default:
> > +             WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
> > +                     (drm_format_info_block_height(format, 0) > 1));

BTW, probably this WARN_ON() (which existed before, but got moved)
should be converted into returning an error instead.

> > +             info->var.bits_per_pixel = format->cpp[0] * 8;
> > +     }
> >       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;

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

[Simon Ser]

On Thursday, February 17th, 2022 at 17:12, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > What is C0?
>
> A non-existing color-indexed mode with zero colors ;-)
> Introduced purely to make a check like in the comment below work.
> What we really want to check here is if the mode is color-indexed
> or not...

Maybe it would be worth introducing a drm_format_info_is_color_indexed
function? Would be self-describing when used, and would avoid to miss
some places to update when adding new color-indexed formats.

Re: [PATCH 2/8] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Simon,

On Thu, Feb 17, 2022 at 5:18 PM Simon Ser <contact@emersion.fr> wrote:
> On Thursday, February 17th, 2022 at 17:12, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > What is C0?
> >
> > A non-existing color-indexed mode with zero colors ;-)
> > Introduced purely to make a check like in the comment below work.
> > What we really want to check here is if the mode is color-indexed
> > or not...
>
> Maybe it would be worth introducing a drm_format_info_is_color_indexed
> function? Would be self-describing when used, and would avoid to miss
> some places to update when adding new color-indexed formats.

Yep, and a .is_color_indexed flag, cfr. the existing .is_yuv flag.

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

[Sam Ravnborg]

Hi Geert,

> > > +     switch (fb->format->depth) {
> >
> > The depth field is deprecated. It's probably better to use
> > fb->format->format and test against 4CC codes.
> 
> The reason I checked for depth instead of a 4CC code is that the only
> thing that matters here is the number of bits per pixel.  Hence this
> function won't need any changes to support R1, R2, R4, and D1 later.
> When we get here, we already know that we are using a format that
> is supported by the fbdev helper code, and thus passed the 4CC
> checks elsewhere.
> 
> Alternatively, we could introduce drm_format_info_bpp() earlier in
> the series, and use that?

The drm_format_info_bpp() is very descriptive, so yes it would be good to use
it here also.

	Sam

Re: [PATCH 8/8] drm/fourcc: Add DRM_FORMAT_D1

[Sam Ravnborg]

Hi Geert,

> 
> C1 is color-indexed, which can be any two colors.
> R1 is light-on-dark.
> D1 is dark-on-light.

It would be nice to have this explained in the fourcc.h file,
preferably with a little more verbosity than the current explanations.

	Sam

Re: [PATCH 0/8] drm: Add support for low-color frame buffer formats

[Sam Ravnborg]

Hi Geert,

On Tue, Feb 15, 2022 at 05:52:18PM +0100, Geert Uytterhoeven wrote:
> 	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 small embedded displays.

This is one of the pieces missing for a long time - great to see
something done here. Thanks Geert!

	Sam

Re: [PATCH 2/8] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 10996 bytes --]

Hi

Am 17.02.22 um 17:12 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> Thanks for your review!
> 
> On Thu, Feb 17, 2022 at 3:57 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 15.02.22 um 17:52 schrieb 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 depths 1/2/4 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 supported
>>>        color-indexed modes.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -376,12 +376,34 @@ 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 (fb->format->depth) {
>>
>> The depth field is deprecated. It's probably better to use
>> fb->format->format and test against 4CC codes.
> 
> The reason I checked for depth instead of a 4CC code is that the only
> thing that matters here is the number of bits per pixel.  Hence this
> function won't need any changes to support R1, R2, R4, and D1 later.
> When we get here, we already know that we are using a format that
> is supported by the fbdev helper code, and thus passed the 4CC
> checks elsewhere.

At some point, we will probably have to change several of these tests to 
4cc. C8 and RGB332 both have 8-bit depth/bpp; same for C4 and RGB121; or 
whatever low-color formats we also want to add.

It's not a blocker now, but maybe something to keep in mind.

> 
> Alternatively, we could introduce drm_format_info_bpp() earlier in
> the series, and use that?

Having a helper for this might indeed be useful. We use depth for the 
number of color bits and bpp for the number of bits in he pixel.  That's 
important for XRGB8888, where depth is 24, or XRGB555 where depth is 15.

If that makes sense, maybe have a helper for depth and one for bpp, even 
if they return the same value in most of the cases.

> 
>>
>>> +     case 1:
>>> +             offset += clip->x1 / 8;
>>> +             len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
>>> +             break;
>>> +
>>
>> Style: no empty lines here.
> 
> OK.
> 
>>> +     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;
>>> +
>>
>> Can we handle case C8 like C[124]? Seems cleaner to me.
> 
> The cases above are purely to handle bpp < 8; they are not
> about color-indexed vs. truecolor modes.
> XRGB1111 mode would need to be handled above, too.

I see.

> 
>>> @@ -1231,19 +1253,30 @@ 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)
>>> -{
>>> -     switch (depth) {
>>> -     case 8:
>>> +                                      const struct drm_format_info *format)
>>> +{
>>> +     u8 depth = format->depth;
>>> +
>>> +     switch (format->format) {
>>> +     // FIXME Perhaps
>>> +     // #define DRM_FORMAT_C0 fourcc_code('C', '0', ' ', ' ')
>>
>> What is C0?
> 
> A non-existing color-indexed mode with zero colors ;-)
> Introduced purely to make a check like in the comment below work.
> What we really want to check here is if the mode is color-indexed
> or not...

I think I'd rather keep that switch.

Best regards
Thomas

> 
>>> +     // if ((format & fourcc_code(0xff, 0xf8, 0xff, 0xff) == DRM_FORMAT_C0) ...
>>> +     case DRM_FORMAT_C1:
>>> +     case DRM_FORMAT_C2:
>>> +     case DRM_FORMAT_C4:
>>> +     case DRM_FORMAT_C8:
>>>                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 +1331,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 +1343,34 @@ 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:
>>> +             bpp = format->depth;
>>> +             break;
>>
>> Added C8 here would be more consistent.
> 
> Again, this is not about color-indexed vs. truecolor, but about bpp.
> drm_format_info_bpp()?
> 
>   > +
>>> +     default:
>>> +             if ((drm_format_info_block_width(format, 0) > 1) ||
>>> +                 (drm_format_info_block_height(format, 0) > 1))
>>> +                     return -EINVAL;
>>> +
>>> +             bpp = format->cpp[0] * 8;
>>> +             break;
>>> +     }
> 
>>> @@ -1680,11 +1727,20 @@ 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)
>>> +                                uint32_t format)
>>>    {
>>>        info->fix.type = FB_TYPE_PACKED_PIXELS;
>>> -     info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
>>> -             FB_VISUAL_TRUECOLOR;
>>> +     switch (format) {
> 
> This one is about color-indexed vs. truecolor.
> 
>>> +     case DRM_FORMAT_C1:
>>> +     case DRM_FORMAT_C2:
>>> +     case DRM_FORMAT_C4:
>>> +     case DRM_FORMAT_C8:
>>> +             info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
>>> +             break;
>>> +     default:
>>> +             info->fix.visual = FB_VISUAL_TRUECOLOR;
>>> +             break;
>>> +     }
>>>        info->fix.mmio_start = 0;
>>>        info->fix.mmio_len = 0;
>>>        info->fix.type_aux = 0;
>>> @@ -1701,19 +1757,29 @@ 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;
>>>
>>> -     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;
>>> +     switch (format->format) {
>>> +     case DRM_FORMAT_C1:
>>> +     case DRM_FORMAT_C2:
>>> +     case DRM_FORMAT_C4:
>>> +             info->var.bits_per_pixel = format->depth;
>>> +             break;
>>
>> C8.
> 
> Again, this is not about color-indexed vs. truecolor, but about bpp.
> Here I do check the 4CC codes, as this controls which modes can be
> handled by the fbdev emulation, and we do not want to let random
> modes with depth or bpp < 8 pass.
> 
>> The fbdev helpers look correct to me.  I'm not so sure about the usage
>> of the format info; especially the depth field.  The docs say that the
>> field is deprecated and should be 0. Maybe depth can be handled within
>> fbdev?
> 
> Perhaps. I don't know enough about DRM to know what the depth field
> is used for.
> 
> Note that true fbdev supports all values of depth < bpp (e.g. a
> 32-color mode (depth = 5) where each pixel is stored in one byte).
> I do not suggest adding support for that, though ;-)
> 
>>> +
>>> +     default:
>>> +             WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
>>> +                     (drm_format_info_block_height(format, 0) > 1));
> 
> BTW, probably this WARN_ON() (which existed before, but got moved)
> should be converted into returning an error instead.
> 
>>> +             info->var.bits_per_pixel = format->cpp[0] * 8;
>>> +     }
>>>        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;
> 
> 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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

Re: [PATCH 2/8] drm/fb-helper: Add support for DRM_FORMAT_C[124]

[Geert Uytterhoeven]

Hi Thomas,

On Fri, Feb 18, 2022 at 9:14 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 17.02.22 um 17:12 schrieb Geert Uytterhoeven:
> > On Thu, Feb 17, 2022 at 3:57 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Am 15.02.22 um 17:52 schrieb 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 depths 1/2/4 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 supported
> >>>        color-indexed modes.
> >>>
> >>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >
> >>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>> @@ -376,12 +376,34 @@ 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 (fb->format->depth) {
> >>
> >> The depth field is deprecated. It's probably better to use
> >> fb->format->format and test against 4CC codes.
> >
> > The reason I checked for depth instead of a 4CC code is that the only
> > thing that matters here is the number of bits per pixel.  Hence this
> > function won't need any changes to support R1, R2, R4, and D1 later.
> > When we get here, we already know that we are using a format that
> > is supported by the fbdev helper code, and thus passed the 4CC
> > checks elsewhere.
>
> At some point, we will probably have to change several of these tests to
> 4cc. C8 and RGB332 both have 8-bit depth/bpp; same for C4 and RGB121; or
> whatever low-color formats we also want to add.
>
> It's not a blocker now, but maybe something to keep in mind.
>
> >
> > Alternatively, we could introduce drm_format_info_bpp() earlier in
> > the series, and use that?
>
> Having a helper for this might indeed be useful. We use depth for the
> number of color bits and bpp for the number of bits in he pixel.  That's
> important for XRGB8888, where depth is 24, or XRGB555 where depth is 15.
>
> If that makes sense, maybe have a helper for depth and one for bpp, even
> if they return the same value in most of the cases.

The helper for bpp is introduced in "[PATCH 3/8] drm/fourcc: Add
drm_format_info_bpp() helper".
I don't think we need a helper for depth, there's already the .depth
field.  It might be deprecated, but it's still used?
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 0/8] drm: Add support for low-color frame buffer formats

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 745 bytes --]



Am 17.02.22 um 21:37 schrieb Sam Ravnborg:
> Hi Geert,
> 
> On Tue, Feb 15, 2022 at 05:52:18PM +0100, Geert Uytterhoeven wrote:
>> 	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 small embedded displays.
> 
> This is one of the pieces missing for a long time - great to see
> something done here. Thanks Geert!

Absolutely! I'm looking forward to see these patches being merged.

Best regards
Thomas

> 
> 	Sam

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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