[PATCH v2 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[PATCH v2 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

        Hi all,

The native display format of ssd1306 OLED displays is monochrome
light-on-dark (R1).  This patch series adds support for the R1 buffer
format to both the ssd130x DRM driver and the FB helpers, so monochrome
applications (including fbdev emulation and the text console) not only
look better, but also avoid the overhead of back-and-forth conversions
between R1 and XR24.

This series consists of 4 parts:
  - Patches 1-2 contain fixes,
  - Patch 3 contains a small improvement,
  - Patch 4 adds R1 support to the ssd130x DRM driver,
  - Patches 5-6 update the DRM client and FB helper code to avoid
    calling drm_mode_legacy_fb_format() where the exact buffer format is
    already known, to prepare for R1 support,
  - Patch 7 adds support for R1 to fbdev emulation and the text console,
  - Patch 8 switches ssd130x to R1 for fbdev emulation and the text
    console.

Changes compared to v1[1]:
  - Drop "[PATCH 1/8] drm/ssd130x: Fix pitch calculation in
    ssd130x_fb_blit_rect()" (already applied),
  - Drop "[PATCH/RFC 3/8] drm/ssd130x: Bail out early if data_array is
    not yet available" (no longer needed),
  - New patch "[PATCH v2 2/8] drm/ssd130x: Fix screen clearing",
  - New patch "[PATCH v2 3/8] drm/ssd130x: Use bool for
    ssd130x_deviceinfo flags",
  - Add Reviewed-by, Tested-by,
  - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use
    shadow-buffer helpers when managing plane's state") in drm/drm-next,
  - Do not allocate intermediate buffer when not needed,
  - s/drm_mode_create_dumb/drm_client_buffer_addfb/ in one-line summary,
  - Fix accidental debug level increase.

This has been tested on an Adafruit FeatherWing 128x32 OLED, connected
to an OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V
softcore, using the fbdev text console.

Thanks for your comments!

P.S. Note that the biggest hurdle was the copious use of the
     drm_mode_legacy_fb_format() helper in various places.  This helper
     cannot decide between C1 and R1 without knowledge of the
     capabilities of the full display pipeline.  Instead of
     special-casing its return value in three callers, I did so in only
     one place, and got rid of two of these calls in the call chain.
     I think Thomas' grand plan is to replace preferred_{bpp,depth} by a
     preferred fourcc format? That would simplify things a lot...

[1] "[PATCH 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1"
    https://lore.kernel.org/r/cover.1689252746.git.geert@linux-m68k.org

Geert Uytterhoeven (8):
  drm/dumb-buffers: Fix drm_mode_create_dumb() for bpp < 8
  drm/ssd130x: Fix screen clearing
  drm/ssd130x: Use bool for ssd130x_deviceinfo flags
  drm/ssd130x: Add support for DRM_FORMAT_R1
  drm/client: Convert drm_client_buffer_addfb() to drm_mode_addfb2()
  drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size
  drm/fb-helper: Add support for DRM_FORMAT_R1
  drm/ssd130x: Switch preferred_bpp/depth to 1

 drivers/gpu/drm/drm_client.c        |  13 ++-
 drivers/gpu/drm/drm_dumb_buffers.c  |   3 +-
 drivers/gpu/drm/drm_fb_helper.c     |  42 ++++++---
 drivers/gpu/drm/drm_fbdev_generic.c |   9 +-
 drivers/gpu/drm/solomon/ssd130x.c   | 127 ++++++++++++++++++++--------
 drivers/gpu/drm/solomon/ssd130x.h   |   4 +-
 include/drm/drm_fb_helper.h         |   2 +
 7 files changed, 138 insertions(+), 62 deletions(-)

-- 
2.34.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 1/8] drm/dumb-buffers: Fix drm_mode_create_dumb() for bpp < 8

[Geert Uytterhoeven]

drm_mode_create_dumb() calculates the number of characters per pixel
from the number of bits per pixel by rounding up, which is not correct
as the actual value of cpp may be non-integer.  While we do not need to
care here about complex formats like YUV, bpp < 8 is a valid use case.

  - The overflow check for the buffer width is not correct if bpp < 8.
    However, it doesn't hurt, as widths larger than U32_MAX / 8 should
    not happen for real anyway.  Add a comment to clarify.
  - Calculating the stride from the number of characters per pixel is
    not correct.  Fix this by calculating it from the number of bits per
    pixel instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Tested-by: Javier Martinez Canillas <javierm@redhat.com>
---
v2:
  - Add Reviewed-by, Tested-by.
---
 drivers/gpu/drm/drm_dumb_buffers.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index 70032bba1c97e787..21a04c32a5e3d785 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -71,10 +71,11 @@ int drm_mode_create_dumb(struct drm_device *dev,
 	/* overflow checks for 32bit size calculations */
 	if (args->bpp > U32_MAX - 8)
 		return -EINVAL;
+	/* Incorrect (especially if bpp < 8), but doesn't hurt much */
 	cpp = DIV_ROUND_UP(args->bpp, 8);
 	if (cpp > U32_MAX / args->width)
 		return -EINVAL;
-	stride = cpp * args->width;
+	stride = DIV_ROUND_UP(args->bpp * args->width, 8);
 	if (args->height > U32_MAX / stride)
 		return -EINVAL;
 
-- 
2.34.1

[PATCH v2 2/8] drm/ssd130x: Fix screen clearing

[Geert Uytterhoeven]

Due to the reuse of buffers, ssd130x_clear_screen() no longers clears
the screen, but merely redraws the last image that is residing in the
intermediate buffer.

As there is no point in clearing the intermediate buffer and transposing
an all-black image, fix this by just clearing the HW format buffer, and
writing it to the panel.

Fixes: 49d7d581ceaf4cf8 ("drm/ssd130x: Don't allocate buffers on each plane update")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
ssd130x_clear_screen() is only called from
ssd130x_primary_plane_helper_atomic_disable(), but this never happens on
my system.

Tested by adding some extra calls to ssd130x_clear_screen() at regular
intervals.

v2:
  - New.
---
 drivers/gpu/drm/solomon/ssd130x.c | 47 +++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 5a80b228d18cae33..78272b1f9d5b167f 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -553,14 +553,45 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
 static void ssd130x_clear_screen(struct ssd130x_device *ssd130x,
 				 struct ssd130x_plane_state *ssd130x_state)
 {
-	struct drm_rect fullscreen = {
-		.x1 = 0,
-		.x2 = ssd130x->width,
-		.y1 = 0,
-		.y2 = ssd130x->height,
-	};
-
-	ssd130x_update_rect(ssd130x, ssd130x_state, &fullscreen);
+	unsigned int page_height = ssd130x->device_info->page_height;
+	unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
+	u8 *data_array = ssd130x_state->data_array;
+	unsigned int width = ssd130x->width;
+	int ret, i;
+
+	if (!ssd130x->page_address_mode) {
+		memset(data_array, 0, width * pages);
+
+		/* Set address range for horizontal addressing mode */
+		ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset, width);
+		if (ret < 0)
+			return;
+
+		ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset, pages);
+		if (ret < 0)
+			return;
+
+		/* Write out update in one go if we aren't using page addressing mode */
+		ssd130x_write_data(ssd130x, data_array, width * pages);
+	} else {
+		/*
+		 * In page addressing mode, the start address needs to be reset,
+		 * and each page then needs to be written out separately.
+		 */
+		memset(data_array, 0, width);
+
+		for (i = 0; i < pages; i++) {
+			ret = ssd130x_set_page_pos(ssd130x,
+						   ssd130x->page_offset + i,
+						   ssd130x->col_offset);
+			if (ret < 0)
+				return;
+
+			ret = ssd130x_write_data(ssd130x, data_array, width);
+			if (ret < 0)
+				return;
+		}
+	}
 }
 
 static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
-- 
2.34.1

[PATCH v2 3/8] drm/ssd130x: Use bool for ssd130x_deviceinfo flags

[Geert Uytterhoeven]

The .need_pwm and .need_chargepump fields in struct ssd130x_deviceinfo
are flags that can have only two possible values: 0 and 1.
Reduce kernel size by changing their types from int to bool.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - New.
---
 drivers/gpu/drm/solomon/ssd130x.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
index 87968b3e7fb8279d..aa39b13615ebedf2 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -40,8 +40,8 @@ struct ssd130x_deviceinfo {
 	u32 default_width;
 	u32 default_height;
 	u32 page_height;
-	int need_pwm;
-	int need_chargepump;
+	bool need_pwm;
+	bool need_chargepump;
 	bool page_mode_only;
 };
 
-- 
2.34.1

[PATCH v2 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

The native display format is monochrome light-on-dark (R1).
Hence add support for R1, so monochrome applications not only look
better, but also avoid the overhead of back-and-forth conversions
between R1 and XR24.

Do not allocate the intermediate conversion buffer when it is not
needed, and reorder the two buffer allocations to streamline operation.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
  - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use
    shadow-buffer helpers when managing plane's state") in drm/drm-next.
    Hence I did not add Javier's tags given on v1.
  - Do not allocate intermediate buffer when not needed.
---
 drivers/gpu/drm/solomon/ssd130x.c | 76 +++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 78272b1f9d5b167f..18007cb4f3de5aa1 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -449,15 +449,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
 
 static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
 			       struct ssd130x_plane_state *ssd130x_state,
+			       u8 *buf, unsigned int pitch,
 			       struct drm_rect *rect)
 {
 	unsigned int x = rect->x1;
 	unsigned int y = rect->y1;
-	u8 *buf = ssd130x_state->buffer;
 	u8 *data_array = ssd130x_state->data_array;
 	unsigned int width = drm_rect_width(rect);
 	unsigned int height = drm_rect_height(rect);
-	unsigned int line_length = DIV_ROUND_UP(width, 8);
 	unsigned int page_height = ssd130x->device_info->page_height;
 	unsigned int pages = DIV_ROUND_UP(height, page_height);
 	struct drm_device *drm = &ssd130x->drm;
@@ -516,7 +515,7 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
 			u8 data = 0;
 
 			for (k = 0; k < m; k++) {
-				u8 byte = buf[(8 * i + k) * line_length + j / 8];
+				u8 byte = buf[(8 * i + k) * pitch + j / 8];
 				u8 bit = (byte >> (j % 8)) & 1;
 
 				data |= bit << k;
@@ -602,27 +601,51 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
 	unsigned int page_height = ssd130x->device_info->page_height;
 	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
-	u8 *buf = ssd130x_state->buffer;
 	struct iosys_map dst;
 	unsigned int dst_pitch;
 	int ret = 0;
+	u8 *buf;
 
 	/* Align y to display page boundaries */
 	rect->y1 = round_down(rect->y1, page_height);
 	rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
 
-	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+	switch (fb->format->format) {
+	case DRM_FORMAT_R1:
+		/* Align x to byte boundaries */
+		rect->x1 = round_down(rect->x1, 8);
+		rect->x2 = round_up(rect->x2, 8);
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		return ret;
+		ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
+
+		dst_pitch = fb->pitches[0];
+		buf = vmap[0].vaddr + rect->y1 * dst_pitch + rect->x1 / 8;
+
+		ssd130x_update_rect(ssd130x, ssd130x_state, buf, dst_pitch,
+				    rect);
+
+		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+		break;
+
+	case DRM_FORMAT_XRGB8888:
+		dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+		buf = ssd130x_state->buffer;
+
+		ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
 
-	iosys_map_set_vaddr(&dst, buf);
-	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
+		iosys_map_set_vaddr(&dst, buf);
+		drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
 
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 
-	ssd130x_update_rect(ssd130x, ssd130x_state, rect);
+		ssd130x_update_rect(ssd130x, ssd130x_state, buf, dst_pitch,
+				    rect);
+		break;
+	}
 
 	return ret;
 }
@@ -644,22 +667,24 @@ static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
-	fi = drm_format_info(DRM_FORMAT_R1);
-	if (!fi)
-		return -EINVAL;
+	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
+	if (!ssd130x_state->data_array)
+		return -ENOMEM;
 
-	pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
+	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
+		fi = drm_format_info(DRM_FORMAT_R1);
+		if (!fi)
+			return -EINVAL;
 
-	ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
-	if (!ssd130x_state->buffer)
-		return -ENOMEM;
+		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
 
-	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
-	if (!ssd130x_state->data_array) {
-		kfree(ssd130x_state->buffer);
-		/* Set to prevent a double free in .atomic_destroy_state() */
-		ssd130x_state->buffer = NULL;
-		return -ENOMEM;
+		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
+		if (!ssd130x_state->buffer) {
+			kfree(ssd130x_state->data_array);
+			/* Set to prevent a double free in .atomic_destroy_state() */
+			ssd130x_state->data_array = NULL;
+			return -ENOMEM;
+		}
 	}
 
 	return 0;
@@ -898,6 +923,7 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = {
 };
 
 static const uint32_t ssd130x_formats[] = {
+	DRM_FORMAT_R1,
 	DRM_FORMAT_XRGB8888,
 };
 
-- 
2.34.1

[PATCH v2 5/8] drm/client: Convert drm_client_buffer_addfb() to drm_mode_addfb2()

[Geert Uytterhoeven]

Currently drm_client_buffer_addfb() uses the legacy drm_mode_addfb(),
which uses bpp and depth to guess the wanted buffer format.
However, drm_client_buffer_addfb() already knows the exact buffer
format, so there is no need to convert back and forth between buffer
format and bpp/depth, and the function can just call drm_mode_addfb2()
directly instead.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Tested-by: Javier Martinez Canillas <javierm@redhat.com>
---
v2:
  - Add Reviewed-by, Tested-by,
  - s/drm_mode_create_dumb/drm_client_buffer_addfb/ in one-line summary.
---
 drivers/gpu/drm/drm_client.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 037e36f2049c1793..0ced189befebae12 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -395,19 +395,16 @@ static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,
 				   u32 handle)
 {
 	struct drm_client_dev *client = buffer->client;
-	struct drm_mode_fb_cmd fb_req = { };
-	const struct drm_format_info *info;
+	struct drm_mode_fb_cmd2 fb_req = { };
 	int ret;
 
-	info = drm_format_info(format);
-	fb_req.bpp = drm_format_info_bpp(info, 0);
-	fb_req.depth = info->depth;
 	fb_req.width = width;
 	fb_req.height = height;
-	fb_req.handle = handle;
-	fb_req.pitch = buffer->pitch;
+	fb_req.pixel_format = format;
+	fb_req.handles[0] = handle;
+	fb_req.pitches[0] = buffer->pitch;
 
-	ret = drm_mode_addfb(client->dev, &fb_req, client->file);
+	ret = drm_mode_addfb2(client->dev, &fb_req, client->file);
 	if (ret)
 		return ret;
 
-- 
2.34.1

[PATCH v2 6/8] drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size

[Geert Uytterhoeven]

drm_fb_helper_single_fb_probe() first calls drm_fb_helper_find_sizes(),
followed by drm_fbdev_generic_helper_fb_probe():
  - The former tries to find a suitable buffer format, taking into
    account limitations of the whole display pipeline,
  - The latter just calls drm_mode_legacy_fb_format() again.

Simplify this by passing the buffer format between these functions
via a new buffer format member in the drm_fb_helper_surface_size
structure.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Tested-by: Javier Martinez Canillas <javierm@redhat.com>
---
v2:
  - Fix accidental debug level increase,
  - Add Reviewed-by, Tested-by.
---
 drivers/gpu/drm/drm_fb_helper.c     | 1 +
 drivers/gpu/drm/drm_fbdev_generic.c | 9 ++++-----
 include/drm/drm_fb_helper.h         | 2 ++
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d612133e2cf7ec99..4dc28fdcc1e0a6a4 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1564,6 +1564,7 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper,
 	info = drm_format_info(surface_format);
 	sizes->surface_bpp = drm_format_info_bpp(info, 0);
 	sizes->surface_depth = info->depth;
+	sizes->surface_format = surface_format;
 
 	/* first up get a count of crtcs now in use and new min/maxes width/heights */
 	crtc_count = 0;
diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index d647d89764cb9894..3830d25bcc3ad035 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -77,16 +77,15 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
 	struct fb_info *info;
 	size_t screen_size;
 	void *screen_buffer;
-	u32 format;
 	int ret;
 
-	drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
+	drm_dbg_kms(dev, "surface width(%d), height(%d), bpp(%d) and format(%p4cc)\n",
 		    sizes->surface_width, sizes->surface_height,
-		    sizes->surface_bpp);
+		    sizes->surface_bpp, &sizes->surface_format);
 
-	format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
 	buffer = drm_client_framebuffer_create(client, sizes->surface_width,
-					       sizes->surface_height, format);
+					       sizes->surface_height,
+					       sizes->surface_format);
 	if (IS_ERR(buffer))
 		return PTR_ERR(buffer);
 
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 375737fd6c36ed19..aa3d62a531d12f37 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -45,6 +45,7 @@ struct drm_fb_helper;
  * @surface_height: scanout buffer height
  * @surface_bpp: scanout buffer bpp
  * @surface_depth: scanout buffer depth
+ * @surface_format: scanout buffer format (optional)
  *
  * Note that the scanout surface width/height may be larger than the fbdev
  * width/height.  In case of multiple displays, the scanout surface is sized
@@ -61,6 +62,7 @@ struct drm_fb_helper_surface_size {
 	u32 surface_height;
 	u32 surface_bpp;
 	u32 surface_depth;
+	u32 surface_format;
 };
 
 /**
-- 
2.34.1

[PATCH v2 7/8] drm/fb-helper: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Add support for the monochrome light-on-dark buffer format (R1) to the
fb helper, so this format can be used for fbdev emulation and for the
text console.  This avoids the overhead of using XR24 and the associated
conversions on display hardware that supports only a simple monochrome
format.

R1 is very similar to C1 (monochrome indexed color), and shares the same
depth and bpp.  As drm_mode_legacy_fb_format() returns a format based on
only depth and bpp, it cannot distinguish between R1 and C1.  Hence
drm_fb_helper_find_format() is modified to try to fall back to R1 if C1
is not supported.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Tested-by: Javier Martinez Canillas <javierm@redhat.com>
---
v2:
  - Add Reviewed-by, Tested-by.
---
 drivers/gpu/drm/drm_fb_helper.c | 41 ++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 4dc28fdcc1e0a6a4..71baf8597516e9fd 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1130,7 +1130,7 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
 {
 	u8 depth = format->depth;
 
-	if (format->is_color_indexed) {
+	if (format->format == DRM_FORMAT_R1 || format->is_color_indexed) {
 		var->red.offset = 0;
 		var->green.offset = 0;
 		var->blue.offset = 0;
@@ -1236,6 +1236,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	case DRM_FORMAT_C1:
 	case DRM_FORMAT_C2:
 	case DRM_FORMAT_C4:
+	case DRM_FORMAT_R1:
 		/* supported format with sub-byte pixels */
 		break;
 
@@ -1439,12 +1440,24 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
+static bool is_supported_format(uint32_t format, const uint32_t *formats,
+				size_t format_count)
+{
+	size_t i;
+
+	for (i = 0; i < format_count; ++i) {
+		if (formats[i] == format)
+			return true;
+	}
+
+	return false;
+}
+
 static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const uint32_t *formats,
 					  size_t format_count, uint32_t bpp, uint32_t depth)
 {
 	struct drm_device *dev = fb_helper->dev;
 	uint32_t format;
-	size_t i;
 
 	/*
 	 * Do not consider YUV or other complicated formats
@@ -1457,10 +1470,12 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const
 	if (!format)
 		goto err;
 
-	for (i = 0; i < format_count; ++i) {
-		if (formats[i] == format)
-			return format;
-	}
+	if (is_supported_format(format, formats, format_count))
+		return format;
+
+	if (format == DRM_FORMAT_C1 &&
+	    is_supported_format(DRM_FORMAT_R1, formats, format_count))
+		return DRM_FORMAT_R1;
 
 err:
 	/* We found nothing. */
@@ -1680,11 +1695,15 @@ 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,
-				   bool is_color_indexed)
+				   const struct drm_format_info *format)
 {
 	info->fix.type = FB_TYPE_PACKED_PIXELS;
-	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
-					    : FB_VISUAL_TRUECOLOR;
+	if (format->format == DRM_FORMAT_R1)
+		info->fix.visual = FB_VISUAL_MONO10;
+	else if (format->is_color_indexed)
+		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
+	else
+		info->fix.visual = FB_VISUAL_TRUECOLOR;
 	info->fix.mmio_start = 0;
 	info->fix.mmio_len = 0;
 	info->fix.type_aux = 0;
@@ -1707,6 +1726,7 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
 	case DRM_FORMAT_C1:
 	case DRM_FORMAT_C2:
 	case DRM_FORMAT_C4:
+	case DRM_FORMAT_R1:
 		/* supported format with sub-byte pixels */
 		break;
 
@@ -1747,8 +1767,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->is_color_indexed);
+	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format);
 	drm_fb_helper_fill_var(info, fb_helper,
 			       sizes->fb_width, sizes->fb_height);
 
-- 
2.34.1

[PATCH v2 8/8] drm/ssd130x: Switch preferred_bpp/depth to 1

[Geert Uytterhoeven]

The native display format is R1.  Hence change the preferred_depth and
preferred_bpp to 1, to avoid the overhead of using XR24 and the
associated conversions when using fbdev emulation and its text console.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Tested-by: Javier Martinez Canillas <javierm@redhat.com>
---
v2:
  - Add Reviewed-by, Tested-by.
---
 drivers/gpu/drm/solomon/ssd130x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 18007cb4f3de5aa1..0d2b36ba40113fa3 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1049,7 +1049,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
 	drm->mode_config.max_width = max_width;
 	drm->mode_config.min_height = mode->vdisplay;
 	drm->mode_config.max_height = max_height;
-	drm->mode_config.preferred_depth = 24;
+	drm->mode_config.preferred_depth = 1;
 	drm->mode_config.funcs = &ssd130x_mode_config_funcs;
 
 	/* Primary plane */
@@ -1179,7 +1179,7 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
 	if (ret)
 		return ERR_PTR(dev_err_probe(dev, ret, "DRM device register failed\n"));
 
-	drm_fbdev_generic_setup(drm, 32);
+	drm_fbdev_generic_setup(drm, 1);
 
 	return ssd130x;
 }
-- 
2.34.1

Re: [PATCH v2 5/8] drm/client: Convert drm_client_buffer_addfb() to drm_mode_addfb2()

[Daniel Stone]

Hi Geert,

On Thu, 24 Aug 2023 at 16:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>         struct drm_client_dev *client = buffer->client;
> -       struct drm_mode_fb_cmd fb_req = { };
> -       const struct drm_format_info *info;
> +       struct drm_mode_fb_cmd2 fb_req = { };
>         int ret;
>
> -       info = drm_format_info(format);
> -       fb_req.bpp = drm_format_info_bpp(info, 0);
> -       fb_req.depth = info->depth;
>         fb_req.width = width;
>         fb_req.height = height;
> -       fb_req.handle = handle;
> -       fb_req.pitch = buffer->pitch;
> +       fb_req.pixel_format = format;
> +       fb_req.handles[0] = handle;
> +       fb_req.pitches[0] = buffer->pitch;
>
> -       ret = drm_mode_addfb(client->dev, &fb_req, client->file);
> +       ret = drm_mode_addfb2(client->dev, &fb_req, client->file);
>         if (ret)
>                 return ret;

This should explicitly set the LINEAR modifier (and the modifier flag)
if the driver supports modifiers.

Cheers,
Daniel

Re: [PATCH v2 5/8] drm/client: Convert drm_client_buffer_addfb() to drm_mode_addfb2()

[Geert Uytterhoeven]

Hi Daniel,

On Thu, Aug 24, 2023 at 5:12 PM Daniel Stone <daniel@fooishbar.org> wrote:
> On Thu, 24 Aug 2023 at 16:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >         struct drm_client_dev *client = buffer->client;
> > -       struct drm_mode_fb_cmd fb_req = { };
> > -       const struct drm_format_info *info;
> > +       struct drm_mode_fb_cmd2 fb_req = { };
> >         int ret;
> >
> > -       info = drm_format_info(format);
> > -       fb_req.bpp = drm_format_info_bpp(info, 0);
> > -       fb_req.depth = info->depth;
> >         fb_req.width = width;
> >         fb_req.height = height;
> > -       fb_req.handle = handle;
> > -       fb_req.pitch = buffer->pitch;
> > +       fb_req.pixel_format = format;
> > +       fb_req.handles[0] = handle;
> > +       fb_req.pitches[0] = buffer->pitch;
> >
> > -       ret = drm_mode_addfb(client->dev, &fb_req, client->file);
> > +       ret = drm_mode_addfb2(client->dev, &fb_req, client->file);
> >         if (ret)
> >                 return ret;
>
> This should explicitly set the LINEAR modifier (and the modifier flag)
> if the driver supports modifiers.

Thanks for your comment!

I have no idea how to do that, and I do not know what the impact
would be.  All I know is that the current implementation of
drm_client_buffer_addfb() does not do that, so changing that in this
patch would mean that this would no longer be a trivial conversion.

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 2/8] drm/ssd130x: Fix screen clearing

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

Thanks a lot for your patch.

> Due to the reuse of buffers, ssd130x_clear_screen() no longers clears
> the screen, but merely redraws the last image that is residing in the
> intermediate buffer.
>
> As there is no point in clearing the intermediate buffer and transposing
> an all-black image, fix this by just clearing the HW format buffer, and
> writing it to the panel.
>
> Fixes: 49d7d581ceaf4cf8 ("drm/ssd130x: Don't allocate buffers on each plane update")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> ssd130x_clear_screen() is only called from
> ssd130x_primary_plane_helper_atomic_disable(), but this never happens on
> my system.
>

AFAIU this should be called if the outputs get disabled.

> Tested by adding some extra calls to ssd130x_clear_screen() at regular
> intervals.
>
> v2:
>   - New.
> ---
>  drivers/gpu/drm/solomon/ssd130x.c | 47 +++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 8 deletions(-)
>

The change makes sense to me.

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 3/8] drm/ssd130x: Use bool for ssd130x_deviceinfo flags

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> The .need_pwm and .need_chargepump fields in struct ssd130x_deviceinfo
> are flags that can have only two possible values: 0 and 1.
> Reduce kernel size by changing their types from int to bool.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> The native display format is monochrome light-on-dark (R1).
> Hence add support for R1, so monochrome applications not only look
> better, but also avoid the overhead of back-and-forth conversions
> between R1 and XR24.
>
> Do not allocate the intermediate conversion buffer when it is not
> needed, and reorder the two buffer allocations to streamline operation.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> v2:
>   - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use
>     shadow-buffer helpers when managing plane's state") in drm/drm-next.
>     Hence I did not add Javier's tags given on v1.
>   - Do not allocate intermediate buffer when not needed.
> ---

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 5/8] drm/client: Convert drm_client_buffer_addfb() to drm_mode_addfb2()

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert and Daniel,

> Hi Daniel,
>
> On Thu, Aug 24, 2023 at 5:12 PM Daniel Stone <daniel@fooishbar.org> wrote:
>> On Thu, 24 Aug 2023 at 16:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> >         struct drm_client_dev *client = buffer->client;
>> > -       struct drm_mode_fb_cmd fb_req = { };
>> > -       const struct drm_format_info *info;
>> > +       struct drm_mode_fb_cmd2 fb_req = { };
>> >         int ret;
>> >
>> > -       info = drm_format_info(format);
>> > -       fb_req.bpp = drm_format_info_bpp(info, 0);
>> > -       fb_req.depth = info->depth;
>> >         fb_req.width = width;
>> >         fb_req.height = height;
>> > -       fb_req.handle = handle;
>> > -       fb_req.pitch = buffer->pitch;
>> > +       fb_req.pixel_format = format;
>> > +       fb_req.handles[0] = handle;
>> > +       fb_req.pitches[0] = buffer->pitch;
>> >
>> > -       ret = drm_mode_addfb(client->dev, &fb_req, client->file);
>> > +       ret = drm_mode_addfb2(client->dev, &fb_req, client->file);
>> >         if (ret)
>> >                 return ret;
>>
>> This should explicitly set the LINEAR modifier (and the modifier flag)
>> if the driver supports modifiers.
>
> Thanks for your comment!
>
> I have no idea how to do that, and I do not know what the impact
> would be.  All I know is that the current implementation of
> drm_client_buffer_addfb() does not do that, so changing that in this
> patch would mean that this would no longer be a trivial conversion.
>

Agree with Geert, this patch basically just inlines the drm_mode_addfb()
implementation which already calls drm_mode_addfb2() but without setting
a struct drm_mode_fb_cmd2 .modifier field or anything modififers related.

So if that is wrong then it should be done as a follow-up patch (which
should also fix the drm_mode_addfb() helper implementation) ?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 2/8] drm/ssd130x: Fix screen clearing

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

[...]

> The change makes sense to me.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

I've also tested this patch now and found no regressions.

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>
>> The native display format is monochrome light-on-dark (R1).
>> Hence add support for R1, so monochrome applications not only look
>> better, but also avoid the overhead of back-and-forth conversions
>> between R1 and XR24.
>>
>> Do not allocate the intermediate conversion buffer when it is not
>> needed, and reorder the two buffer allocations to streamline operation.
>>
>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> ---
>> v2:
>>   - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use
>>     shadow-buffer helpers when managing plane's state") in drm/drm-next.
>>     Hence I did not add Javier's tags given on v1.
>>   - Do not allocate intermediate buffer when not needed.
>> ---
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

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

> -- 
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 3/8] drm/ssd130x: Use bool for ssd130x_deviceinfo flags

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>
>> The .need_pwm and .need_chargepump fields in struct ssd130x_deviceinfo
>> are flags that can have only two possible values: 0 and 1.
>> Reduce kernel size by changing their types from int to bool.
>>
>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> ---
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 1/8] drm/dumb-buffers: Fix drm_mode_create_dumb() for bpp < 8

[Thomas Zimmermann]

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

Hi

Am 24.08.23 um 17:08 schrieb Geert Uytterhoeven:
> drm_mode_create_dumb() calculates the number of characters per pixel
> from the number of bits per pixel by rounding up, which is not correct
> as the actual value of cpp may be non-integer.  While we do not need to
> care here about complex formats like YUV, bpp < 8 is a valid use case.
> 
>    - The overflow check for the buffer width is not correct if bpp < 8.
>      However, it doesn't hurt, as widths larger than U32_MAX / 8 should
>      not happen for real anyway.  Add a comment to clarify.
>    - Calculating the stride from the number of characters per pixel is
>      not correct.  Fix this by calculating it from the number of bits per
>      pixel instead.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Tested-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> v2:
>    - Add Reviewed-by, Tested-by.
> ---
>   drivers/gpu/drm/drm_dumb_buffers.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> index 70032bba1c97e787..21a04c32a5e3d785 100644
> --- a/drivers/gpu/drm/drm_dumb_buffers.c
> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> @@ -71,10 +71,11 @@ int drm_mode_create_dumb(struct drm_device *dev,
>   	/* overflow checks for 32bit size calculations */
>   	if (args->bpp > U32_MAX - 8)
>   		return -EINVAL;
> +	/* Incorrect (especially if bpp < 8), but doesn't hurt much */
>   	cpp = DIV_ROUND_UP(args->bpp, 8);
>   	if (cpp > U32_MAX / args->width)
>   		return -EINVAL;

At first, I was confused by this. So I'd really prefer to outright 
delete this code. As you say, it's incorrect and doesn't add anything. 
The concept of cpp is somewhat wrong anyway.

> -	stride = cpp * args->width;
> +	stride = DIV_ROUND_UP(args->bpp * args->width, 8);

Do we need an overflow check for (bpp * width < U32_MAX) ?

Best regards
Thomas

>   	if (args->height > U32_MAX / stride)
>   		return -EINVAL;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v2 1/8] drm/dumb-buffers: Fix drm_mode_create_dumb() for bpp < 8

[Geert Uytterhoeven]

Hi Thomas,

On Thu, Aug 31, 2023 at 9:40 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 24.08.23 um 17:08 schrieb Geert Uytterhoeven:
> > drm_mode_create_dumb() calculates the number of characters per pixel
> > from the number of bits per pixel by rounding up, which is not correct
> > as the actual value of cpp may be non-integer.  While we do not need to
> > care here about complex formats like YUV, bpp < 8 is a valid use case.
> >
> >    - The overflow check for the buffer width is not correct if bpp < 8.
> >      However, it doesn't hurt, as widths larger than U32_MAX / 8 should
> >      not happen for real anyway.  Add a comment to clarify.
> >    - Calculating the stride from the number of characters per pixel is
> >      not correct.  Fix this by calculating it from the number of bits per
> >      pixel instead.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> > Tested-by: Javier Martinez Canillas <javierm@redhat.com>
> > ---
> > v2:
> >    - Add Reviewed-by, Tested-by.
> > ---
> >   drivers/gpu/drm/drm_dumb_buffers.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> > index 70032bba1c97e787..21a04c32a5e3d785 100644
> > --- a/drivers/gpu/drm/drm_dumb_buffers.c
> > +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> > @@ -71,10 +71,11 @@ int drm_mode_create_dumb(struct drm_device *dev,
> >       /* overflow checks for 32bit size calculations */
> >       if (args->bpp > U32_MAX - 8)
> >               return -EINVAL;
> > +     /* Incorrect (especially if bpp < 8), but doesn't hurt much */
> >       cpp = DIV_ROUND_UP(args->bpp, 8);
> >       if (cpp > U32_MAX / args->width)
> >               return -EINVAL;
>
> At first, I was confused by this.

Me too ;-)

> So I'd really prefer to outright
> delete this code. As you say, it's incorrect and doesn't add anything.
> The concept of cpp is somewhat wrong anyway.
>
> > -     stride = cpp * args->width;
> > +     stride = DIV_ROUND_UP(args->bpp * args->width, 8);
>
> Do we need an overflow check for (bpp * width < U32_MAX) ?

I think I thought it would be covered by the above, but on more thought,
that is indeed not true.

So perhaps drop the cpp code above, and test bpp instead:

    -        cpp = DIV_ROUND_UP(args->bpp, 8);
    -        if (cpp > U32_MAX / args->width)
    +        if (args->bpp > U32_MAX / args->width)
                     return -EINVAL;

That would preclude a very wide buffer with height 1, though.

BTW, in v1 I also had this question:

    Why is drm_mode_create_dumb.size __u64?  The test for "args->height >
    U32_MAX / stride" rejects all sizes not fitting in __u32 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 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Thomas Zimmermann]

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

Hi

Am 24.08.23 um 17:08 schrieb Geert Uytterhoeven:
> The native display format is monochrome light-on-dark (R1).
> Hence add support for R1, so monochrome applications not only look
> better, but also avoid the overhead of back-and-forth conversions
> between R1 and XR24.
> 
> Do not allocate the intermediate conversion buffer when it is not
> needed, and reorder the two buffer allocations to streamline operation.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> v2:
>    - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use
>      shadow-buffer helpers when managing plane's state") in drm/drm-next.
>      Hence I did not add Javier's tags given on v1.
>    - Do not allocate intermediate buffer when not needed.
> ---
>   drivers/gpu/drm/solomon/ssd130x.c | 76 +++++++++++++++++++++----------
>   1 file changed, 51 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 78272b1f9d5b167f..18007cb4f3de5aa1 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -449,15 +449,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
>   
>   static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
>   			       struct ssd130x_plane_state *ssd130x_state,
> +			       u8 *buf, unsigned int pitch,
>   			       struct drm_rect *rect)
>   {
>   	unsigned int x = rect->x1;
>   	unsigned int y = rect->y1;
> -	u8 *buf = ssd130x_state->buffer;
>   	u8 *data_array = ssd130x_state->data_array;
>   	unsigned int width = drm_rect_width(rect);
>   	unsigned int height = drm_rect_height(rect);
> -	unsigned int line_length = DIV_ROUND_UP(width, 8);
>   	unsigned int page_height = ssd130x->device_info->page_height;
>   	unsigned int pages = DIV_ROUND_UP(height, page_height);
>   	struct drm_device *drm = &ssd130x->drm;
> @@ -516,7 +515,7 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
>   			u8 data = 0;
>   
>   			for (k = 0; k < m; k++) {
> -				u8 byte = buf[(8 * i + k) * line_length + j / 8];
> +				u8 byte = buf[(8 * i + k) * pitch + j / 8];
>   				u8 bit = (byte >> (j % 8)) & 1;
>   
>   				data |= bit << k;
> @@ -602,27 +601,51 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,

The handing of different formats in this function is confusing. I'd 
rather implement ssd130x_fb_blit_rect_r1 and 
ssd130x_fb_blit_rect_xrgb8888 which then handle a single format.

>   	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
>   	unsigned int page_height = ssd130x->device_info->page_height;
>   	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
> -	u8 *buf = ssd130x_state->buffer;
>   	struct iosys_map dst;
>   	unsigned int dst_pitch;
>   	int ret = 0;
> +	u8 *buf;
>   
>   	/* Align y to display page boundaries */
>   	rect->y1 = round_down(rect->y1, page_height);
>   	rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
>   
> -	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> +	switch (fb->format->format) {
> +	case DRM_FORMAT_R1:
> +		/* Align x to byte boundaries */
> +		rect->x1 = round_down(rect->x1, 8);
> +		rect->x2 = round_up(rect->x2, 8);

Is rounding to multiples of 8 guaranteed to work? I know it did on 
VGA-compatible HW, because VGA enforces it. But is that generally the case?

>   
> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> -	if (ret)
> -		return ret;
> +		ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +		if (ret)
> +			return ret;
> +
> +		dst_pitch = fb->pitches[0];
> +		buf = vmap[0].vaddr + rect->y1 * dst_pitch + rect->x1 / 8;
> +
> +		ssd130x_update_rect(ssd130x, ssd130x_state, buf, dst_pitch,
> +				    rect);
> +
> +		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +		break;
> +
> +	case DRM_FORMAT_XRGB8888:
> +		dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> +		buf = ssd130x_state->buffer;
> +
> +		ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +		if (ret)
> +			return ret;
>   
> -	iosys_map_set_vaddr(&dst, buf);
> -	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
> +		iosys_map_set_vaddr(&dst, buf);
> +		drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
>   
> -	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>   
> -	ssd130x_update_rect(ssd130x, ssd130x_state, rect);
> +		ssd130x_update_rect(ssd130x, ssd130x_state, buf, dst_pitch,
> +				    rect);
> +		break;
> +	}
>   
>   	return ret;
>   }
> @@ -644,22 +667,24 @@ static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
>   	if (ret)
>   		return ret;
>   
> -	fi = drm_format_info(DRM_FORMAT_R1);
> -	if (!fi)
> -		return -EINVAL;
> +	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> +	if (!ssd130x_state->data_array)
> +		return -ENOMEM;
>   
> -	pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> +	if (plane_state->fb->format->format != DRM_FORMAT_R1) {
> +		fi = drm_format_info(DRM_FORMAT_R1);
> +		if (!fi)
> +			return -EINVAL;
>   
> -	ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> -	if (!ssd130x_state->buffer)
> -		return -ENOMEM;
> +		pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
>   
> -	ssd130x_state->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
> -	if (!ssd130x_state->data_array) {
> -		kfree(ssd130x_state->buffer);
> -		/* Set to prevent a double free in .atomic_destroy_state() */
> -		ssd130x_state->buffer = NULL;
> -		return -ENOMEM;
> +		ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> +		if (!ssd130x_state->buffer) {
> +			kfree(ssd130x_state->data_array);
> +			/* Set to prevent a double free in .atomic_destroy_state() */
> +			ssd130x_state->data_array = NULL;
> +			return -ENOMEM;
> +		}
>   	}
>   
>   	return 0;
> @@ -898,6 +923,7 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = {
>   };
>   
>   static const uint32_t ssd130x_formats[] = {
> +	DRM_FORMAT_R1,
>   	DRM_FORMAT_XRGB8888,
>   };
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v2 5/8] drm/client: Convert drm_client_buffer_addfb() to drm_mode_addfb2()

[Thomas Zimmermann]

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

Hi

Am 24.08.23 um 17:22 schrieb Geert Uytterhoeven:
> Hi Daniel,
> 
> On Thu, Aug 24, 2023 at 5:12 PM Daniel Stone <daniel@fooishbar.org> wrote:
>> On Thu, 24 Aug 2023 at 16:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>          struct drm_client_dev *client = buffer->client;
>>> -       struct drm_mode_fb_cmd fb_req = { };
>>> -       const struct drm_format_info *info;
>>> +       struct drm_mode_fb_cmd2 fb_req = { };
>>>          int ret;
>>>
>>> -       info = drm_format_info(format);
>>> -       fb_req.bpp = drm_format_info_bpp(info, 0);
>>> -       fb_req.depth = info->depth;
>>>          fb_req.width = width;
>>>          fb_req.height = height;
>>> -       fb_req.handle = handle;
>>> -       fb_req.pitch = buffer->pitch;
>>> +       fb_req.pixel_format = format;
>>> +       fb_req.handles[0] = handle;
>>> +       fb_req.pitches[0] = buffer->pitch;
>>>
>>> -       ret = drm_mode_addfb(client->dev, &fb_req, client->file);
>>> +       ret = drm_mode_addfb2(client->dev, &fb_req, client->file);
>>>          if (ret)
>>>                  return ret;
>>
>> This should explicitly set the LINEAR modifier (and the modifier flag)
>> if the driver supports modifiers.
> 
> Thanks for your comment!
> 
> I have no idea how to do that, and I do not know what the impact
> would be.  All I know is that the current implementation of
> drm_client_buffer_addfb() does not do that, so changing that in this
> patch would mean that this would no longer be a trivial conversion.

I agree. That change is too large and invasive for this patchset. Rather 
add a comment or TODO item.

The correct way for adding the modifier is to extend the 
drm_client_buffer_addfb() and pass in the information from the caller. 
That caller is fbdev code. We currently detect the format from (bpp, 
depth) values and forward the format to _addfb(). [1] Here we'd have to 
check the driver for a modifier as well and forward it to _addfb()

[1] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_fbdev_generic.c#L88

The pair of (bpp, depth) values comes from __drm_fb_helper_find_sizes, 
[2] which looks through the planes' formats and tries to find something 
that fits the user's requested color mode. It would have to look for 
modifiers as well. At some point, I want the helper to return formats 
directly, but that's still a bit away.

[2] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_fb_helper.c#L1504

Best regards
Thomas

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v2 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Thomas,

On Thu, Aug 31, 2023 at 10:01 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 24.08.23 um 17:08 schrieb Geert Uytterhoeven:
> > The native display format is monochrome light-on-dark (R1).
> > Hence add support for R1, so monochrome applications not only look
> > better, but also avoid the overhead of back-and-forth conversions
> > between R1 and XR24.
> >
> > Do not allocate the intermediate conversion buffer when it is not
> > needed, and reorder the two buffer allocations to streamline operation.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > v2:
> >    - Rework on top op commit 8c3926367ac9df6c ("drm/ssd130x: Use
> >      shadow-buffer helpers when managing plane's state") in drm/drm-next.
> >      Hence I did not add Javier's tags given on v1.
> >    - Do not allocate intermediate buffer when not needed.
> > ---
> >   drivers/gpu/drm/solomon/ssd130x.c | 76 +++++++++++++++++++++----------
> >   1 file changed, 51 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> > index 78272b1f9d5b167f..18007cb4f3de5aa1 100644
> > --- a/drivers/gpu/drm/solomon/ssd130x.c
> > +++ b/drivers/gpu/drm/solomon/ssd130x.c
> > @@ -449,15 +449,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
> >
> >   static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
> >                              struct ssd130x_plane_state *ssd130x_state,
> > +                            u8 *buf, unsigned int pitch,
> >                              struct drm_rect *rect)
> >   {
> >       unsigned int x = rect->x1;
> >       unsigned int y = rect->y1;
> > -     u8 *buf = ssd130x_state->buffer;
> >       u8 *data_array = ssd130x_state->data_array;
> >       unsigned int width = drm_rect_width(rect);
> >       unsigned int height = drm_rect_height(rect);
> > -     unsigned int line_length = DIV_ROUND_UP(width, 8);
> >       unsigned int page_height = ssd130x->device_info->page_height;
> >       unsigned int pages = DIV_ROUND_UP(height, page_height);
> >       struct drm_device *drm = &ssd130x->drm;
> > @@ -516,7 +515,7 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
> >                       u8 data = 0;
> >
> >                       for (k = 0; k < m; k++) {
> > -                             u8 byte = buf[(8 * i + k) * line_length + j / 8];
> > +                             u8 byte = buf[(8 * i + k) * pitch + j / 8];
> >                               u8 bit = (byte >> (j % 8)) & 1;
> >
> >                               data |= bit << k;
> > @@ -602,27 +601,51 @@ static int ssd130x_fb_blit_rect(struct drm_plane_state *state,
>
> The handing of different formats in this function is confusing. I'd
> rather implement ssd130x_fb_blit_rect_r1 and
> ssd130x_fb_blit_rect_xrgb8888 which then handle a single format.

OK, will split.

> >       struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
> >       unsigned int page_height = ssd130x->device_info->page_height;
> >       struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(state);
> > -     u8 *buf = ssd130x_state->buffer;
> >       struct iosys_map dst;
> >       unsigned int dst_pitch;
> >       int ret = 0;
> > +     u8 *buf;
> >
> >       /* Align y to display page boundaries */
> >       rect->y1 = round_down(rect->y1, page_height);
> >       rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
> >
> > -     dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> > +     switch (fb->format->format) {
> > +     case DRM_FORMAT_R1:
> > +             /* Align x to byte boundaries */
> > +             rect->x1 = round_down(rect->x1, 8);
> > +             rect->x2 = round_up(rect->x2, 8);
>
> Is rounding to multiples of 8 guaranteed to work? I know it did on
> VGA-compatible HW, because VGA enforces it. But is that generally the case?

That depends: some hardware requires e.g. 32-bit writes.
But this driver is for a display with a serial (i2c/spi) interface,
so everything should be fine ;-)

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 6/8] drm/fb-helper: Pass buffer format via drm_fb_helper_surface_size

[Thomas Zimmermann]

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

Hi

Am 24.08.23 um 17:08 schrieb Geert Uytterhoeven:
> drm_fb_helper_single_fb_probe() first calls drm_fb_helper_find_sizes(),
> followed by drm_fbdev_generic_helper_fb_probe():
>    - The former tries to find a suitable buffer format, taking into
>      account limitations of the whole display pipeline,
>    - The latter just calls drm_mode_legacy_fb_format() again.
> 
> Simplify this by passing the buffer format between these functions
> via a new buffer format member in the drm_fb_helper_surface_size
> structure.

That is a bit premature and I'd like to not merge this patch.

A number of drivers implement fbdev emulation with their own workarounds 
for surface_bpp and surface_depth. My plan has been to push 
_find_sizes() into the drivers' fb_probe helpers and let them handle 
everything. Once everything has been cleaned up, surface_bpp and 
surface_depth can hopefully be removed entirely.

Best regards
Thomas

> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Tested-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> v2:
>    - Fix accidental debug level increase,
>    - Add Reviewed-by, Tested-by.
> ---
>   drivers/gpu/drm/drm_fb_helper.c     | 1 +
>   drivers/gpu/drm/drm_fbdev_generic.c | 9 ++++-----
>   include/drm/drm_fb_helper.h         | 2 ++
>   3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d612133e2cf7ec99..4dc28fdcc1e0a6a4 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1564,6 +1564,7 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper,
>   	info = drm_format_info(surface_format);
>   	sizes->surface_bpp = drm_format_info_bpp(info, 0);
>   	sizes->surface_depth = info->depth;
> +	sizes->surface_format = surface_format;
>   
>   	/* first up get a count of crtcs now in use and new min/maxes width/heights */
>   	crtc_count = 0;
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index d647d89764cb9894..3830d25bcc3ad035 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -77,16 +77,15 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
>   	struct fb_info *info;
>   	size_t screen_size;
>   	void *screen_buffer;
> -	u32 format;
>   	int ret;
>   
> -	drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
> +	drm_dbg_kms(dev, "surface width(%d), height(%d), bpp(%d) and format(%p4cc)\n",
>   		    sizes->surface_width, sizes->surface_height,
> -		    sizes->surface_bpp);
> +		    sizes->surface_bpp, &sizes->surface_format);
>   
> -	format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
>   	buffer = drm_client_framebuffer_create(client, sizes->surface_width,
> -					       sizes->surface_height, format);
> +					       sizes->surface_height,
> +					       sizes->surface_format);
>   	if (IS_ERR(buffer))
>   		return PTR_ERR(buffer);
>   
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 375737fd6c36ed19..aa3d62a531d12f37 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -45,6 +45,7 @@ struct drm_fb_helper;
>    * @surface_height: scanout buffer height
>    * @surface_bpp: scanout buffer bpp
>    * @surface_depth: scanout buffer depth
> + * @surface_format: scanout buffer format (optional)
>    *
>    * Note that the scanout surface width/height may be larger than the fbdev
>    * width/height.  In case of multiple displays, the scanout surface is sized
> @@ -61,6 +62,7 @@ struct drm_fb_helper_surface_size {
>   	u32 surface_height;
>   	u32 surface_bpp;
>   	u32 surface_depth;
> +	u32 surface_format;
>   };
>   
>   /**

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v2 7/8] drm/fb-helper: Add support for DRM_FORMAT_R1

[Thomas Zimmermann]

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

Hi

Am 24.08.23 um 17:08 schrieb Geert Uytterhoeven:
> Add support for the monochrome light-on-dark buffer format (R1) to the
> fb helper, so this format can be used for fbdev emulation and for the
> text console.  This avoids the overhead of using XR24 and the associated
> conversions on display hardware that supports only a simple monochrome
> format.
> 
> R1 is very similar to C1 (monochrome indexed color), and shares the same
> depth and bpp.  As drm_mode_legacy_fb_format() returns a format based on
> only depth and bpp, it cannot distinguish between R1 and C1.  Hence

Could we rather add another parameter to drm_mode_legacy_fb_format(); 
say 'bool indexed'. If set to true, it prefers _Cx formats, otherwise _Rx.

The parameter would be 'true' for most calls; except in 
drm_fb_helper_find_format(). There, we can go through the array of given 
formats and check if there's one that has is_color_indexed set. If so, 
we specify true, if not we specify false.

Best regards
Thomas

> drm_fb_helper_find_format() is modified to try to fall back to R1 if C1
> is not supported.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Tested-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> v2:
>    - Add Reviewed-by, Tested-by.
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 41 ++++++++++++++++++++++++---------
>   1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 4dc28fdcc1e0a6a4..71baf8597516e9fd 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1130,7 +1130,7 @@ static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
>   {
>   	u8 depth = format->depth;
>   
> -	if (format->is_color_indexed) {
> +	if (format->format == DRM_FORMAT_R1 || format->is_color_indexed) {
>   		var->red.offset = 0;
>   		var->green.offset = 0;
>   		var->blue.offset = 0;
> @@ -1236,6 +1236,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>   	case DRM_FORMAT_C1:
>   	case DRM_FORMAT_C2:
>   	case DRM_FORMAT_C4:
> +	case DRM_FORMAT_R1:
>   		/* supported format with sub-byte pixels */
>   		break;
>   
> @@ -1439,12 +1440,24 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
>   }
>   EXPORT_SYMBOL(drm_fb_helper_pan_display);
>   
> +static bool is_supported_format(uint32_t format, const uint32_t *formats,
> +				size_t format_count)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < format_count; ++i) {
> +		if (formats[i] == format)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>   static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const uint32_t *formats,
>   					  size_t format_count, uint32_t bpp, uint32_t depth)
>   {
>   	struct drm_device *dev = fb_helper->dev;
>   	uint32_t format;
> -	size_t i;
>   
>   	/*
>   	 * Do not consider YUV or other complicated formats
> @@ -1457,10 +1470,12 @@ static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const
>   	if (!format)
>   		goto err;
>   
> -	for (i = 0; i < format_count; ++i) {
> -		if (formats[i] == format)
> -			return format;
> -	}
> +	if (is_supported_format(format, formats, format_count))
> +		return format;
> +
> +	if (format == DRM_FORMAT_C1 &&
> +	    is_supported_format(DRM_FORMAT_R1, formats, format_count))
> +		return DRM_FORMAT_R1;
>   
>   err:
>   	/* We found nothing. */
> @@ -1680,11 +1695,15 @@ 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,
> -				   bool is_color_indexed)
> +				   const struct drm_format_info *format)
>   {
>   	info->fix.type = FB_TYPE_PACKED_PIXELS;
> -	info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
> -					    : FB_VISUAL_TRUECOLOR;
> +	if (format->format == DRM_FORMAT_R1)
> +		info->fix.visual = FB_VISUAL_MONO10;
> +	else if (format->is_color_indexed)
> +		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
> +	else
> +		info->fix.visual = FB_VISUAL_TRUECOLOR;
>   	info->fix.mmio_start = 0;
>   	info->fix.mmio_len = 0;
>   	info->fix.type_aux = 0;
> @@ -1707,6 +1726,7 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
>   	case DRM_FORMAT_C1:
>   	case DRM_FORMAT_C2:
>   	case DRM_FORMAT_C4:
> +	case DRM_FORMAT_R1:
>   		/* supported format with sub-byte pixels */
>   		break;
>   
> @@ -1747,8 +1767,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->is_color_indexed);
> +	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->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
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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