[PATCH 0/6] drm/gud: Use the shadow plane helper

[PATCH 0/6] drm/gud: Use the shadow plane helper

[Noralf Trønnes via B4 Submission Endpoint]

From: Noralf Trønnes <noralf@tronnes.org>

Hi,

I have started to look at igt for testing and want to use CRC tests. To
implement support for this I need to move away from the simple kms
helper.

When looking around for examples I came across Thomas' nice shadow
helper and thought, yes this is perfect for drm/gud. So I'll switch to
that before I move away from the simple kms helper.

The async framebuffer flushing code path now uses a shadow buffer and
doesn't touch the framebuffer when it shouldn't. I have also taken the
opportunity to inline the synchronous flush code path since this will be
the future default when userspace predominently don't run all displays
in the same rendering loop. A shared rendering loop slows down all
displays to run at the speed of the slowest one.

Noralf.

Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

---
Noralf Trønnes (6):
      drm/gem: shadow_fb_access: Prepare imported buffers for CPU access
      drm/gud: Fix UBSAN warning
      drm/gud: Don't retry a failed framebuffer flush
      drm/gud: Split up gud_flush_work()
      drm/gud: Prepare buffer for CPU access in gud_flush_work()
      drm/gud: Use the shadow plane helper

 drivers/gpu/drm/drm_gem_atomic_helper.c |  13 ++-
 drivers/gpu/drm/gud/gud_drv.c           |   1 +
 drivers/gpu/drm/gud/gud_internal.h      |   1 +
 drivers/gpu/drm/gud/gud_pipe.c          | 198 +++++++++++++++-----------------
 drivers/gpu/drm/solomon/ssd130x.c       |  10 +-
 drivers/gpu/drm/tiny/gm12u320.c         |  10 +-
 drivers/gpu/drm/tiny/ofdrm.c            |  10 +-
 drivers/gpu/drm/tiny/simpledrm.c        |  10 +-
 drivers/gpu/drm/udl/udl_modeset.c       |  11 +-
 9 files changed, 117 insertions(+), 147 deletions(-)
---
base-commit: 7257702951305b1f0259c3468c39fc59d1ad4d8b
change-id: 20221122-gud-shadow-plane-ae37a95d4d8d

Best regards,
-- 
Noralf Trønnes <noralf@tronnes.org>

[PATCH 1/6] drm/gem: shadow_fb_access: Prepare imported buffers for CPU access

[Noralf Trønnes via B4 Submission Endpoint]

From: Noralf Trønnes <noralf@tronnes.org>

Complete the shadow fb access functions by also preparing imported buffers
for CPU access. Update the affected drivers that currently use
drm_gem_fb_begin_cpu_access().

Through this change the following SHMEM drivers will now also make sure
their imported buffers are prepared for CPU access: cirrus, hyperv,
mgag200, vkms

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_gem_atomic_helper.c | 13 ++++++++++++-
 drivers/gpu/drm/solomon/ssd130x.c       | 10 +---------
 drivers/gpu/drm/tiny/gm12u320.c         | 10 +---------
 drivers/gpu/drm/tiny/ofdrm.c            | 10 ++--------
 drivers/gpu/drm/tiny/simpledrm.c        | 10 ++--------
 drivers/gpu/drm/udl/udl_modeset.c       | 11 ++---------
 6 files changed, 20 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index e42800718f51..0eef4bb30d25 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -368,6 +368,7 @@ EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
  * maps all buffer objects of the plane's framebuffer into kernel address
  * space and stores them in struct &drm_shadow_plane_state.map. The first data
  * bytes are available in struct &drm_shadow_plane_state.data.
+ * It also prepares imported buffers for CPU access.
  *
  * See drm_gem_end_shadow_fb_access() for cleanup.
  *
@@ -378,11 +379,20 @@ int drm_gem_begin_shadow_fb_access(struct drm_plane *plane, struct drm_plane_sta
 {
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_framebuffer *fb = plane_state->fb;
+	int ret;
 
 	if (!fb)
 		return 0;
 
-	return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
+	ret = drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
+	if (ret)
+		return ret;
+
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
+		drm_gem_fb_vunmap(fb, shadow_plane_state->map);
+
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_begin_shadow_fb_access);
 
@@ -404,6 +414,7 @@ void drm_gem_end_shadow_fb_access(struct drm_plane *plane, struct drm_plane_stat
 	if (!fb)
 		return;
 
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 	drm_gem_fb_vunmap(fb, shadow_plane_state->map);
 }
 EXPORT_SYMBOL(drm_gem_end_shadow_fb_access);
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 53464afc2b9a..58a2f0113f24 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -544,7 +544,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
 	struct iosys_map dst;
 	unsigned int dst_pitch;
-	int ret = 0;
 	u8 *buf = NULL;
 
 	/* Align y to display page boundaries */
@@ -556,21 +555,14 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	if (!buf)
 		return -ENOMEM;
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		goto out_free;
-
 	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);
-
 	ssd130x_update_rect(ssd130x, buf, rect);
 
-out_free:
 	kfree(buf);
 
-	return ret;
+	return 0;
 }
 
 static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index 130fd07a967d..59aad4b468cc 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -252,7 +252,7 @@ static void gm12u320_32bpp_to_24bpp_packed(u8 *dst, u8 *src, int len)
 
 static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
 {
-	int block, dst_offset, len, remain, ret, x1, x2, y1, y2;
+	int block, dst_offset, len, remain, x1, x2, y1, y2;
 	struct drm_framebuffer *fb;
 	void *vaddr;
 	u8 *src;
@@ -269,12 +269,6 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
 	y2 = gm12u320->fb_update.rect.y2;
 	vaddr = gm12u320->fb_update.src_map.vaddr; /* TODO: Use mapping abstraction properly */
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret) {
-		GM12U320_ERR("drm_gem_fb_begin_cpu_access err: %d\n", ret);
-		goto put_fb;
-	}
-
 	src = vaddr + y1 * fb->pitches[0] + x1 * 4;
 
 	x1 += (GM12U320_REAL_WIDTH - GM12U320_USER_WIDTH) / 2;
@@ -309,8 +303,6 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
 		src += fb->pitches[0];
 	}
 
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-put_fb:
 	drm_framebuffer_put(fb);
 	gm12u320->fb_update.fb = NULL;
 unlock:
diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
index dc9e4d71b12a..ed3072563db9 100644
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -820,14 +820,10 @@ static void ofdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	const struct drm_format_info *dst_format = odev->format;
 	struct drm_atomic_helper_damage_iter iter;
 	struct drm_rect damage;
-	int ret, idx;
-
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		return;
+	int idx;
 
 	if (!drm_dev_enter(dev, &idx))
-		goto out_drm_gem_fb_end_cpu_access;
+		return;
 
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
@@ -843,8 +839,6 @@ static void ofdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	}
 
 	drm_dev_exit(idx);
-out_drm_gem_fb_end_cpu_access:
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 }
 
 static void ofdrm_primary_plane_helper_atomic_disable(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 162eb44dcba8..1c0d9e277dc3 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -481,14 +481,10 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
 	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
 	struct drm_atomic_helper_damage_iter iter;
 	struct drm_rect damage;
-	int ret, idx;
-
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		return;
+	int idx;
 
 	if (!drm_dev_enter(dev, &idx))
-		goto out_drm_gem_fb_end_cpu_access;
+		return;
 
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
@@ -504,8 +500,6 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
 	}
 
 	drm_dev_exit(idx);
-out_drm_gem_fb_end_cpu_access:
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 }
 
 static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 4b79d44752c9..022b18aa3f48 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -271,17 +271,13 @@ static void udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
 	struct drm_atomic_helper_damage_iter iter;
 	struct drm_rect damage;
-	int ret, idx;
+	int idx;
 
 	if (!fb)
 		return; /* no framebuffer; plane is disabled */
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		return;
-
 	if (!drm_dev_enter(dev, &idx))
-		goto out_drm_gem_fb_end_cpu_access;
+		return;
 
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
@@ -289,9 +285,6 @@ static void udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	}
 
 	drm_dev_exit(idx);
-
-out_drm_gem_fb_end_cpu_access:
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 }
 
 static const struct drm_plane_helper_funcs udl_primary_plane_helper_funcs = {

-- 
b4 0.10.1

[PATCH 2/6] drm/gud: Fix UBSAN warning

[Noralf Trønnes via B4 Submission Endpoint]

From: Noralf Trønnes <noralf@tronnes.org>

UBSAN complains about invalid value for bool:

[  101.165172] [drm] Initialized gud 1.0.0 20200422 for 2-3.2:1.0 on minor 1
[  101.213360] gud 2-3.2:1.0: [drm] fb1: guddrmfb frame buffer device
[  101.213426] usbcore: registered new interface driver gud
[  101.989431] ================================================================================
[  101.989441] UBSAN: invalid-load in /home/pi/linux/include/linux/iosys-map.h:253:9
[  101.989447] load of value 121 is not a valid value for type '_Bool'
[  101.989451] CPU: 1 PID: 455 Comm: kworker/1:6 Not tainted 5.18.0-rc5-gud-5.18-rc5 #3
[  101.989456] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018
[  101.989459] Workqueue: events_long gud_flush_work [gud]
[  101.989471] Call Trace:
[  101.989474]  <TASK>
[  101.989479]  dump_stack_lvl+0x49/0x5f
[  101.989488]  dump_stack+0x10/0x12
[  101.989493]  ubsan_epilogue+0x9/0x3b
[  101.989498]  __ubsan_handle_load_invalid_value.cold+0x44/0x49
[  101.989504]  dma_buf_vmap.cold+0x38/0x3d
[  101.989511]  ? find_busiest_group+0x48/0x300
[  101.989520]  drm_gem_shmem_vmap+0x76/0x1b0 [drm_shmem_helper]
[  101.989528]  drm_gem_shmem_object_vmap+0x9/0xb [drm_shmem_helper]
[  101.989535]  drm_gem_vmap+0x26/0x60 [drm]
[  101.989594]  drm_gem_fb_vmap+0x47/0x150 [drm_kms_helper]
[  101.989630]  gud_prep_flush+0xc1/0x710 [gud]
[  101.989639]  ? _raw_spin_lock+0x17/0x40
[  101.989648]  gud_flush_work+0x1e0/0x430 [gud]
[  101.989653]  ? __switch_to+0x11d/0x470
[  101.989664]  process_one_work+0x21f/0x3f0
[  101.989673]  worker_thread+0x200/0x3e0
[  101.989679]  ? rescuer_thread+0x390/0x390
[  101.989684]  kthread+0xfd/0x130
[  101.989690]  ? kthread_complete_and_exit+0x20/0x20
[  101.989696]  ret_from_fork+0x22/0x30
[  101.989706]  </TASK>
[  101.989708] ================================================================================

The source of this warning is in iosys_map_clear() called from
dma_buf_vmap(). It conditionally sets values based on map->is_iomem. The
iosys_map variables are allocated uninitialized on the stack leading to
->is_iomem having all kinds of values and not only 0/1.

Fix this by zeroing the iosys_map variables.

Fixes: 40e1a70b4aed ("drm: Add GUD USB Display driver")
Cc: <stable@vger.kernel.org> # v5.18+
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/gud/gud_pipe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 7c6dc2bcd14a..61f4abaf1811 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -157,8 +157,8 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
 {
 	struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
 	u8 compression = gdrm->compression;
-	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
-	struct iosys_map map_data[DRM_FORMAT_MAX_PLANES];
+	struct iosys_map map[DRM_FORMAT_MAX_PLANES] = { };
+	struct iosys_map map_data[DRM_FORMAT_MAX_PLANES] = { };
 	struct iosys_map dst;
 	void *vaddr, *buf;
 	size_t pitch, len;

-- 
b4 0.10.1

[PATCH 3/6] drm/gud: Don't retry a failed framebuffer flush

[Noralf Trønnes via B4 Submission Endpoint]

From: Noralf Trønnes <noralf@tronnes.org>

If a framebuffer flush fails the driver will do one retry by requeing the
worker. Currently the worker is used even for synchronous flushing, but a
later patch will inline it, so this needs to change. Thinking about how to
solve this I came to the conclusion that this retry mechanism was a fix
for a problem that was only in the mind of the developer (me) and not
something that solved a real problem.

So let's remove this for now and revisit later should it become necessary.
gud_add_damage() has now only one caller so it can be inlined.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/gud/gud_pipe.c | 48 +++++++-----------------------------------
 1 file changed, 8 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 61f4abaf1811..ff1358815af5 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -333,37 +333,6 @@ void gud_clear_damage(struct gud_device *gdrm)
 	gdrm->damage.y2 = 0;
 }
 
-static void gud_add_damage(struct gud_device *gdrm, struct drm_rect *damage)
-{
-	gdrm->damage.x1 = min(gdrm->damage.x1, damage->x1);
-	gdrm->damage.y1 = min(gdrm->damage.y1, damage->y1);
-	gdrm->damage.x2 = max(gdrm->damage.x2, damage->x2);
-	gdrm->damage.y2 = max(gdrm->damage.y2, damage->y2);
-}
-
-static void gud_retry_failed_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
-				   struct drm_rect *damage)
-{
-	/*
-	 * pipe_update waits for the worker when the display mode is going to change.
-	 * This ensures that the width and height is still the same making it safe to
-	 * add back the damage.
-	 */
-
-	mutex_lock(&gdrm->damage_lock);
-	if (!gdrm->fb) {
-		drm_framebuffer_get(fb);
-		gdrm->fb = fb;
-	}
-	gud_add_damage(gdrm, damage);
-	mutex_unlock(&gdrm->damage_lock);
-
-	/* Retry only once to avoid a possible storm in case of continues errors. */
-	if (!gdrm->prev_flush_failed)
-		queue_work(system_long_wq, &gdrm->work);
-	gdrm->prev_flush_failed = true;
-}
-
 void gud_flush_work(struct work_struct *work)
 {
 	struct gud_device *gdrm = container_of(work, struct gud_device, work);
@@ -407,14 +376,10 @@ void gud_flush_work(struct work_struct *work)
 		ret = gud_flush_rect(gdrm, fb, format, &rect);
 		if (ret) {
 			if (ret != -ENODEV && ret != -ECONNRESET &&
-			    ret != -ESHUTDOWN && ret != -EPROTO) {
-				bool prev_flush_failed = gdrm->prev_flush_failed;
-
-				gud_retry_failed_flush(gdrm, fb, &damage);
-				if (!prev_flush_failed)
-					dev_err_ratelimited(fb->dev->dev,
-							    "Failed to flush framebuffer: error=%d\n", ret);
-			}
+			    ret != -ESHUTDOWN && ret != -EPROTO)
+				dev_err_ratelimited(fb->dev->dev,
+						    "Failed to flush framebuffer: error=%d\n", ret);
+			gdrm->prev_flush_failed = true;
 			break;
 		}
 
@@ -439,7 +404,10 @@ static void gud_fb_queue_damage(struct gud_device *gdrm, struct drm_framebuffer
 		gdrm->fb = fb;
 	}
 
-	gud_add_damage(gdrm, damage);
+	gdrm->damage.x1 = min(gdrm->damage.x1, damage->x1);
+	gdrm->damage.y1 = min(gdrm->damage.y1, damage->y1);
+	gdrm->damage.x2 = max(gdrm->damage.x2, damage->x2);
+	gdrm->damage.y2 = max(gdrm->damage.y2, damage->y2);
 
 	mutex_unlock(&gdrm->damage_lock);
 

-- 
b4 0.10.1

[PATCH 4/6] drm/gud: Split up gud_flush_work()

[Noralf Trønnes via B4 Submission Endpoint]

From: Noralf Trønnes <noralf@tronnes.org>

In preparation for inlining synchronous flushing split out the part of
gud_flush_work() that can be shared by the sync and async code paths.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/gud/gud_pipe.c | 72 +++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index ff1358815af5..d2af9947494f 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -333,15 +333,49 @@ void gud_clear_damage(struct gud_device *gdrm)
 	gdrm->damage.y2 = 0;
 }
 
+static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb,
+			     struct drm_rect *damage)
+{
+	const struct drm_format_info *format;
+	unsigned int i, lines;
+	size_t pitch;
+	int ret;
+
+	format = fb->format;
+	if (format->format == DRM_FORMAT_XRGB8888 && gdrm->xrgb8888_emulation_format)
+		format = gdrm->xrgb8888_emulation_format;
+
+	/* Split update if it's too big */
+	pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(damage));
+	lines = drm_rect_height(damage);
+
+	if (gdrm->bulk_len < lines * pitch)
+		lines = gdrm->bulk_len / pitch;
+
+	for (i = 0; i < DIV_ROUND_UP(drm_rect_height(damage), lines); i++) {
+		struct drm_rect rect = *damage;
+
+		rect.y1 += i * lines;
+		rect.y2 = min_t(u32, rect.y1 + lines, damage->y2);
+
+		ret = gud_flush_rect(gdrm, fb, format, &rect);
+		if (ret) {
+			if (ret != -ENODEV && ret != -ECONNRESET &&
+			    ret != -ESHUTDOWN && ret != -EPROTO)
+				dev_err_ratelimited(fb->dev->dev,
+						    "Failed to flush framebuffer: error=%d\n", ret);
+			gdrm->prev_flush_failed = true;
+			break;
+		}
+	}
+}
+
 void gud_flush_work(struct work_struct *work)
 {
 	struct gud_device *gdrm = container_of(work, struct gud_device, work);
-	const struct drm_format_info *format;
 	struct drm_framebuffer *fb;
 	struct drm_rect damage;
-	unsigned int i, lines;
-	int idx, ret = 0;
-	size_t pitch;
+	int idx;
 
 	if (!drm_dev_enter(&gdrm->drm, &idx))
 		return;
@@ -356,35 +390,7 @@ void gud_flush_work(struct work_struct *work)
 	if (!fb)
 		goto out;
 
-	format = fb->format;
-	if (format->format == DRM_FORMAT_XRGB8888 && gdrm->xrgb8888_emulation_format)
-		format = gdrm->xrgb8888_emulation_format;
-
-	/* Split update if it's too big */
-	pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(&damage));
-	lines = drm_rect_height(&damage);
-
-	if (gdrm->bulk_len < lines * pitch)
-		lines = gdrm->bulk_len / pitch;
-
-	for (i = 0; i < DIV_ROUND_UP(drm_rect_height(&damage), lines); i++) {
-		struct drm_rect rect = damage;
-
-		rect.y1 += i * lines;
-		rect.y2 = min_t(u32, rect.y1 + lines, damage.y2);
-
-		ret = gud_flush_rect(gdrm, fb, format, &rect);
-		if (ret) {
-			if (ret != -ENODEV && ret != -ECONNRESET &&
-			    ret != -ESHUTDOWN && ret != -EPROTO)
-				dev_err_ratelimited(fb->dev->dev,
-						    "Failed to flush framebuffer: error=%d\n", ret);
-			gdrm->prev_flush_failed = true;
-			break;
-		}
-
-		gdrm->prev_flush_failed = false;
-	}
+	gud_flush_damage(gdrm, fb, &damage);
 
 	drm_framebuffer_put(fb);
 out:

-- 
b4 0.10.1

[PATCH 5/6] drm/gud: Prepare buffer for CPU access in gud_flush_work()

[Noralf Trønnes via B4 Submission Endpoint]

From: Noralf Trønnes <noralf@tronnes.org>

In preparation for moving to the shadow plane helper prepare the
framebuffer for CPU access as early as possible.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/gud/gud_pipe.c | 67 +++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index d2af9947494f..dfada6eedc58 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -15,6 +15,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem.h>
+#include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_print.h>
 #include <drm/drm_rect.h>
@@ -152,32 +153,21 @@ static size_t gud_xrgb8888_to_color(u8 *dst, const struct drm_format_info *forma
 }
 
 static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
+			  const struct iosys_map *map, bool cached_reads,
 			  const struct drm_format_info *format, struct drm_rect *rect,
 			  struct gud_set_buffer_req *req)
 {
-	struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
 	u8 compression = gdrm->compression;
-	struct iosys_map map[DRM_FORMAT_MAX_PLANES] = { };
-	struct iosys_map map_data[DRM_FORMAT_MAX_PLANES] = { };
 	struct iosys_map dst;
 	void *vaddr, *buf;
 	size_t pitch, len;
-	int ret = 0;
 
 	pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(rect));
 	len = pitch * drm_rect_height(rect);
 	if (len > gdrm->bulk_len)
 		return -E2BIG;
 
-	ret = drm_gem_fb_vmap(fb, map, map_data);
-	if (ret)
-		return ret;
-
-	vaddr = map_data[0].vaddr;
-
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		goto vunmap;
+	vaddr = map[0].vaddr;
 retry:
 	if (compression)
 		buf = gdrm->compress_buf;
@@ -192,29 +182,27 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
 	if (format != fb->format) {
 		if (format->format == GUD_DRM_FORMAT_R1) {
 			len = gud_xrgb8888_to_r124(buf, format, vaddr, fb, rect);
-			if (!len) {
-				ret = -ENOMEM;
-				goto end_cpu_access;
-			}
+			if (!len)
+				return -ENOMEM;
 		} else if (format->format == DRM_FORMAT_R8) {
-			drm_fb_xrgb8888_to_gray8(&dst, NULL, map_data, fb, rect);
+			drm_fb_xrgb8888_to_gray8(&dst, NULL, map, fb, rect);
 		} else if (format->format == DRM_FORMAT_RGB332) {
-			drm_fb_xrgb8888_to_rgb332(&dst, NULL, map_data, fb, rect);
+			drm_fb_xrgb8888_to_rgb332(&dst, NULL, map, fb, rect);
 		} else if (format->format == DRM_FORMAT_RGB565) {
-			drm_fb_xrgb8888_to_rgb565(&dst, NULL, map_data, fb, rect,
+			drm_fb_xrgb8888_to_rgb565(&dst, NULL, map, fb, rect,
 						  gud_is_big_endian());
 		} else if (format->format == DRM_FORMAT_RGB888) {
-			drm_fb_xrgb8888_to_rgb888(&dst, NULL, map_data, fb, rect);
+			drm_fb_xrgb8888_to_rgb888(&dst, NULL, map, fb, rect);
 		} else {
 			len = gud_xrgb8888_to_color(buf, format, vaddr, fb, rect);
 		}
 	} else if (gud_is_big_endian() && format->cpp[0] > 1) {
-		drm_fb_swab(&dst, NULL, map_data, fb, rect, !import_attach);
-	} else if (compression && !import_attach && pitch == fb->pitches[0]) {
+		drm_fb_swab(&dst, NULL, map, fb, rect, cached_reads);
+	} else if (compression && cached_reads && pitch == fb->pitches[0]) {
 		/* can compress directly from the framebuffer */
 		buf = vaddr + rect->y1 * pitch;
 	} else {
-		drm_fb_memcpy(&dst, NULL, map_data, fb, rect);
+		drm_fb_memcpy(&dst, NULL, map, fb, rect);
 	}
 
 	memset(req, 0, sizeof(*req));
@@ -237,12 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
 		req->compressed_length = cpu_to_le32(complen);
 	}
 
-end_cpu_access:
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-vunmap:
-	drm_gem_fb_vunmap(fb, map);
-
-	return ret;
+	return 0;
 }
 
 struct gud_usb_bulk_context {
@@ -285,6 +268,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
 }
 
 static int gud_flush_rect(struct gud_device *gdrm, struct drm_framebuffer *fb,
+			  const struct iosys_map *map, bool cached_reads,
 			  const struct drm_format_info *format, struct drm_rect *rect)
 {
 	struct gud_set_buffer_req req;
@@ -293,7 +277,7 @@ static int gud_flush_rect(struct gud_device *gdrm, struct drm_framebuffer *fb,
 
 	drm_dbg(&gdrm->drm, "Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
 
-	ret = gud_prep_flush(gdrm, fb, format, rect, &req);
+	ret = gud_prep_flush(gdrm, fb, map, cached_reads, format, rect, &req);
 	if (ret)
 		return ret;
 
@@ -334,6 +318,7 @@ void gud_clear_damage(struct gud_device *gdrm)
 }
 
 static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb,
+			     const struct iosys_map *map, bool cached_reads,
 			     struct drm_rect *damage)
 {
 	const struct drm_format_info *format;
@@ -358,7 +343,7 @@ static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb
 		rect.y1 += i * lines;
 		rect.y2 = min_t(u32, rect.y1 + lines, damage->y2);
 
-		ret = gud_flush_rect(gdrm, fb, format, &rect);
+		ret = gud_flush_rect(gdrm, fb, map, cached_reads, format, &rect);
 		if (ret) {
 			if (ret != -ENODEV && ret != -ECONNRESET &&
 			    ret != -ESHUTDOWN && ret != -EPROTO)
@@ -373,9 +358,10 @@ static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb
 void gud_flush_work(struct work_struct *work)
 {
 	struct gud_device *gdrm = container_of(work, struct gud_device, work);
+	struct iosys_map gem_map = { }, fb_map = { };
 	struct drm_framebuffer *fb;
 	struct drm_rect damage;
-	int idx;
+	int idx, ret;
 
 	if (!drm_dev_enter(&gdrm->drm, &idx))
 		return;
@@ -390,8 +376,21 @@ void gud_flush_work(struct work_struct *work)
 	if (!fb)
 		goto out;
 
-	gud_flush_damage(gdrm, fb, &damage);
+	ret = drm_gem_fb_vmap(fb, &gem_map, &fb_map);
+	if (ret)
+		goto fb_put;
 
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
+		goto vunmap;
+
+	/* Imported buffers are assumed to be WriteCombined with uncached reads */
+	gud_flush_damage(gdrm, fb, &fb_map, !fb->obj[0]->import_attach, &damage);
+
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+vunmap:
+	drm_gem_fb_vunmap(fb, &gem_map);
+fb_put:
 	drm_framebuffer_put(fb);
 out:
 	drm_dev_exit(idx);

-- 
b4 0.10.1

[PATCH 6/6] drm/gud: Use the shadow plane helper

[Noralf Trønnes via B4 Submission Endpoint]

From: Noralf Trønnes <noralf@tronnes.org>

Use the shadow plane helper to take care of preparing the framebuffer for
CPU access. The synchronous flushing is now done inline without the use of
a worker. The async path now uses a shadow buffer to hold framebuffer
changes and it doesn't read the framebuffer behind userspace's back
anymore.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/gud/gud_drv.c      |  1 +
 drivers/gpu/drm/gud/gud_internal.h |  1 +
 drivers/gpu/drm/gud/gud_pipe.c     | 69 ++++++++++++++++++++++++--------------
 3 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
index d57dab104358..5aac7cda0505 100644
--- a/drivers/gpu/drm/gud/gud_drv.c
+++ b/drivers/gpu/drm/gud/gud_drv.c
@@ -365,6 +365,7 @@ static void gud_debugfs_init(struct drm_minor *minor)
 static const struct drm_simple_display_pipe_funcs gud_pipe_funcs = {
 	.check      = gud_pipe_check,
 	.update	    = gud_pipe_update,
+	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS
 };
 
 static const struct drm_mode_config_funcs gud_mode_config_funcs = {
diff --git a/drivers/gpu/drm/gud/gud_internal.h b/drivers/gpu/drm/gud/gud_internal.h
index e351a1f1420d..0d148a6f27aa 100644
--- a/drivers/gpu/drm/gud/gud_internal.h
+++ b/drivers/gpu/drm/gud/gud_internal.h
@@ -43,6 +43,7 @@ struct gud_device {
 	struct drm_framebuffer *fb;
 	struct drm_rect damage;
 	bool prev_flush_failed;
+	void *shadow_buf;
 };
 
 static inline struct gud_device *to_gud_device(struct drm_device *drm)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index dfada6eedc58..7686325f7ee7 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -358,10 +358,10 @@ static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb
 void gud_flush_work(struct work_struct *work)
 {
 	struct gud_device *gdrm = container_of(work, struct gud_device, work);
-	struct iosys_map gem_map = { }, fb_map = { };
+	struct iosys_map shadow_map;
 	struct drm_framebuffer *fb;
 	struct drm_rect damage;
-	int idx, ret;
+	int idx;
 
 	if (!drm_dev_enter(&gdrm->drm, &idx))
 		return;
@@ -369,6 +369,7 @@ void gud_flush_work(struct work_struct *work)
 	mutex_lock(&gdrm->damage_lock);
 	fb = gdrm->fb;
 	gdrm->fb = NULL;
+	iosys_map_set_vaddr(&shadow_map, gdrm->shadow_buf);
 	damage = gdrm->damage;
 	gud_clear_damage(gdrm);
 	mutex_unlock(&gdrm->damage_lock);
@@ -376,33 +377,33 @@ void gud_flush_work(struct work_struct *work)
 	if (!fb)
 		goto out;
 
-	ret = drm_gem_fb_vmap(fb, &gem_map, &fb_map);
-	if (ret)
-		goto fb_put;
+	gud_flush_damage(gdrm, fb, &shadow_map, true, &damage);
 
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret)
-		goto vunmap;
-
-	/* Imported buffers are assumed to be WriteCombined with uncached reads */
-	gud_flush_damage(gdrm, fb, &fb_map, !fb->obj[0]->import_attach, &damage);
-
-	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-vunmap:
-	drm_gem_fb_vunmap(fb, &gem_map);
-fb_put:
 	drm_framebuffer_put(fb);
 out:
 	drm_dev_exit(idx);
 }
 
-static void gud_fb_queue_damage(struct gud_device *gdrm, struct drm_framebuffer *fb,
-				struct drm_rect *damage)
+static int gud_fb_queue_damage(struct gud_device *gdrm, struct drm_framebuffer *fb,
+			       const struct iosys_map *map, struct drm_rect *damage)
 {
 	struct drm_framebuffer *old_fb = NULL;
+	struct iosys_map shadow_map;
 
 	mutex_lock(&gdrm->damage_lock);
 
+	if (!gdrm->shadow_buf) {
+		gdrm->shadow_buf = vzalloc(fb->pitches[0] * fb->height);
+		if (!gdrm->shadow_buf) {
+			mutex_unlock(&gdrm->damage_lock);
+			return -ENOMEM;
+		}
+	}
+
+	iosys_map_set_vaddr(&shadow_map, gdrm->shadow_buf);
+	iosys_map_incr(&shadow_map, drm_fb_clip_offset(fb->pitches[0], fb->format, damage));
+	drm_fb_memcpy(&shadow_map, fb->pitches, map, fb, damage);
+
 	if (fb != gdrm->fb) {
 		old_fb = gdrm->fb;
 		drm_framebuffer_get(fb);
@@ -420,6 +421,26 @@ static void gud_fb_queue_damage(struct gud_device *gdrm, struct drm_framebuffer
 
 	if (old_fb)
 		drm_framebuffer_put(old_fb);
+
+	return 0;
+}
+
+static void gud_fb_handle_damage(struct gud_device *gdrm, struct drm_framebuffer *fb,
+				 const struct iosys_map *map, struct drm_rect *damage)
+{
+	int ret;
+
+	if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
+		drm_rect_init(damage, 0, 0, fb->width, fb->height);
+
+	if (gud_async_flush) {
+		ret = gud_fb_queue_damage(gdrm, fb, map, damage);
+		if (ret != -ENOMEM)
+			return;
+	}
+
+	/* Imported buffers are assumed to be WriteCombined with uncached reads */
+	gud_flush_damage(gdrm, fb, map, !fb->obj[0]->import_attach, damage);
 }
 
 int gud_pipe_check(struct drm_simple_display_pipe *pipe,
@@ -544,6 +565,7 @@ void gud_pipe_update(struct drm_simple_display_pipe *pipe,
 	struct drm_device *drm = pipe->crtc.dev;
 	struct gud_device *gdrm = to_gud_device(drm);
 	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_crtc *crtc = &pipe->crtc;
 	struct drm_rect damage;
@@ -557,6 +579,8 @@ void gud_pipe_update(struct drm_simple_display_pipe *pipe,
 			gdrm->fb = NULL;
 		}
 		gud_clear_damage(gdrm);
+		vfree(gdrm->shadow_buf);
+		gdrm->shadow_buf = NULL;
 		mutex_unlock(&gdrm->damage_lock);
 	}
 
@@ -572,13 +596,8 @@ void gud_pipe_update(struct drm_simple_display_pipe *pipe,
 	if (crtc->state->active_changed)
 		gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
 
-	if (drm_atomic_helper_damage_merged(old_state, state, &damage)) {
-		if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
-			drm_rect_init(&damage, 0, 0, fb->width, fb->height);
-		gud_fb_queue_damage(gdrm, fb, &damage);
-		if (!gud_async_flush)
-			flush_work(&gdrm->work);
-	}
+	if (drm_atomic_helper_damage_merged(old_state, state, &damage))
+		gud_fb_handle_damage(gdrm, fb, &shadow_plane_state->data[0], &damage);
 
 	if (!crtc->state->enable)
 		gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);

-- 
b4 0.10.1

Re: [PATCH 1/6] drm/gem: shadow_fb_access: Prepare imported buffers for CPU access

[Javier Martinez Canillas]

Hello Noralf,

On 11/22/22 21:58, Noralf Trønnes via B4 Submission Endpoint wrote:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> Complete the shadow fb access functions by also preparing imported buffers
> for CPU access. Update the affected drivers that currently use
> drm_gem_fb_begin_cpu_access().
>
> Through this change the following SHMEM drivers will now also make sure
> their imported buffers are prepared for CPU access: cirrus, hyperv,
> mgag200, vkms
> 

[...]

> @@ -378,11 +379,20 @@ int drm_gem_begin_shadow_fb_access(struct drm_plane *plane, struct drm_plane_sta
>  {
>  	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>  	struct drm_framebuffer *fb = plane_state->fb;
> +	int ret;
>  
>  	if (!fb)
>  		return 0;
>  
> -	return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
> +	ret = drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret)
> +		drm_gem_fb_vunmap(fb, shadow_plane_state->map);
> +
> +	return ret;

Makes sense to me to have the CPU access prepare here too.

> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 53464afc2b9a..58a2f0113f24 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -544,7 +544,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>  	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
>  	struct iosys_map dst;
>  	unsigned int dst_pitch;
> -	int ret = 0;
>  	u8 *buf = NULL;
>  
>  	/* Align y to display page boundaries */
> @@ -556,21 +555,14 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> -	if (ret)
> -		goto out_free;
> -
>  	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);
> -

The only thing I'm not sure about is that drivers used to call the begin/end
CPU access only during the window while the BO data was accessed but now the
windows will be slightly bigger if that happens in .{begin,end}_fb_access.

If that's not an issue then, I agree with your patch since it simplifies the
logic in the drivers and gets rid of duplicated code.

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/6] drm/gud: Fix UBSAN warning

[Javier Martinez Canillas]

On 11/22/22 21:58, Noralf Trønnes via B4 Submission Endpoint wrote:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> UBSAN complains about invalid value for bool:
> 
> [  101.165172] [drm] Initialized gud 1.0.0 20200422 for 2-3.2:1.0 on minor 1
> [  101.213360] gud 2-3.2:1.0: [drm] fb1: guddrmfb frame buffer device
> [  101.213426] usbcore: registered new interface driver gud
> [  101.989431] ================================================================================
> [  101.989441] UBSAN: invalid-load in /home/pi/linux/include/linux/iosys-map.h:253:9
> [  101.989447] load of value 121 is not a valid value for type '_Bool'
> [  101.989451] CPU: 1 PID: 455 Comm: kworker/1:6 Not tainted 5.18.0-rc5-gud-5.18-rc5 #3
> [  101.989456] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018
> [  101.989459] Workqueue: events_long gud_flush_work [gud]
> [  101.989471] Call Trace:
> [  101.989474]  <TASK>
> [  101.989479]  dump_stack_lvl+0x49/0x5f
> [  101.989488]  dump_stack+0x10/0x12
> [  101.989493]  ubsan_epilogue+0x9/0x3b
> [  101.989498]  __ubsan_handle_load_invalid_value.cold+0x44/0x49
> [  101.989504]  dma_buf_vmap.cold+0x38/0x3d
> [  101.989511]  ? find_busiest_group+0x48/0x300
> [  101.989520]  drm_gem_shmem_vmap+0x76/0x1b0 [drm_shmem_helper]
> [  101.989528]  drm_gem_shmem_object_vmap+0x9/0xb [drm_shmem_helper]
> [  101.989535]  drm_gem_vmap+0x26/0x60 [drm]
> [  101.989594]  drm_gem_fb_vmap+0x47/0x150 [drm_kms_helper]
> [  101.989630]  gud_prep_flush+0xc1/0x710 [gud]
> [  101.989639]  ? _raw_spin_lock+0x17/0x40
> [  101.989648]  gud_flush_work+0x1e0/0x430 [gud]
> [  101.989653]  ? __switch_to+0x11d/0x470
> [  101.989664]  process_one_work+0x21f/0x3f0
> [  101.989673]  worker_thread+0x200/0x3e0
> [  101.989679]  ? rescuer_thread+0x390/0x390
> [  101.989684]  kthread+0xfd/0x130
> [  101.989690]  ? kthread_complete_and_exit+0x20/0x20
> [  101.989696]  ret_from_fork+0x22/0x30
> [  101.989706]  </TASK>
> [  101.989708] ================================================================================
> 
> The source of this warning is in iosys_map_clear() called from
> dma_buf_vmap(). It conditionally sets values based on map->is_iomem. The
> iosys_map variables are allocated uninitialized on the stack leading to
> ->is_iomem having all kinds of values and not only 0/1.
> 
> Fix this by zeroing the iosys_map variables.
> 
> Fixes: 40e1a70b4aed ("drm: Add GUD USB Display driver")
> Cc: <stable@vger.kernel.org> # v5.18+
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 3/6] drm/gud: Don't retry a failed framebuffer flush

[Javier Martinez Canillas]

On 11/22/22 21:58, Noralf Trønnes via B4 Submission Endpoint wrote:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> If a framebuffer flush fails the driver will do one retry by requeing the
> worker. Currently the worker is used even for synchronous flushing, but a
> later patch will inline it, so this needs to change. Thinking about how to
> solve this I came to the conclusion that this retry mechanism was a fix
> for a problem that was only in the mind of the developer (me) and not
> something that solved a real problem.
> 
> So let's remove this for now and revisit later should it become necessary.
> gud_add_damage() has now only one caller so it can be inlined.
>

Makes sense.
 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/gud/gud_pipe.c | 48 +++++++-----------------------------------
>  1 file changed, 8 insertions(+), 40 deletions(-)

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/6] drm/gem: shadow_fb_access: Prepare imported buffers for CPU access

[Thomas Zimmermann]

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

(cc: danvet)

Hi

Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> Complete the shadow fb access functions by also preparing imported buffers
> for CPU access. Update the affected drivers that currently use
> drm_gem_fb_begin_cpu_access().
> 
> Through this change the following SHMEM drivers will now also make sure
> their imported buffers are prepared for CPU access: cirrus, hyperv,
> mgag200, vkms

I had a similar patch recently, but Daniel shot it down. AFAIR 
begin_cpu_access *somehow* interferes with *something* and that can 
leads to *problems.*  Sorry that's the best I remember. Daniel should 
know. :D

Best regards
Thomas

> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/drm_gem_atomic_helper.c | 13 ++++++++++++-
>   drivers/gpu/drm/solomon/ssd130x.c       | 10 +---------
>   drivers/gpu/drm/tiny/gm12u320.c         | 10 +---------
>   drivers/gpu/drm/tiny/ofdrm.c            | 10 ++--------
>   drivers/gpu/drm/tiny/simpledrm.c        | 10 ++--------
>   drivers/gpu/drm/udl/udl_modeset.c       | 11 ++---------
>   6 files changed, 20 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
> index e42800718f51..0eef4bb30d25 100644
> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
> @@ -368,6 +368,7 @@ EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
>    * maps all buffer objects of the plane's framebuffer into kernel address
>    * space and stores them in struct &drm_shadow_plane_state.map. The first data
>    * bytes are available in struct &drm_shadow_plane_state.data.
> + * It also prepares imported buffers for CPU access.
>    *
>    * See drm_gem_end_shadow_fb_access() for cleanup.
>    *
> @@ -378,11 +379,20 @@ int drm_gem_begin_shadow_fb_access(struct drm_plane *plane, struct drm_plane_sta
>   {
>   	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>   	struct drm_framebuffer *fb = plane_state->fb;
> +	int ret;
>   
>   	if (!fb)
>   		return 0;
>   
> -	return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
> +	ret = drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret)
> +		drm_gem_fb_vunmap(fb, shadow_plane_state->map);
> +
> +	return ret;
>   }
>   EXPORT_SYMBOL(drm_gem_begin_shadow_fb_access);
>   
> @@ -404,6 +414,7 @@ void drm_gem_end_shadow_fb_access(struct drm_plane *plane, struct drm_plane_stat
>   	if (!fb)
>   		return;
>   
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>   	drm_gem_fb_vunmap(fb, shadow_plane_state->map);
>   }
>   EXPORT_SYMBOL(drm_gem_end_shadow_fb_access);
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 53464afc2b9a..58a2f0113f24 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -544,7 +544,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>   	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
>   	struct iosys_map dst;
>   	unsigned int dst_pitch;
> -	int ret = 0;
>   	u8 *buf = NULL;
>   
>   	/* Align y to display page boundaries */
> @@ -556,21 +555,14 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>   	if (!buf)
>   		return -ENOMEM;
>   
> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> -	if (ret)
> -		goto out_free;
> -
>   	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);
> -
>   	ssd130x_update_rect(ssd130x, buf, rect);
>   
> -out_free:
>   	kfree(buf);
>   
> -	return ret;
> +	return 0;
>   }
>   
>   static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index 130fd07a967d..59aad4b468cc 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c
> @@ -252,7 +252,7 @@ static void gm12u320_32bpp_to_24bpp_packed(u8 *dst, u8 *src, int len)
>   
>   static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
>   {
> -	int block, dst_offset, len, remain, ret, x1, x2, y1, y2;
> +	int block, dst_offset, len, remain, x1, x2, y1, y2;
>   	struct drm_framebuffer *fb;
>   	void *vaddr;
>   	u8 *src;
> @@ -269,12 +269,6 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
>   	y2 = gm12u320->fb_update.rect.y2;
>   	vaddr = gm12u320->fb_update.src_map.vaddr; /* TODO: Use mapping abstraction properly */
>   
> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> -	if (ret) {
> -		GM12U320_ERR("drm_gem_fb_begin_cpu_access err: %d\n", ret);
> -		goto put_fb;
> -	}
> -
>   	src = vaddr + y1 * fb->pitches[0] + x1 * 4;
>   
>   	x1 += (GM12U320_REAL_WIDTH - GM12U320_USER_WIDTH) / 2;
> @@ -309,8 +303,6 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
>   		src += fb->pitches[0];
>   	}
>   
> -	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> -put_fb:
>   	drm_framebuffer_put(fb);
>   	gm12u320->fb_update.fb = NULL;
>   unlock:
> diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
> index dc9e4d71b12a..ed3072563db9 100644
> --- a/drivers/gpu/drm/tiny/ofdrm.c
> +++ b/drivers/gpu/drm/tiny/ofdrm.c
> @@ -820,14 +820,10 @@ static void ofdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   	const struct drm_format_info *dst_format = odev->format;
>   	struct drm_atomic_helper_damage_iter iter;
>   	struct drm_rect damage;
> -	int ret, idx;
> -
> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> -	if (ret)
> -		return;
> +	int idx;
>   
>   	if (!drm_dev_enter(dev, &idx))
> -		goto out_drm_gem_fb_end_cpu_access;
> +		return;
>   
>   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>   	drm_atomic_for_each_plane_damage(&iter, &damage) {
> @@ -843,8 +839,6 @@ static void ofdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   	}
>   
>   	drm_dev_exit(idx);
> -out_drm_gem_fb_end_cpu_access:
> -	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>   }
>   
>   static void ofdrm_primary_plane_helper_atomic_disable(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 162eb44dcba8..1c0d9e277dc3 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -481,14 +481,10 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>   	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>   	struct drm_atomic_helper_damage_iter iter;
>   	struct drm_rect damage;
> -	int ret, idx;
> -
> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> -	if (ret)
> -		return;
> +	int idx;
>   
>   	if (!drm_dev_enter(dev, &idx))
> -		goto out_drm_gem_fb_end_cpu_access;
> +		return;
>   
>   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>   	drm_atomic_for_each_plane_damage(&iter, &damage) {
> @@ -504,8 +500,6 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>   	}
>   
>   	drm_dev_exit(idx);
> -out_drm_gem_fb_end_cpu_access:
> -	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>   }
>   
>   static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 4b79d44752c9..022b18aa3f48 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -271,17 +271,13 @@ static void udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>   	struct drm_atomic_helper_damage_iter iter;
>   	struct drm_rect damage;
> -	int ret, idx;
> +	int idx;
>   
>   	if (!fb)
>   		return; /* no framebuffer; plane is disabled */
>   
> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> -	if (ret)
> -		return;
> -
>   	if (!drm_dev_enter(dev, &idx))
> -		goto out_drm_gem_fb_end_cpu_access;
> +		return;
>   
>   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>   	drm_atomic_for_each_plane_damage(&iter, &damage) {
> @@ -289,9 +285,6 @@ static void udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
>   	}
>   
>   	drm_dev_exit(idx);
> -
> -out_drm_gem_fb_end_cpu_access:
> -	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>   }
>   
>   static const struct drm_plane_helper_funcs udl_primary_plane_helper_funcs = {
> 

-- 
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/6] drm/gud: Split up gud_flush_work()

[Javier Martinez Canillas]

On 11/22/22 21:58, Noralf Trønnes via B4 Submission Endpoint wrote:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> In preparation for inlining synchronous flushing split out the part of
> gud_flush_work() that can be shared by the sync and async code paths.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 5/6] drm/gud: Prepare buffer for CPU access in gud_flush_work()

[Javier Martinez Canillas]

On 11/22/22 21:58, Noralf Trønnes via B4 Submission Endpoint wrote:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> In preparation for moving to the shadow plane helper prepare the
> framebuffer for CPU access as early as possible.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 6/6] drm/gud: Use the shadow plane helper

[Javier Martinez Canillas]

On 11/22/22 21:58, Noralf Trønnes via B4 Submission Endpoint wrote:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> Use the shadow plane helper to take care of preparing the framebuffer for
> CPU access. The synchronous flushing is now done inline without the use of
> a worker. The async path now uses a shadow buffer to hold framebuffer
> changes and it doesn't read the framebuffer behind userspace's back
> anymore.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

Patch looks good to me. But it would be good if Thomas can double check.

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/6] drm/gud: Fix UBSAN warning

[Thomas Zimmermann]

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



Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> UBSAN complains about invalid value for bool:
> 
> [  101.165172] [drm] Initialized gud 1.0.0 20200422 for 2-3.2:1.0 on minor 1
> [  101.213360] gud 2-3.2:1.0: [drm] fb1: guddrmfb frame buffer device
> [  101.213426] usbcore: registered new interface driver gud
> [  101.989431] ================================================================================
> [  101.989441] UBSAN: invalid-load in /home/pi/linux/include/linux/iosys-map.h:253:9
> [  101.989447] load of value 121 is not a valid value for type '_Bool'
> [  101.989451] CPU: 1 PID: 455 Comm: kworker/1:6 Not tainted 5.18.0-rc5-gud-5.18-rc5 #3
> [  101.989456] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018
> [  101.989459] Workqueue: events_long gud_flush_work [gud]
> [  101.989471] Call Trace:
> [  101.989474]  <TASK>
> [  101.989479]  dump_stack_lvl+0x49/0x5f
> [  101.989488]  dump_stack+0x10/0x12
> [  101.989493]  ubsan_epilogue+0x9/0x3b
> [  101.989498]  __ubsan_handle_load_invalid_value.cold+0x44/0x49
> [  101.989504]  dma_buf_vmap.cold+0x38/0x3d
> [  101.989511]  ? find_busiest_group+0x48/0x300
> [  101.989520]  drm_gem_shmem_vmap+0x76/0x1b0 [drm_shmem_helper]
> [  101.989528]  drm_gem_shmem_object_vmap+0x9/0xb [drm_shmem_helper]
> [  101.989535]  drm_gem_vmap+0x26/0x60 [drm]
> [  101.989594]  drm_gem_fb_vmap+0x47/0x150 [drm_kms_helper]
> [  101.989630]  gud_prep_flush+0xc1/0x710 [gud]
> [  101.989639]  ? _raw_spin_lock+0x17/0x40
> [  101.989648]  gud_flush_work+0x1e0/0x430 [gud]
> [  101.989653]  ? __switch_to+0x11d/0x470
> [  101.989664]  process_one_work+0x21f/0x3f0
> [  101.989673]  worker_thread+0x200/0x3e0
> [  101.989679]  ? rescuer_thread+0x390/0x390
> [  101.989684]  kthread+0xfd/0x130
> [  101.989690]  ? kthread_complete_and_exit+0x20/0x20
> [  101.989696]  ret_from_fork+0x22/0x30
> [  101.989706]  </TASK>
> [  101.989708] ================================================================================
> 
> The source of this warning is in iosys_map_clear() called from
> dma_buf_vmap(). It conditionally sets values based on map->is_iomem. The
> iosys_map variables are allocated uninitialized on the stack leading to
> ->is_iomem having all kinds of values and not only 0/1.
> 
> Fix this by zeroing the iosys_map variables.
> 
> Fixes: 40e1a70b4aed ("drm: Add GUD USB Display driver")
> Cc: <stable@vger.kernel.org> # v5.18+
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/gud/gud_pipe.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index 7c6dc2bcd14a..61f4abaf1811 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -157,8 +157,8 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
>   {
>   	struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
>   	u8 compression = gdrm->compression;
> -	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> -	struct iosys_map map_data[DRM_FORMAT_MAX_PLANES];
> +	struct iosys_map map[DRM_FORMAT_MAX_PLANES] = { };
> +	struct iosys_map map_data[DRM_FORMAT_MAX_PLANES] = { };
>   	struct iosys_map dst;
>   	void *vaddr, *buf;
>   	size_t pitch, len;
> 

-- 
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 3/6] drm/gud: Don't retry a failed framebuffer flush

[Thomas Zimmermann]

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



Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> If a framebuffer flush fails the driver will do one retry by requeing the
> worker. Currently the worker is used even for synchronous flushing, but a
> later patch will inline it, so this needs to change. Thinking about how to
> solve this I came to the conclusion that this retry mechanism was a fix
> for a problem that was only in the mind of the developer (me) and not
> something that solved a real problem.
> 
> So let's remove this for now and revisit later should it become necessary.
> gud_add_damage() has now only one caller so it can be inlined.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/gud/gud_pipe.c | 48 +++++++-----------------------------------
>   1 file changed, 8 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index 61f4abaf1811..ff1358815af5 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -333,37 +333,6 @@ void gud_clear_damage(struct gud_device *gdrm)
>   	gdrm->damage.y2 = 0;
>   }
>   
> -static void gud_add_damage(struct gud_device *gdrm, struct drm_rect *damage)
> -{
> -	gdrm->damage.x1 = min(gdrm->damage.x1, damage->x1);
> -	gdrm->damage.y1 = min(gdrm->damage.y1, damage->y1);
> -	gdrm->damage.x2 = max(gdrm->damage.x2, damage->x2);
> -	gdrm->damage.y2 = max(gdrm->damage.y2, damage->y2);
> -}
> -
> -static void gud_retry_failed_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
> -				   struct drm_rect *damage)
> -{
> -	/*
> -	 * pipe_update waits for the worker when the display mode is going to change.
> -	 * This ensures that the width and height is still the same making it safe to
> -	 * add back the damage.
> -	 */
> -
> -	mutex_lock(&gdrm->damage_lock);
> -	if (!gdrm->fb) {
> -		drm_framebuffer_get(fb);
> -		gdrm->fb = fb;
> -	}
> -	gud_add_damage(gdrm, damage);
> -	mutex_unlock(&gdrm->damage_lock);
> -
> -	/* Retry only once to avoid a possible storm in case of continues errors. */
> -	if (!gdrm->prev_flush_failed)
> -		queue_work(system_long_wq, &gdrm->work);
> -	gdrm->prev_flush_failed = true;
> -}
> -
>   void gud_flush_work(struct work_struct *work)
>   {
>   	struct gud_device *gdrm = container_of(work, struct gud_device, work);
> @@ -407,14 +376,10 @@ void gud_flush_work(struct work_struct *work)
>   		ret = gud_flush_rect(gdrm, fb, format, &rect);
>   		if (ret) {
>   			if (ret != -ENODEV && ret != -ECONNRESET &&
> -			    ret != -ESHUTDOWN && ret != -EPROTO) {
> -				bool prev_flush_failed = gdrm->prev_flush_failed;
> -
> -				gud_retry_failed_flush(gdrm, fb, &damage);
> -				if (!prev_flush_failed)
> -					dev_err_ratelimited(fb->dev->dev,
> -							    "Failed to flush framebuffer: error=%d\n", ret);
> -			}
> +			    ret != -ESHUTDOWN && ret != -EPROTO)
> +				dev_err_ratelimited(fb->dev->dev,
> +						    "Failed to flush framebuffer: error=%d\n", ret);
> +			gdrm->prev_flush_failed = true;
>   			break;
>   		}
>   
> @@ -439,7 +404,10 @@ static void gud_fb_queue_damage(struct gud_device *gdrm, struct drm_framebuffer
>   		gdrm->fb = fb;
>   	}
>   
> -	gud_add_damage(gdrm, damage);
> +	gdrm->damage.x1 = min(gdrm->damage.x1, damage->x1);
> +	gdrm->damage.y1 = min(gdrm->damage.y1, damage->y1);
> +	gdrm->damage.x2 = max(gdrm->damage.x2, damage->x2);
> +	gdrm->damage.y2 = max(gdrm->damage.y2, damage->y2);
>   
>   	mutex_unlock(&gdrm->damage_lock);
>   
> 

-- 
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/6] drm/gud: Split up gud_flush_work()

[Thomas Zimmermann]

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



Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> In preparation for inlining synchronous flushing split out the part of
> gud_flush_work() that can be shared by the sync and async code paths.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/gud/gud_pipe.c | 72 +++++++++++++++++++++++-------------------
>   1 file changed, 39 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index ff1358815af5..d2af9947494f 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -333,15 +333,49 @@ void gud_clear_damage(struct gud_device *gdrm)
>   	gdrm->damage.y2 = 0;
>   }
>   
> +static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb,
> +			     struct drm_rect *damage)
> +{
> +	const struct drm_format_info *format;
> +	unsigned int i, lines;
> +	size_t pitch;
> +	int ret;
> +
> +	format = fb->format;
> +	if (format->format == DRM_FORMAT_XRGB8888 && gdrm->xrgb8888_emulation_format)
> +		format = gdrm->xrgb8888_emulation_format;
> +
> +	/* Split update if it's too big */
> +	pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(damage));
> +	lines = drm_rect_height(damage);
> +
> +	if (gdrm->bulk_len < lines * pitch)
> +		lines = gdrm->bulk_len / pitch;
> +
> +	for (i = 0; i < DIV_ROUND_UP(drm_rect_height(damage), lines); i++) {
> +		struct drm_rect rect = *damage;
> +
> +		rect.y1 += i * lines;
> +		rect.y2 = min_t(u32, rect.y1 + lines, damage->y2);
> +
> +		ret = gud_flush_rect(gdrm, fb, format, &rect);
> +		if (ret) {
> +			if (ret != -ENODEV && ret != -ECONNRESET &&
> +			    ret != -ESHUTDOWN && ret != -EPROTO)
> +				dev_err_ratelimited(fb->dev->dev,
> +						    "Failed to flush framebuffer: error=%d\n", ret);
> +			gdrm->prev_flush_failed = true;
> +			break;
> +		}
> +	}
> +}
> +
>   void gud_flush_work(struct work_struct *work)
>   {
>   	struct gud_device *gdrm = container_of(work, struct gud_device, work);
> -	const struct drm_format_info *format;
>   	struct drm_framebuffer *fb;
>   	struct drm_rect damage;
> -	unsigned int i, lines;
> -	int idx, ret = 0;
> -	size_t pitch;
> +	int idx;
>   
>   	if (!drm_dev_enter(&gdrm->drm, &idx))
>   		return;
> @@ -356,35 +390,7 @@ void gud_flush_work(struct work_struct *work)
>   	if (!fb)
>   		goto out;
>   
> -	format = fb->format;
> -	if (format->format == DRM_FORMAT_XRGB8888 && gdrm->xrgb8888_emulation_format)
> -		format = gdrm->xrgb8888_emulation_format;
> -
> -	/* Split update if it's too big */
> -	pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(&damage));
> -	lines = drm_rect_height(&damage);
> -
> -	if (gdrm->bulk_len < lines * pitch)
> -		lines = gdrm->bulk_len / pitch;
> -
> -	for (i = 0; i < DIV_ROUND_UP(drm_rect_height(&damage), lines); i++) {
> -		struct drm_rect rect = damage;
> -
> -		rect.y1 += i * lines;
> -		rect.y2 = min_t(u32, rect.y1 + lines, damage.y2);
> -
> -		ret = gud_flush_rect(gdrm, fb, format, &rect);
> -		if (ret) {
> -			if (ret != -ENODEV && ret != -ECONNRESET &&
> -			    ret != -ESHUTDOWN && ret != -EPROTO)
> -				dev_err_ratelimited(fb->dev->dev,
> -						    "Failed to flush framebuffer: error=%d\n", ret);
> -			gdrm->prev_flush_failed = true;
> -			break;
> -		}
> -
> -		gdrm->prev_flush_failed = false;
> -	}
> +	gud_flush_damage(gdrm, fb, &damage);
>   
>   	drm_framebuffer_put(fb);
>   out:
> 

-- 
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 5/6] drm/gud: Prepare buffer for CPU access in gud_flush_work()

[Thomas Zimmermann]

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

Hi

Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> In preparation for moving to the shadow plane helper prepare the
> framebuffer for CPU access as early as possible.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/gud/gud_pipe.c | 67 +++++++++++++++++++++---------------------
>   1 file changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index d2af9947494f..dfada6eedc58 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -15,6 +15,7 @@
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_gem.h>
> +#include <drm/drm_gem_atomic_helper.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_rect.h>
> @@ -152,32 +153,21 @@ static size_t gud_xrgb8888_to_color(u8 *dst, const struct drm_format_info *forma
>   }
>   
>   static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
> +			  const struct iosys_map *map, bool cached_reads,

Other functions call this 'src'. This would be the better name IMHO. 
Alternatively, 'data' would be preferable over 'map' because of the 
semantics of map vs map_data.


>   			  const struct drm_format_info *format, struct drm_rect *rect,
>   			  struct gud_set_buffer_req *req)
>   {
> -	struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
>   	u8 compression = gdrm->compression;
> -	struct iosys_map map[DRM_FORMAT_MAX_PLANES] = { };
> -	struct iosys_map map_data[DRM_FORMAT_MAX_PLANES] = { };
>   	struct iosys_map dst;
>   	void *vaddr, *buf;
>   	size_t pitch, len;
> -	int ret = 0;
>   
>   	pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(rect));
>   	len = pitch * drm_rect_height(rect);
>   	if (len > gdrm->bulk_len)
>   		return -E2BIG;
>   
> -	ret = drm_gem_fb_vmap(fb, map, map_data);
> -	if (ret)
> -		return ret;
> -
> -	vaddr = map_data[0].vaddr;
> -
> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);

As I mentioned in the reply to patch 1, begin_cpu_access has to remain 
in the driver. Moving it into the calling function is OK, though.

I know that it would be better to have a begin_cpu_access that cannot 
return an error. Daniel told me that we'd have to rework some other code 
to make this happen.

> -	if (ret)
> -		goto vunmap;
> +	vaddr = map[0].vaddr;
>   retry:
>   	if (compression)
>   		buf = gdrm->compress_buf;
> @@ -192,29 +182,27 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
>   	if (format != fb->format) {
>   		if (format->format == GUD_DRM_FORMAT_R1) {
>   			len = gud_xrgb8888_to_r124(buf, format, vaddr, fb, rect);
> -			if (!len) {
> -				ret = -ENOMEM;
> -				goto end_cpu_access;
> -			}
> +			if (!len)
> +				return -ENOMEM;
>   		} else if (format->format == DRM_FORMAT_R8) {
> -			drm_fb_xrgb8888_to_gray8(&dst, NULL, map_data, fb, rect);
> +			drm_fb_xrgb8888_to_gray8(&dst, NULL, map, fb, rect);
>   		} else if (format->format == DRM_FORMAT_RGB332) {
> -			drm_fb_xrgb8888_to_rgb332(&dst, NULL, map_data, fb, rect);
> +			drm_fb_xrgb8888_to_rgb332(&dst, NULL, map, fb, rect);
>   		} else if (format->format == DRM_FORMAT_RGB565) {
> -			drm_fb_xrgb8888_to_rgb565(&dst, NULL, map_data, fb, rect,
> +			drm_fb_xrgb8888_to_rgb565(&dst, NULL, map, fb, rect,
>   						  gud_is_big_endian());
>   		} else if (format->format == DRM_FORMAT_RGB888) {
> -			drm_fb_xrgb8888_to_rgb888(&dst, NULL, map_data, fb, rect);
> +			drm_fb_xrgb8888_to_rgb888(&dst, NULL, map, fb, rect);
>   		} else {
>   			len = gud_xrgb8888_to_color(buf, format, vaddr, fb, rect);
>   		}
>   	} else if (gud_is_big_endian() && format->cpp[0] > 1) {
> -		drm_fb_swab(&dst, NULL, map_data, fb, rect, !import_attach);
> -	} else if (compression && !import_attach && pitch == fb->pitches[0]) {
> +		drm_fb_swab(&dst, NULL, map, fb, rect, cached_reads);
> +	} else if (compression && cached_reads && pitch == fb->pitches[0]) {
>   		/* can compress directly from the framebuffer */
>   		buf = vaddr + rect->y1 * pitch;
>   	} else {
> -		drm_fb_memcpy(&dst, NULL, map_data, fb, rect);
> +		drm_fb_memcpy(&dst, NULL, map, fb, rect);
>   	}
>   
>   	memset(req, 0, sizeof(*req));
> @@ -237,12 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
>   		req->compressed_length = cpu_to_le32(complen);
>   	}
>   
> -end_cpu_access:
> -	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> -vunmap:
> -	drm_gem_fb_vunmap(fb, map);
> -
> -	return ret;
> +	return 0;
>   }
>   
>   struct gud_usb_bulk_context {
> @@ -285,6 +268,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
>   }
>   
>   static int gud_flush_rect(struct gud_device *gdrm, struct drm_framebuffer *fb,
> +			  const struct iosys_map *map, bool cached_reads,
>   			  const struct drm_format_info *format, struct drm_rect *rect)
>   {
>   	struct gud_set_buffer_req req;
> @@ -293,7 +277,7 @@ static int gud_flush_rect(struct gud_device *gdrm, struct drm_framebuffer *fb,
>   
>   	drm_dbg(&gdrm->drm, "Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
>   
> -	ret = gud_prep_flush(gdrm, fb, format, rect, &req);
> +	ret = gud_prep_flush(gdrm, fb, map, cached_reads, format, rect, &req);
>   	if (ret)
>   		return ret;
>   
> @@ -334,6 +318,7 @@ void gud_clear_damage(struct gud_device *gdrm)
>   }
>   
>   static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb,
> +			     const struct iosys_map *map, bool cached_reads,
>   			     struct drm_rect *damage)
>   {
>   	const struct drm_format_info *format;
> @@ -358,7 +343,7 @@ static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb
>   		rect.y1 += i * lines;
>   		rect.y2 = min_t(u32, rect.y1 + lines, damage->y2);
>   
> -		ret = gud_flush_rect(gdrm, fb, format, &rect);
> +		ret = gud_flush_rect(gdrm, fb, map, cached_reads, format, &rect);
>   		if (ret) {
>   			if (ret != -ENODEV && ret != -ECONNRESET &&
>   			    ret != -ESHUTDOWN && ret != -EPROTO)
> @@ -373,9 +358,10 @@ static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb
>   void gud_flush_work(struct work_struct *work)
>   {
>   	struct gud_device *gdrm = container_of(work, struct gud_device, work);
> +	struct iosys_map gem_map = { }, fb_map = { };
>   	struct drm_framebuffer *fb;
>   	struct drm_rect damage;
> -	int idx;
> +	int idx, ret;
>   
>   	if (!drm_dev_enter(&gdrm->drm, &idx))
>   		return;
> @@ -390,8 +376,21 @@ void gud_flush_work(struct work_struct *work)
>   	if (!fb)
>   		goto out;
>   
> -	gud_flush_damage(gdrm, fb, &damage);
> +	ret = drm_gem_fb_vmap(fb, &gem_map, &fb_map);
> +	if (ret)
> +		goto fb_put;
>   
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret)
> +		goto vunmap;
> +
> +	/* Imported buffers are assumed to be WriteCombined with uncached reads */
> +	gud_flush_damage(gdrm, fb, &fb_map, !fb->obj[0]->import_attach, &damage);
> +
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +vunmap:
> +	drm_gem_fb_vunmap(fb, &gem_map);
> +fb_put:
>   	drm_framebuffer_put(fb);
>   out:
>   	drm_dev_exit(idx);
> 

-- 
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 6/6] drm/gud: Use the shadow plane helper

[Thomas Zimmermann]

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

Hi

Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> Use the shadow plane helper to take care of preparing the framebuffer for
> CPU access. The synchronous flushing is now done inline without the use of
> a worker. The async path now uses a shadow buffer to hold framebuffer
> changes and it doesn't read the framebuffer behind userspace's back
> anymore.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/gud/gud_drv.c      |  1 +
>   drivers/gpu/drm/gud/gud_internal.h |  1 +
>   drivers/gpu/drm/gud/gud_pipe.c     | 69 ++++++++++++++++++++++++--------------
>   3 files changed, 46 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> index d57dab104358..5aac7cda0505 100644
> --- a/drivers/gpu/drm/gud/gud_drv.c
> +++ b/drivers/gpu/drm/gud/gud_drv.c
> @@ -365,6 +365,7 @@ static void gud_debugfs_init(struct drm_minor *minor)
>   static const struct drm_simple_display_pipe_funcs gud_pipe_funcs = {
>   	.check      = gud_pipe_check,
>   	.update	    = gud_pipe_update,
> +	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS
>   };
>   
>   static const struct drm_mode_config_funcs gud_mode_config_funcs = {
> diff --git a/drivers/gpu/drm/gud/gud_internal.h b/drivers/gpu/drm/gud/gud_internal.h
> index e351a1f1420d..0d148a6f27aa 100644
> --- a/drivers/gpu/drm/gud/gud_internal.h
> +++ b/drivers/gpu/drm/gud/gud_internal.h
> @@ -43,6 +43,7 @@ struct gud_device {
>   	struct drm_framebuffer *fb;
>   	struct drm_rect damage;
>   	bool prev_flush_failed;
> +	void *shadow_buf;
>   };
>   
>   static inline struct gud_device *to_gud_device(struct drm_device *drm)
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index dfada6eedc58..7686325f7ee7 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -358,10 +358,10 @@ static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb
>   void gud_flush_work(struct work_struct *work)
>   {
>   	struct gud_device *gdrm = container_of(work, struct gud_device, work);
> -	struct iosys_map gem_map = { }, fb_map = { };
> +	struct iosys_map shadow_map;
>   	struct drm_framebuffer *fb;
>   	struct drm_rect damage;
> -	int idx, ret;
> +	int idx;
>   
>   	if (!drm_dev_enter(&gdrm->drm, &idx))
>   		return;
> @@ -369,6 +369,7 @@ void gud_flush_work(struct work_struct *work)
>   	mutex_lock(&gdrm->damage_lock);
>   	fb = gdrm->fb;
>   	gdrm->fb = NULL;
> +	iosys_map_set_vaddr(&shadow_map, gdrm->shadow_buf);
>   	damage = gdrm->damage;
>   	gud_clear_damage(gdrm);
>   	mutex_unlock(&gdrm->damage_lock);
> @@ -376,33 +377,33 @@ void gud_flush_work(struct work_struct *work)
>   	if (!fb)
>   		goto out;
>   
> -	ret = drm_gem_fb_vmap(fb, &gem_map, &fb_map);
> -	if (ret)
> -		goto fb_put;
> +	gud_flush_damage(gdrm, fb, &shadow_map, true, &damage);
>   
> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> -	if (ret)
> -		goto vunmap;
> -
> -	/* Imported buffers are assumed to be WriteCombined with uncached reads */
> -	gud_flush_damage(gdrm, fb, &fb_map, !fb->obj[0]->import_attach, &damage);
> -
> -	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> -vunmap:
> -	drm_gem_fb_vunmap(fb, &gem_map);
> -fb_put:
>   	drm_framebuffer_put(fb);
>   out:
>   	drm_dev_exit(idx);
>   }
>   
> -static void gud_fb_queue_damage(struct gud_device *gdrm, struct drm_framebuffer *fb,
> -				struct drm_rect *damage)
> +static int gud_fb_queue_damage(struct gud_device *gdrm, struct drm_framebuffer *fb,
> +			       const struct iosys_map *map, struct drm_rect *damage)
>   {
>   	struct drm_framebuffer *old_fb = NULL;
> +	struct iosys_map shadow_map;
>   
>   	mutex_lock(&gdrm->damage_lock);
>   
> +	if (!gdrm->shadow_buf) {
> +		gdrm->shadow_buf = vzalloc(fb->pitches[0] * fb->height);
> +		if (!gdrm->shadow_buf) {
> +			mutex_unlock(&gdrm->damage_lock);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	iosys_map_set_vaddr(&shadow_map, gdrm->shadow_buf);
> +	iosys_map_incr(&shadow_map, drm_fb_clip_offset(fb->pitches[0], fb->format, damage));
> +	drm_fb_memcpy(&shadow_map, fb->pitches, map, fb, damage);
> +

It's all good (sans the begin_cpu_access issue) in terms of code, but 
the memcpy here can have quite an impact on performace.

So why not just nuke the async code entirely? It's a userspace problem 
and we don't have many(/any?) other drivers with such a workaround. Udl, 
our other usb-based driver, works well enough without.

Best regards
Thomas

>   	if (fb != gdrm->fb) {
>   		old_fb = gdrm->fb;
>   		drm_framebuffer_get(fb);
> @@ -420,6 +421,26 @@ static void gud_fb_queue_damage(struct gud_device *gdrm, struct drm_framebuffer
>   
>   	if (old_fb)
>   		drm_framebuffer_put(old_fb);
> +
> +	return 0;
> +}
> +
> +static void gud_fb_handle_damage(struct gud_device *gdrm, struct drm_framebuffer *fb,
> +				 const struct iosys_map *map, struct drm_rect *damage)
> +{
> +	int ret;
> +
> +	if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
> +		drm_rect_init(damage, 0, 0, fb->width, fb->height);
> +
> +	if (gud_async_flush) {
> +		ret = gud_fb_queue_damage(gdrm, fb, map, damage);
> +		if (ret != -ENOMEM)
> +			return;
> +	}
> +
> +	/* Imported buffers are assumed to be WriteCombined with uncached reads */
> +	gud_flush_damage(gdrm, fb, map, !fb->obj[0]->import_attach, damage);
>   }
>   
>   int gud_pipe_check(struct drm_simple_display_pipe *pipe,
> @@ -544,6 +565,7 @@ void gud_pipe_update(struct drm_simple_display_pipe *pipe,
>   	struct drm_device *drm = pipe->crtc.dev;
>   	struct gud_device *gdrm = to_gud_device(drm);
>   	struct drm_plane_state *state = pipe->plane.state;
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>   	struct drm_framebuffer *fb = state->fb;
>   	struct drm_crtc *crtc = &pipe->crtc;
>   	struct drm_rect damage;
> @@ -557,6 +579,8 @@ void gud_pipe_update(struct drm_simple_display_pipe *pipe,
>   			gdrm->fb = NULL;
>   		}
>   		gud_clear_damage(gdrm);
> +		vfree(gdrm->shadow_buf);
> +		gdrm->shadow_buf = NULL;
>   		mutex_unlock(&gdrm->damage_lock);
>   	}
>   
> @@ -572,13 +596,8 @@ void gud_pipe_update(struct drm_simple_display_pipe *pipe,
>   	if (crtc->state->active_changed)
>   		gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
>   
> -	if (drm_atomic_helper_damage_merged(old_state, state, &damage)) {
> -		if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
> -			drm_rect_init(&damage, 0, 0, fb->width, fb->height);
> -		gud_fb_queue_damage(gdrm, fb, &damage);
> -		if (!gud_async_flush)
> -			flush_work(&gdrm->work);
> -	}
> +	if (drm_atomic_helper_damage_merged(old_state, state, &damage))
> +		gud_fb_handle_damage(gdrm, fb, &shadow_plane_state->data[0], &damage);
>   
>   	if (!crtc->state->enable)
>   		gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
> 

-- 
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 1/6] drm/gem: shadow_fb_access: Prepare imported buffers for CPU access

[Noralf Trønnes]

Den 23.11.2022 09.39, skrev Thomas Zimmermann:
> (cc: danvet)
> 
> Hi
> 
> Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
>> From: Noralf Trønnes <noralf@tronnes.org>
>>
>> Complete the shadow fb access functions by also preparing imported
>> buffers
>> for CPU access. Update the affected drivers that currently use
>> drm_gem_fb_begin_cpu_access().
>>
>> Through this change the following SHMEM drivers will now also make sure
>> their imported buffers are prepared for CPU access: cirrus, hyperv,
>> mgag200, vkms
> 
> I had a similar patch recently, but Daniel shot it down. AFAIR
> begin_cpu_access *somehow* interferes with *something* and that can
> leads to *problems.*  Sorry that's the best I remember. Daniel should
> know. :D
> 

I loved this explanation, it gave me a good laugh :)

Yeah, I wondered why you hadn't done it, but assumed you just hadn't
gotten atround to it yet and if there were other reasons I would be told :)

Noralf.

> Best regards
> Thomas
> 
>>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/drm_gem_atomic_helper.c | 13 ++++++++++++-
>>   drivers/gpu/drm/solomon/ssd130x.c       | 10 +---------
>>   drivers/gpu/drm/tiny/gm12u320.c         | 10 +---------
>>   drivers/gpu/drm/tiny/ofdrm.c            | 10 ++--------
>>   drivers/gpu/drm/tiny/simpledrm.c        | 10 ++--------
>>   drivers/gpu/drm/udl/udl_modeset.c       | 11 ++---------
>>   6 files changed, 20 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c
>> b/drivers/gpu/drm/drm_gem_atomic_helper.c
>> index e42800718f51..0eef4bb30d25 100644
>> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
>> @@ -368,6 +368,7 @@ EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
>>    * maps all buffer objects of the plane's framebuffer into kernel
>> address
>>    * space and stores them in struct &drm_shadow_plane_state.map. The
>> first data
>>    * bytes are available in struct &drm_shadow_plane_state.data.
>> + * It also prepares imported buffers for CPU access.
>>    *
>>    * See drm_gem_end_shadow_fb_access() for cleanup.
>>    *
>> @@ -378,11 +379,20 @@ int drm_gem_begin_shadow_fb_access(struct
>> drm_plane *plane, struct drm_plane_sta
>>   {
>>       struct drm_shadow_plane_state *shadow_plane_state =
>> to_drm_shadow_plane_state(plane_state);
>>       struct drm_framebuffer *fb = plane_state->fb;
>> +    int ret;
>>         if (!fb)
>>           return 0;
>>   -    return drm_gem_fb_vmap(fb, shadow_plane_state->map,
>> shadow_plane_state->data);
>> +    ret = drm_gem_fb_vmap(fb, shadow_plane_state->map,
>> shadow_plane_state->data);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> +    if (ret)
>> +        drm_gem_fb_vunmap(fb, shadow_plane_state->map);
>> +
>> +    return ret;
>>   }
>>   EXPORT_SYMBOL(drm_gem_begin_shadow_fb_access);
>>   @@ -404,6 +414,7 @@ void drm_gem_end_shadow_fb_access(struct
>> drm_plane *plane, struct drm_plane_stat
>>       if (!fb)
>>           return;
>>   +    drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>       drm_gem_fb_vunmap(fb, shadow_plane_state->map);
>>   }
>>   EXPORT_SYMBOL(drm_gem_end_shadow_fb_access);
>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c
>> b/drivers/gpu/drm/solomon/ssd130x.c
>> index 53464afc2b9a..58a2f0113f24 100644
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -544,7 +544,6 @@ static int ssd130x_fb_blit_rect(struct
>> drm_framebuffer *fb, const struct iosys_m
>>       struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
>>       struct iosys_map dst;
>>       unsigned int dst_pitch;
>> -    int ret = 0;
>>       u8 *buf = NULL;
>>         /* Align y to display page boundaries */
>> @@ -556,21 +555,14 @@ static int ssd130x_fb_blit_rect(struct
>> drm_framebuffer *fb, const struct iosys_m
>>       if (!buf)
>>           return -ENOMEM;
>>   -    ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> -    if (ret)
>> -        goto out_free;
>> -
>>       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);
>> -
>>       ssd130x_update_rect(ssd130x, buf, rect);
>>   -out_free:
>>       kfree(buf);
>>   -    return ret;
>> +    return 0;
>>   }
>>     static void ssd130x_primary_plane_helper_atomic_update(struct
>> drm_plane *plane,
>> diff --git a/drivers/gpu/drm/tiny/gm12u320.c
>> b/drivers/gpu/drm/tiny/gm12u320.c
>> index 130fd07a967d..59aad4b468cc 100644
>> --- a/drivers/gpu/drm/tiny/gm12u320.c
>> +++ b/drivers/gpu/drm/tiny/gm12u320.c
>> @@ -252,7 +252,7 @@ static void gm12u320_32bpp_to_24bpp_packed(u8
>> *dst, u8 *src, int len)
>>     static void gm12u320_copy_fb_to_blocks(struct gm12u320_device
>> *gm12u320)
>>   {
>> -    int block, dst_offset, len, remain, ret, x1, x2, y1, y2;
>> +    int block, dst_offset, len, remain, x1, x2, y1, y2;
>>       struct drm_framebuffer *fb;
>>       void *vaddr;
>>       u8 *src;
>> @@ -269,12 +269,6 @@ static void gm12u320_copy_fb_to_blocks(struct
>> gm12u320_device *gm12u320)
>>       y2 = gm12u320->fb_update.rect.y2;
>>       vaddr = gm12u320->fb_update.src_map.vaddr; /* TODO: Use mapping
>> abstraction properly */
>>   -    ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> -    if (ret) {
>> -        GM12U320_ERR("drm_gem_fb_begin_cpu_access err: %d\n", ret);
>> -        goto put_fb;
>> -    }
>> -
>>       src = vaddr + y1 * fb->pitches[0] + x1 * 4;
>>         x1 += (GM12U320_REAL_WIDTH - GM12U320_USER_WIDTH) / 2;
>> @@ -309,8 +303,6 @@ static void gm12u320_copy_fb_to_blocks(struct
>> gm12u320_device *gm12u320)
>>           src += fb->pitches[0];
>>       }
>>   -    drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> -put_fb:
>>       drm_framebuffer_put(fb);
>>       gm12u320->fb_update.fb = NULL;
>>   unlock:
>> diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
>> index dc9e4d71b12a..ed3072563db9 100644
>> --- a/drivers/gpu/drm/tiny/ofdrm.c
>> +++ b/drivers/gpu/drm/tiny/ofdrm.c
>> @@ -820,14 +820,10 @@ static void
>> ofdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>       const struct drm_format_info *dst_format = odev->format;
>>       struct drm_atomic_helper_damage_iter iter;
>>       struct drm_rect damage;
>> -    int ret, idx;
>> -
>> -    ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> -    if (ret)
>> -        return;
>> +    int idx;
>>         if (!drm_dev_enter(dev, &idx))
>> -        goto out_drm_gem_fb_end_cpu_access;
>> +        return;
>>         drm_atomic_helper_damage_iter_init(&iter, old_plane_state,
>> plane_state);
>>       drm_atomic_for_each_plane_damage(&iter, &damage) {
>> @@ -843,8 +839,6 @@ static void
>> ofdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>       }
>>         drm_dev_exit(idx);
>> -out_drm_gem_fb_end_cpu_access:
>> -    drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>   }
>>     static void ofdrm_primary_plane_helper_atomic_disable(struct
>> drm_plane *plane,
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
>> b/drivers/gpu/drm/tiny/simpledrm.c
>> index 162eb44dcba8..1c0d9e277dc3 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -481,14 +481,10 @@ static void
>> simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>>       struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>>       struct drm_atomic_helper_damage_iter iter;
>>       struct drm_rect damage;
>> -    int ret, idx;
>> -
>> -    ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> -    if (ret)
>> -        return;
>> +    int idx;
>>         if (!drm_dev_enter(dev, &idx))
>> -        goto out_drm_gem_fb_end_cpu_access;
>> +        return;
>>         drm_atomic_helper_damage_iter_init(&iter, old_plane_state,
>> plane_state);
>>       drm_atomic_for_each_plane_damage(&iter, &damage) {
>> @@ -504,8 +500,6 @@ static void
>> simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>>       }
>>         drm_dev_exit(idx);
>> -out_drm_gem_fb_end_cpu_access:
>> -    drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>   }
>>     static void simpledrm_primary_plane_helper_atomic_disable(struct
>> drm_plane *plane,
>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c
>> b/drivers/gpu/drm/udl/udl_modeset.c
>> index 4b79d44752c9..022b18aa3f48 100644
>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>> @@ -271,17 +271,13 @@ static void
>> udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>       struct drm_plane_state *old_plane_state =
>> drm_atomic_get_old_plane_state(state, plane);
>>       struct drm_atomic_helper_damage_iter iter;
>>       struct drm_rect damage;
>> -    int ret, idx;
>> +    int idx;
>>         if (!fb)
>>           return; /* no framebuffer; plane is disabled */
>>   -    ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> -    if (ret)
>> -        return;
>> -
>>       if (!drm_dev_enter(dev, &idx))
>> -        goto out_drm_gem_fb_end_cpu_access;
>> +        return;
>>         drm_atomic_helper_damage_iter_init(&iter, old_plane_state,
>> plane_state);
>>       drm_atomic_for_each_plane_damage(&iter, &damage) {
>> @@ -289,9 +285,6 @@ static void
>> udl_primary_plane_helper_atomic_update(struct drm_plane *plane,
>>       }
>>         drm_dev_exit(idx);
>> -
>> -out_drm_gem_fb_end_cpu_access:
>> -    drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>   }
>>     static const struct drm_plane_helper_funcs
>> udl_primary_plane_helper_funcs = {
>>
> 

Re: [PATCH 6/6] drm/gud: Use the shadow plane helper

[Noralf Trønnes]

Den 23.11.2022 11.38, skrev Thomas Zimmermann:
> Hi
> 
> Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
>> From: Noralf Trønnes <noralf@tronnes.org>
>>
>> Use the shadow plane helper to take care of preparing the framebuffer for
>> CPU access. The synchronous flushing is now done inline without the
>> use of
>> a worker. The async path now uses a shadow buffer to hold framebuffer
>> changes and it doesn't read the framebuffer behind userspace's back
>> anymore.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/gud/gud_drv.c      |  1 +
>>   drivers/gpu/drm/gud/gud_internal.h |  1 +
>>   drivers/gpu/drm/gud/gud_pipe.c     | 69
>> ++++++++++++++++++++++++--------------
>>   3 files changed, 46 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/gud/gud_drv.c
>> b/drivers/gpu/drm/gud/gud_drv.c
>> index d57dab104358..5aac7cda0505 100644
>> --- a/drivers/gpu/drm/gud/gud_drv.c
>> +++ b/drivers/gpu/drm/gud/gud_drv.c
>> @@ -365,6 +365,7 @@ static void gud_debugfs_init(struct drm_minor *minor)
>>   static const struct drm_simple_display_pipe_funcs gud_pipe_funcs = {
>>       .check      = gud_pipe_check,
>>       .update        = gud_pipe_update,
>> +    DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS
>>   };
>>     static const struct drm_mode_config_funcs gud_mode_config_funcs = {
>> diff --git a/drivers/gpu/drm/gud/gud_internal.h
>> b/drivers/gpu/drm/gud/gud_internal.h
>> index e351a1f1420d..0d148a6f27aa 100644
>> --- a/drivers/gpu/drm/gud/gud_internal.h
>> +++ b/drivers/gpu/drm/gud/gud_internal.h
>> @@ -43,6 +43,7 @@ struct gud_device {
>>       struct drm_framebuffer *fb;
>>       struct drm_rect damage;
>>       bool prev_flush_failed;
>> +    void *shadow_buf;
>>   };
>>     static inline struct gud_device *to_gud_device(struct drm_device
>> *drm)
>> diff --git a/drivers/gpu/drm/gud/gud_pipe.c
>> b/drivers/gpu/drm/gud/gud_pipe.c
>> index dfada6eedc58..7686325f7ee7 100644
>> --- a/drivers/gpu/drm/gud/gud_pipe.c
>> +++ b/drivers/gpu/drm/gud/gud_pipe.c
>> @@ -358,10 +358,10 @@ static void gud_flush_damage(struct gud_device
>> *gdrm, struct drm_framebuffer *fb
>>   void gud_flush_work(struct work_struct *work)
>>   {
>>       struct gud_device *gdrm = container_of(work, struct gud_device,
>> work);
>> -    struct iosys_map gem_map = { }, fb_map = { };
>> +    struct iosys_map shadow_map;
>>       struct drm_framebuffer *fb;
>>       struct drm_rect damage;
>> -    int idx, ret;
>> +    int idx;
>>         if (!drm_dev_enter(&gdrm->drm, &idx))
>>           return;
>> @@ -369,6 +369,7 @@ void gud_flush_work(struct work_struct *work)
>>       mutex_lock(&gdrm->damage_lock);
>>       fb = gdrm->fb;
>>       gdrm->fb = NULL;
>> +    iosys_map_set_vaddr(&shadow_map, gdrm->shadow_buf);
>>       damage = gdrm->damage;
>>       gud_clear_damage(gdrm);
>>       mutex_unlock(&gdrm->damage_lock);
>> @@ -376,33 +377,33 @@ void gud_flush_work(struct work_struct *work)
>>       if (!fb)
>>           goto out;
>>   -    ret = drm_gem_fb_vmap(fb, &gem_map, &fb_map);
>> -    if (ret)
>> -        goto fb_put;
>> +    gud_flush_damage(gdrm, fb, &shadow_map, true, &damage);
>>   -    ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> -    if (ret)
>> -        goto vunmap;
>> -
>> -    /* Imported buffers are assumed to be WriteCombined with uncached
>> reads */
>> -    gud_flush_damage(gdrm, fb, &fb_map, !fb->obj[0]->import_attach,
>> &damage);
>> -
>> -    drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> -vunmap:
>> -    drm_gem_fb_vunmap(fb, &gem_map);
>> -fb_put:
>>       drm_framebuffer_put(fb);
>>   out:
>>       drm_dev_exit(idx);
>>   }
>>   -static void gud_fb_queue_damage(struct gud_device *gdrm, struct
>> drm_framebuffer *fb,
>> -                struct drm_rect *damage)
>> +static int gud_fb_queue_damage(struct gud_device *gdrm, struct
>> drm_framebuffer *fb,
>> +                   const struct iosys_map *map, struct drm_rect *damage)
>>   {
>>       struct drm_framebuffer *old_fb = NULL;
>> +    struct iosys_map shadow_map;
>>         mutex_lock(&gdrm->damage_lock);
>>   +    if (!gdrm->shadow_buf) {
>> +        gdrm->shadow_buf = vzalloc(fb->pitches[0] * fb->height);
>> +        if (!gdrm->shadow_buf) {
>> +            mutex_unlock(&gdrm->damage_lock);
>> +            return -ENOMEM;
>> +        }
>> +    }
>> +
>> +    iosys_map_set_vaddr(&shadow_map, gdrm->shadow_buf);
>> +    iosys_map_incr(&shadow_map, drm_fb_clip_offset(fb->pitches[0],
>> fb->format, damage));
>> +    drm_fb_memcpy(&shadow_map, fb->pitches, map, fb, damage);
>> +
> 
> It's all good (sans the begin_cpu_access issue) in terms of code, but
> the memcpy here can have quite an impact on performace.
> 

Yes if the gud display is the only one in the rendering loop it's not
good. This is on an old HP laptop running the async path (RGB565 over USB):

$ modetest -M gud -s 36:1920x1080 -v
setting mode 1920x1080-60.00Hz on connectors 36, crtc 33
freq: 414.56Hz
freq: 439.71Hz

This is the normal path:

$ modetest -M gud -s 36:1920x1080 -v
setting mode 1920x1080-60.00Hz on connectors 36, crtc 33
freq: 18.11Hz
freq: 17.98Hz

> So why not just nuke the async code entirely? It's a userspace problem
> and we don't have many(/any?) other drivers with such a workaround. Udl,
> our other usb-based driver, works well enough without.
> 

I want to do it gradually so I did consider switching the default to
synchronous flushing in this patchset. I think maybe I should just do
that since you say it "works well enough".

I took the async idea from tiny/gm12u320 and Hans explained the need for
the async worker. He's well versed with userspace so I did the same. But
that is 3 years ago and things change. My long term plan is to drop the
async path, but I want to keep it until I know there are no problems.

I've seen some reports about the udl driver having horrible performance,
but I haven't seen any explanations as to why. It could be because GNOME
until recently[1] didn't provide damage info to the kernel.

The GNOME rendering loop issue was solved in 3.38[2]

Noralf.

[1] https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1879
[2]
https://blogs.gnome.org/shell-dev/2020/07/02/splitting-up-the-frame-clock/

> Best regards
> Thomas
> 
>>       if (fb != gdrm->fb) {
>>           old_fb = gdrm->fb;
>>           drm_framebuffer_get(fb);
>> @@ -420,6 +421,26 @@ static void gud_fb_queue_damage(struct gud_device
>> *gdrm, struct drm_framebuffer
>>         if (old_fb)
>>           drm_framebuffer_put(old_fb);
>> +
>> +    return 0;
>> +}
>> +
>> +static void gud_fb_handle_damage(struct gud_device *gdrm, struct
>> drm_framebuffer *fb,
>> +                 const struct iosys_map *map, struct drm_rect *damage)
>> +{
>> +    int ret;
>> +
>> +    if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
>> +        drm_rect_init(damage, 0, 0, fb->width, fb->height);
>> +
>> +    if (gud_async_flush) {
>> +        ret = gud_fb_queue_damage(gdrm, fb, map, damage);
>> +        if (ret != -ENOMEM)
>> +            return;
>> +    }
>> +
>> +    /* Imported buffers are assumed to be WriteCombined with uncached
>> reads */
>> +    gud_flush_damage(gdrm, fb, map, !fb->obj[0]->import_attach, damage);
>>   }
>>     int gud_pipe_check(struct drm_simple_display_pipe *pipe,
>> @@ -544,6 +565,7 @@ void gud_pipe_update(struct
>> drm_simple_display_pipe *pipe,
>>       struct drm_device *drm = pipe->crtc.dev;
>>       struct gud_device *gdrm = to_gud_device(drm);
>>       struct drm_plane_state *state = pipe->plane.state;
>> +    struct drm_shadow_plane_state *shadow_plane_state =
>> to_drm_shadow_plane_state(state);
>>       struct drm_framebuffer *fb = state->fb;
>>       struct drm_crtc *crtc = &pipe->crtc;
>>       struct drm_rect damage;
>> @@ -557,6 +579,8 @@ void gud_pipe_update(struct
>> drm_simple_display_pipe *pipe,
>>               gdrm->fb = NULL;
>>           }
>>           gud_clear_damage(gdrm);
>> +        vfree(gdrm->shadow_buf);
>> +        gdrm->shadow_buf = NULL;
>>           mutex_unlock(&gdrm->damage_lock);
>>       }
>>   @@ -572,13 +596,8 @@ void gud_pipe_update(struct
>> drm_simple_display_pipe *pipe,
>>       if (crtc->state->active_changed)
>>           gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE,
>> crtc->state->active);
>>   -    if (drm_atomic_helper_damage_merged(old_state, state, &damage)) {
>> -        if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
>> -            drm_rect_init(&damage, 0, 0, fb->width, fb->height);
>> -        gud_fb_queue_damage(gdrm, fb, &damage);
>> -        if (!gud_async_flush)
>> -            flush_work(&gdrm->work);
>> -    }
>> +    if (drm_atomic_helper_damage_merged(old_state, state, &damage))
>> +        gud_fb_handle_damage(gdrm, fb, &shadow_plane_state->data[0],
>> &damage);
>>         if (!crtc->state->enable)
>>           gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
>>
> 

Re: [PATCH 1/6] drm/gem: shadow_fb_access: Prepare imported buffers for CPU access

[Noralf Trønnes]

Den 23.11.2022 09.22, skrev Javier Martinez Canillas:
> Hello Noralf,
> 
> On 11/22/22 21:58, Noralf Trønnes via B4 Submission Endpoint wrote:
>> From: Noralf Trønnes <noralf@tronnes.org>
>>
>> Complete the shadow fb access functions by also preparing imported buffers
>> for CPU access. Update the affected drivers that currently use
>> drm_gem_fb_begin_cpu_access().
>>
>> Through this change the following SHMEM drivers will now also make sure
>> their imported buffers are prepared for CPU access: cirrus, hyperv,
>> mgag200, vkms
>>
> 
> [...]
> 
>> @@ -378,11 +379,20 @@ int drm_gem_begin_shadow_fb_access(struct drm_plane *plane, struct drm_plane_sta
>>  {
>>  	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>>  	struct drm_framebuffer *fb = plane_state->fb;
>> +	int ret;
>>  
>>  	if (!fb)
>>  		return 0;
>>  
>> -	return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
>> +	ret = drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> +	if (ret)
>> +		drm_gem_fb_vunmap(fb, shadow_plane_state->map);
>> +
>> +	return ret;
> 
> Makes sense to me to have the CPU access prepare here too.
> 
>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
>> index 53464afc2b9a..58a2f0113f24 100644
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -544,7 +544,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>>  	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
>>  	struct iosys_map dst;
>>  	unsigned int dst_pitch;
>> -	int ret = 0;
>>  	u8 *buf = NULL;
>>  
>>  	/* Align y to display page boundaries */
>> @@ -556,21 +555,14 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>>  	if (!buf)
>>  		return -ENOMEM;
>>  
>> -	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> -	if (ret)
>> -		goto out_free;
>> -
>>  	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);
>> -
> 
> The only thing I'm not sure about is that drivers used to call the begin/end
> CPU access only during the window while the BO data was accessed but now the
> windows will be slightly bigger if that happens in .{begin,end}_fb_access.
> 

I didn't think that could be an issue since userspace isn't touching the
buffer while the commit is ongoing anyways, but it's a complicated
machinery. We'll see what Daniel has to say.

Noralf.

> If that's not an issue then, I agree with your patch since it simplifies the
> logic in the drivers and gets rid of duplicated code.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

Re: [PATCH 6/6] drm/gud: Use the shadow plane helper

[Thomas Zimmermann]

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

Hi

Am 23.11.22 um 13:37 schrieb Noralf Trønnes:
> 
> 
> Den 23.11.2022 11.38, skrev Thomas Zimmermann:
>> Hi
>>
>> Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
>>> From: Noralf Trønnes <noralf@tronnes.org>
>>>
>>> Use the shadow plane helper to take care of preparing the framebuffer for
>>> CPU access. The synchronous flushing is now done inline without the
>>> use of
>>> a worker. The async path now uses a shadow buffer to hold framebuffer
>>> changes and it doesn't read the framebuffer behind userspace's back
>>> anymore.
>>>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>>>    drivers/gpu/drm/gud/gud_drv.c      |  1 +
>>>    drivers/gpu/drm/gud/gud_internal.h |  1 +
>>>    drivers/gpu/drm/gud/gud_pipe.c     | 69
>>> ++++++++++++++++++++++++--------------
>>>    3 files changed, 46 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/gud/gud_drv.c
>>> b/drivers/gpu/drm/gud/gud_drv.c
>>> index d57dab104358..5aac7cda0505 100644
>>> --- a/drivers/gpu/drm/gud/gud_drv.c
>>> +++ b/drivers/gpu/drm/gud/gud_drv.c
>>> @@ -365,6 +365,7 @@ static void gud_debugfs_init(struct drm_minor *minor)
>>>    static const struct drm_simple_display_pipe_funcs gud_pipe_funcs = {
>>>        .check      = gud_pipe_check,
>>>        .update        = gud_pipe_update,
>>> +    DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS
>>>    };
>>>      static const struct drm_mode_config_funcs gud_mode_config_funcs = {
>>> diff --git a/drivers/gpu/drm/gud/gud_internal.h
>>> b/drivers/gpu/drm/gud/gud_internal.h
>>> index e351a1f1420d..0d148a6f27aa 100644
>>> --- a/drivers/gpu/drm/gud/gud_internal.h
>>> +++ b/drivers/gpu/drm/gud/gud_internal.h
>>> @@ -43,6 +43,7 @@ struct gud_device {
>>>        struct drm_framebuffer *fb;
>>>        struct drm_rect damage;
>>>        bool prev_flush_failed;
>>> +    void *shadow_buf;
>>>    };
>>>      static inline struct gud_device *to_gud_device(struct drm_device
>>> *drm)
>>> diff --git a/drivers/gpu/drm/gud/gud_pipe.c
>>> b/drivers/gpu/drm/gud/gud_pipe.c
>>> index dfada6eedc58..7686325f7ee7 100644
>>> --- a/drivers/gpu/drm/gud/gud_pipe.c
>>> +++ b/drivers/gpu/drm/gud/gud_pipe.c
>>> @@ -358,10 +358,10 @@ static void gud_flush_damage(struct gud_device
>>> *gdrm, struct drm_framebuffer *fb
>>>    void gud_flush_work(struct work_struct *work)
>>>    {
>>>        struct gud_device *gdrm = container_of(work, struct gud_device,
>>> work);
>>> -    struct iosys_map gem_map = { }, fb_map = { };
>>> +    struct iosys_map shadow_map;
>>>        struct drm_framebuffer *fb;
>>>        struct drm_rect damage;
>>> -    int idx, ret;
>>> +    int idx;
>>>          if (!drm_dev_enter(&gdrm->drm, &idx))
>>>            return;
>>> @@ -369,6 +369,7 @@ void gud_flush_work(struct work_struct *work)
>>>        mutex_lock(&gdrm->damage_lock);
>>>        fb = gdrm->fb;
>>>        gdrm->fb = NULL;
>>> +    iosys_map_set_vaddr(&shadow_map, gdrm->shadow_buf);
>>>        damage = gdrm->damage;
>>>        gud_clear_damage(gdrm);
>>>        mutex_unlock(&gdrm->damage_lock);
>>> @@ -376,33 +377,33 @@ void gud_flush_work(struct work_struct *work)
>>>        if (!fb)
>>>            goto out;
>>>    -    ret = drm_gem_fb_vmap(fb, &gem_map, &fb_map);
>>> -    if (ret)
>>> -        goto fb_put;
>>> +    gud_flush_damage(gdrm, fb, &shadow_map, true, &damage);
>>>    -    ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>>> -    if (ret)
>>> -        goto vunmap;
>>> -
>>> -    /* Imported buffers are assumed to be WriteCombined with uncached
>>> reads */
>>> -    gud_flush_damage(gdrm, fb, &fb_map, !fb->obj[0]->import_attach,
>>> &damage);
>>> -
>>> -    drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>> -vunmap:
>>> -    drm_gem_fb_vunmap(fb, &gem_map);
>>> -fb_put:
>>>        drm_framebuffer_put(fb);
>>>    out:
>>>        drm_dev_exit(idx);
>>>    }
>>>    -static void gud_fb_queue_damage(struct gud_device *gdrm, struct
>>> drm_framebuffer *fb,
>>> -                struct drm_rect *damage)
>>> +static int gud_fb_queue_damage(struct gud_device *gdrm, struct
>>> drm_framebuffer *fb,
>>> +                   const struct iosys_map *map, struct drm_rect *damage)
>>>    {
>>>        struct drm_framebuffer *old_fb = NULL;
>>> +    struct iosys_map shadow_map;
>>>          mutex_lock(&gdrm->damage_lock);
>>>    +    if (!gdrm->shadow_buf) {
>>> +        gdrm->shadow_buf = vzalloc(fb->pitches[0] * fb->height);
>>> +        if (!gdrm->shadow_buf) {
>>> +            mutex_unlock(&gdrm->damage_lock);
>>> +            return -ENOMEM;
>>> +        }
>>> +    }
>>> +
>>> +    iosys_map_set_vaddr(&shadow_map, gdrm->shadow_buf);
>>> +    iosys_map_incr(&shadow_map, drm_fb_clip_offset(fb->pitches[0],
>>> fb->format, damage));
>>> +    drm_fb_memcpy(&shadow_map, fb->pitches, map, fb, damage);
>>> +
>>
>> It's all good (sans the begin_cpu_access issue) in terms of code, but
>> the memcpy here can have quite an impact on performace.
>>
> 
> Yes if the gud display is the only one in the rendering loop it's not
> good. This is on an old HP laptop running the async path (RGB565 over USB):
> 
> $ modetest -M gud -s 36:1920x1080 -v
> setting mode 1920x1080-60.00Hz on connectors 36, crtc 33
> freq: 414.56Hz
> freq: 439.71Hz
> 
> This is the normal path:
> 
> $ modetest -M gud -s 36:1920x1080 -v
> setting mode 1920x1080-60.00Hz on connectors 36, crtc 33
> freq: 18.11Hz
> freq: 17.98Hz
> 
>> So why not just nuke the async code entirely? It's a userspace problem
>> and we don't have many(/any?) other drivers with such a workaround. Udl,
>> our other usb-based driver, works well enough without.
>>
> 
> I want to do it gradually so I did consider switching the default to
> synchronous flushing in this patchset. I think maybe I should just do
> that since you say it "works well enough".
> 
> I took the async idea from tiny/gm12u320 and Hans explained the need for
> the async worker. He's well versed with userspace so I did the same. But
> that is 3 years ago and things change. My long term plan is to drop the
> async path, but I want to keep it until I know there are no problems.

If the performace is a fundamental problem it may deserve a DRM-wide 
solution. I once looked into that briefly for udl, but it didn't seem to 
be that important then.

Personally, I'd rather push this problem to userspace.

> 
> I've seen some reports about the udl driver having horrible performance,
> but I haven't seen any explanations as to why. It could be because GNOME
> until recently[1] didn't provide damage info to the kernel.

We recently made quite a few improvements to udl, with the most 
important for performance probably being commit 0a80005d3c5f ("drm/udl: 
Enable damage clipping"). Without, each screen update transfered a full 
frame to the adapter. OTOH trying to transfer full-size FullHD frames 
over USB2 will still be slow.

> 
> The GNOME rendering loop issue was solved in 3.38[2]

Ah, I see. Thanks.

Best regards
Thomas

> 
> Noralf.
> 
> [1] https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1879
> [2]
> https://blogs.gnome.org/shell-dev/2020/07/02/splitting-up-the-frame-clock/
> 
>> Best regards
>> Thomas
>>
>>>        if (fb != gdrm->fb) {
>>>            old_fb = gdrm->fb;
>>>            drm_framebuffer_get(fb);
>>> @@ -420,6 +421,26 @@ static void gud_fb_queue_damage(struct gud_device
>>> *gdrm, struct drm_framebuffer
>>>          if (old_fb)
>>>            drm_framebuffer_put(old_fb);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void gud_fb_handle_damage(struct gud_device *gdrm, struct
>>> drm_framebuffer *fb,
>>> +                 const struct iosys_map *map, struct drm_rect *damage)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
>>> +        drm_rect_init(damage, 0, 0, fb->width, fb->height);
>>> +
>>> +    if (gud_async_flush) {
>>> +        ret = gud_fb_queue_damage(gdrm, fb, map, damage);
>>> +        if (ret != -ENOMEM)
>>> +            return;
>>> +    }
>>> +
>>> +    /* Imported buffers are assumed to be WriteCombined with uncached
>>> reads */
>>> +    gud_flush_damage(gdrm, fb, map, !fb->obj[0]->import_attach, damage);
>>>    }
>>>      int gud_pipe_check(struct drm_simple_display_pipe *pipe,
>>> @@ -544,6 +565,7 @@ void gud_pipe_update(struct
>>> drm_simple_display_pipe *pipe,
>>>        struct drm_device *drm = pipe->crtc.dev;
>>>        struct gud_device *gdrm = to_gud_device(drm);
>>>        struct drm_plane_state *state = pipe->plane.state;
>>> +    struct drm_shadow_plane_state *shadow_plane_state =
>>> to_drm_shadow_plane_state(state);
>>>        struct drm_framebuffer *fb = state->fb;
>>>        struct drm_crtc *crtc = &pipe->crtc;
>>>        struct drm_rect damage;
>>> @@ -557,6 +579,8 @@ void gud_pipe_update(struct
>>> drm_simple_display_pipe *pipe,
>>>                gdrm->fb = NULL;
>>>            }
>>>            gud_clear_damage(gdrm);
>>> +        vfree(gdrm->shadow_buf);
>>> +        gdrm->shadow_buf = NULL;
>>>            mutex_unlock(&gdrm->damage_lock);
>>>        }
>>>    @@ -572,13 +596,8 @@ void gud_pipe_update(struct
>>> drm_simple_display_pipe *pipe,
>>>        if (crtc->state->active_changed)
>>>            gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE,
>>> crtc->state->active);
>>>    -    if (drm_atomic_helper_damage_merged(old_state, state, &damage)) {
>>> -        if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
>>> -            drm_rect_init(&damage, 0, 0, fb->width, fb->height);
>>> -        gud_fb_queue_damage(gdrm, fb, &damage);
>>> -        if (!gud_async_flush)
>>> -            flush_work(&gdrm->work);
>>> -    }
>>> +    if (drm_atomic_helper_damage_merged(old_state, state, &damage))
>>> +        gud_fb_handle_damage(gdrm, fb, &shadow_plane_state->data[0],
>>> &damage);
>>>          if (!crtc->state->enable)
>>>            gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
>>>
>>

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