Re: drm/fbdev-dma: regression

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi


Am 29.11.24 um 00:37 schrieb Nuno Gonçalves:
>> Here's a first attempt to address this bug. Could you please apply the
>> attached patch and report on the results? It should work against the
>> upcoming v6.13-rc1 or against a recent drm-misc-next.
> Hi. No luck yet.

Thanks for testing. Attached is another patch. It attempts to restore 
the fbdev handling as before the regression. Please give it a try.

Best regards
Thomas

>
> I collected crashes before the patch:
>
> BUG: Bad page map in process husband  pte:60000001b46fcb pmd:800000002736403
> page: refcount:1 mapcount:-1 mapping:000000005cb4182c index:0x6 pfn:0x1b46
> aops:0xffff8000807722e0 ino:15 dentry name(?):"fb0"
> flags: 0x14(referenced|dirty|zone=0)
> raw: 0000000000000014 dead000000000100 dead000000000122 ffff0000018cf348
> raw: 0000000000000006 0000000000000000 00000001fffffffe 0000000000000000
> page dumped because: bad pte
> addr:0000ffff87b4e000 vm_flags:040400fb anon_vma:0000000000000000
> mapping:ffff0000018cf348 index:6
> file:fb0 fault:fb_deferred_io_fault mmap:fb_mmap read_folio:0x0
> CPU: 2 UID: 0 PID: 313 Comm: husband Not tainted 6.12.0 #1
> Hardware name: Raspberry Pi Compute Module 3 Rev 1.0 (DT)
> Call trace:
>   show_stack+0x18/0x30 (C)
>   dump_stack_lvl+0x60/0x80
>   dump_stack+0x18/0x24
>   print_bad_pte+0x174/0x1d0
>   unmap_page_range+0x47c/0xc90
>   unmap_vmas+0xa4/0x100
>   exit_mmap+0xbc/0x2c0
>   mmput+0x70/0x190
>   do_exit+0x248/0x8f0
>   do_group_exit+0x34/0x90
>   get_signal+0x834/0x860
>   do_signal+0xf4/0x330
>   do_notify_resume+0xc0/0x140
>   el0_svc+0xb8/0xd0
>   el0t_64_sync_handler+0x10c/0x140
>   el0t_64_sync+0x198/0x19c
> Disabling lock debugging due to kernel taint
>
> And after the patch:
>
> lk_smartMem abort info:
>   /tmp/0c  ESR = 0x0000000096000044
> 8081f6-1  EC = 0x25: DABT (current EL), IL = 32 bits
> 165-4fd2  SET = 0, FnV = 0
> -5e4e03b  EA = 0, S1PTW = 0
> e-57fa8e  FSC = 0x04: level 0 translation fault
> 22.dmp
> Data abort info:
>    ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
>    CM = 0, WnR = 1, TnD = 0, TagAccess = 0
>    GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [ffbf801f8e7bf010] address between user and kernel address ranges
> Internal error: Oops: 0000000096000044 [#1] SMP
> CPU: 1 UID: 0 PID: 260 Comm: husband Not tainted 6.12.0 #1
> Hardware name: Raspberry Pi Compute Module 3 Rev 1.0 (DT)
> pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : tlb_remove_table+0xc8/0x1c0
> lr : tlb_remove_table+0xc0/0x1c0
> sp : ffff80008117b9d0
> x29: ffff80008117b9d0 x28: ffff80008117bb98 x27: 0000aaaad1df8000
> x26: 0000aaaad1df7fff x25: ffff0000044f4468 x24: 00000000ffffffdb
> x23: fffffdffc008f2c0 x22: ffff8000808a1860 x21: 0000aaaad1c00000
> x20: fffffdffc008f2c0 x19: ffff80008117bb98 x18: ffff80008117bb50
> x17: ffffffffffffffff x16: 0000000000000000 x15: ffff8000807b62a8
> x14: 0000ffffb7640fff x13: 0000000000000000 x12: 0000000000000000
> x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000037b6f
> x8 : ffff80008094a0e7 x7 : 0000000000000800 x6 : 0000000000000000
> x5 : 0000000000000801 x4 : dead000000000122 x3 : 0000000000000000
> x2 : 0000020040000000 x1 : ffff000000000000 x0 : ffbf801f8e7bf000
> Call trace:
>   tlb_remove_table+0xc8/0x1c0 (P)
>   tlb_remove_table+0xc0/0x1c0 (L)
>   free_pgd_range+0x228/0x5d0
>   free_pgtables+0x1c4/0x270
>   exit_mmap+0x130/0x2c0
>   mmput+0x70/0x190
>   do_exit+0x248/0x8f0
>   do_group_exit+0x34/0x90
>   get_signal+0x834/0x860
>   do_signal+0x118/0x330
>   do_notify_resume+0xc0/0x140
>   el0_svc+0xb8/0xd0
>   el0t_64_sync_handler+0x10c/0x140
>   el0t_64_sync+0x198/0x19c
> Code: 52850000 94006fdd f9000660 b4000080 (b900101f)
> ---[ end trace 0000000000000000 ]---
> Fixing recursive fault but reboot is needed!
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000008
> Mem abort info:
>    ESR = 0x0000000096000044
>    EC = 0x25: DABT (current EL), IL = 32 bits
>    SET = 0, FnV = 0
>    EA = 0, S1PTW = 0
>    FSC = 0x04: level 0 translation fault
> Data abort info:
>    ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
>    CM = 0, WnR = 1, TnD = 0, TagAccess = 0
>    GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> user pgtable: 4k pages, 48-bit VAs, pgdp=000000000238c000
> [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
> Internal error: Oops: 0000000096000044 [#2] SMP
> CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G      D            6.12.0 #1
> Tainted: [D]=DIE
> Hardware name: Raspberry Pi Compute Module 3 Rev 1.0 (DT)
> pstate: a0000005 (NzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : free_unref_page_commit+0x70/0x2c0
> lr : free_unref_page+0x17c/0x400
> sp : ffff800080003da0
> x29: ffff800080003da0 x28: 0000000000000000 x27: 0000000000000000
> x26: 0000000000000000 x25: ffff00003e39e0b8 x24: 0000000000000000
> x23: 0000000000000000 x22: 00000000ffffffff x21: ffff00003e39efc0
> x20: 0000000000000000 x19: ffff800080add340 x18: ffff800081163c40
> x17: ffff7fffbda55000 x16: ffff800080000000 x15: 0000000000001900
> x14: 00000f423fffe700 x13: 0000000000000000 x12: ffff00003e39efc0
> x11: 0000000000000001 x10: 0000000000000001 x9 : ffff7fffbda55000
> x8 : fffffdffc00e81c8 x7 : ffff00003e39efe0 x6 : 0000000000000020
> x5 : ffff00003e39efc0 x4 : 00000000ffffffff x3 : 0000000000000000
> x2 : fffffdffc00e81c0 x1 : 0000000000000000 x0 : ffff800080add340
> Call trace:
>   free_unref_page_commit+0x70/0x2c0 (P)
>   free_unref_page+0x17c/0x400 (L)
>   free_unref_page+0x17c/0x400
>   __folio_put+0x50/0xb0
>   tlb_remove_table_rcu+0xb0/0xc0
>   rcu_core+0x1f4/0x520
>   rcu_core_si+0x10/0x20
>   handle_softirqs+0xf4/0x230
>   __do_softirq+0x14/0x20
>   ____do_softirq+0x10/0x20
>   call_on_irq_stack+0x24/0x54
>   do_softirq_own_stack+0x1c/0x30
>   __irq_exit_rcu+0xc8/0x100
>   irq_exit_rcu+0x10/0x20
>   el1_interrupt+0x38/0x60
>   el1h_64_irq_handler+0x18/0x30
>   el1h_64_irq+0x6c/0x70
>   default_idle_call+0x28/0x3c (P)
>   default_idle_call+0x24/0x3c (L)
>   do_idle+0x9c/0xf0
>   cpu_startup_entry+0x34/0x40
>   rest_init+0xb8/0xc0
>   do_one_initcall+0x0/0x16c
>   __primary_switched+0x88/0x90
> Code: 8b2ac0e7 f8296827 8b0600a7 f9401181 (f9000428)
> ---[ end trace 0000000000000000 ]---
> Kernel panic - not syncing: Oops: Fatal exception in interrupt
> SMP: stopping secondary CPUs
> Kernel Offset: disabled
> CPU features: 0x000,00000c00,00800000,0200420b
> Memory Limit: none
> ---[ end Kernel panic - not syncing: Oops: Fatal exception in interrupt ]---
>
> Thanks,
> Nuno

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

[-- Attachment #2: 0001-fbdev-dma-Add-shadow-buffering-for-deferred-I-O.patch --]
[-- Type: text/x-patch, Size: 9321 bytes --]

From e4e91dbee6b49c6887a5b82100e43900b24fe0a7 Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <tzimmermann@suse.de>
Date: Thu, 28 Nov 2024 11:11:25 +0100
Subject: [PATCH] fbdev-dma: Add shadow buffering for deferred I/O

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fbdev_dma.c | 213 +++++++++++++++++++++++---------
 1 file changed, 153 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
index b14b581c059d..5d4c5f5d0825 100644
--- a/drivers/gpu/drm/drm_fbdev_dma.c
+++ b/drivers/gpu/drm/drm_fbdev_dma.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: MIT
 
 #include <linux/fb.h>
+#include <linux/vmalloc.h>
 
 #include <drm/drm_drv.h>
 #include <drm/drm_fbdev_dma.h>
@@ -74,33 +75,98 @@ FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS(drm_fbdev_dma,
 				   drm_fb_helper_damage_range,
 				   drm_fb_helper_damage_area);
 
-static int drm_fbdev_dma_deferred_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
+static void drm_fbdev_dma_deferred_fb_destroy(struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
-	struct drm_framebuffer *fb = fb_helper->fb;
-	struct drm_gem_dma_object *dma = drm_fb_dma_get_gem_obj(fb, 0);
+	void *shadow = info->screen_buffer;
+
+	if (!fb_helper->dev)
+		return;
 
-	if (!dma->map_noncoherent)
-		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+	if (info->fbdefio)
+		fb_deferred_io_cleanup(info);
+	drm_fb_helper_fini(fb_helper);
+	vfree(shadow);
 
-	return fb_deferred_io_mmap(info, vma);
+	drm_client_buffer_vunmap(fb_helper->buffer);
+	drm_client_framebuffer_delete(fb_helper->buffer);
+	drm_client_release(&fb_helper->client);
+	drm_fb_helper_unprepare(fb_helper);
+	kfree(fb_helper);
 }
 
 static const struct fb_ops drm_fbdev_dma_deferred_fb_ops = {
 	.owner = THIS_MODULE,
 	.fb_open = drm_fbdev_dma_fb_open,
 	.fb_release = drm_fbdev_dma_fb_release,
-	__FB_DEFAULT_DEFERRED_OPS_RDWR(drm_fbdev_dma),
+	FB_DEFAULT_DEFERRED_OPS(drm_fbdev_dma),
 	DRM_FB_HELPER_DEFAULT_OPS,
-	__FB_DEFAULT_DEFERRED_OPS_DRAW(drm_fbdev_dma),
-	.fb_mmap = drm_fbdev_dma_deferred_fb_mmap,
-	.fb_destroy = drm_fbdev_dma_fb_destroy,
+	.fb_destroy = drm_fbdev_dma_deferred_fb_destroy,
 };
 
 /*
  * struct drm_fb_helper
  */
 
+static void drm_fbdev_dma_damage_blit_real(struct drm_fb_helper *fb_helper,
+					   struct drm_clip_rect *clip,
+					   struct iosys_map *dst)
+{
+	struct drm_framebuffer *fb = fb_helper->fb;
+	size_t offset = clip->y1 * fb->pitches[0];
+	size_t len = clip->x2 - clip->x1;
+	unsigned int y;
+	void *src;
+
+	switch (drm_format_info_bpp(fb->format, 0)) {
+	case 1:
+		offset += clip->x1 / 8;
+		len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
+		break;
+	case 2:
+		offset += clip->x1 / 4;
+		len = DIV_ROUND_UP(len + clip->x1 % 4, 4);
+		break;
+	case 4:
+		offset += clip->x1 / 2;
+		len = DIV_ROUND_UP(len + clip->x1 % 2, 2);
+		break;
+	default:
+		offset += clip->x1 * fb->format->cpp[0];
+		len *= fb->format->cpp[0];
+		break;
+	}
+
+	src = fb_helper->info->screen_buffer + offset;
+	iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
+
+	for (y = clip->y1; y < clip->y2; y++) {
+		iosys_map_memcpy_to(dst, 0, src, len);
+		iosys_map_incr(dst, fb->pitches[0]);
+		src += fb->pitches[0];
+	}
+}
+
+static int drm_fbdev_dma_damage_blit(struct drm_fb_helper *fb_helper,
+				     struct drm_clip_rect *clip)
+{
+	struct drm_client_buffer *buffer = fb_helper->buffer;
+	struct iosys_map dst;
+
+	/*
+	 * For fbdev emulation, we only have to protect against fbdev modeset
+	 * operations. Nothing else will involve the client buffer's BO. So it
+	 * is sufficient to acquire struct drm_fb_helper.lock here.
+	 */
+	mutex_lock(&fb_helper->lock);
+
+	dst = buffer->map;
+	drm_fbdev_dma_damage_blit_real(fb_helper, clip, &dst);
+
+	mutex_unlock(&fb_helper->lock);
+
+	return 0;
+}
 static int drm_fbdev_dma_helper_fb_dirty(struct drm_fb_helper *helper,
 					 struct drm_clip_rect *clip)
 {
@@ -112,6 +178,10 @@ static int drm_fbdev_dma_helper_fb_dirty(struct drm_fb_helper *helper,
 		return 0;
 
 	if (helper->fb->funcs->dirty) {
+		ret = drm_fbdev_dma_damage_blit(helper, clip);
+		if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
+			return ret;
+
 		ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
 		if (drm_WARN_ONCE(dev, ret, "Dirty helper failed: ret=%d\n", ret))
 			return ret;
@@ -128,14 +198,80 @@ static const struct drm_fb_helper_funcs drm_fbdev_dma_helper_funcs = {
  * struct drm_fb_helper
  */
 
+static int drm_fbdev_dma_driver_fbdev_probe_tail(struct drm_fb_helper *fb_helper,
+						 struct drm_fb_helper_surface_size *sizes)
+{
+	struct drm_device *dev = fb_helper->dev;
+	struct drm_client_buffer *buffer = fb_helper->buffer;
+	struct drm_gem_dma_object *dma_obj = to_drm_gem_dma_obj(buffer->gem);
+	struct drm_framebuffer *fb = fb_helper->fb;
+	struct fb_info *info = fb_helper->info;
+	struct iosys_map map = buffer->map;
+
+	info->fbops = &drm_fbdev_dma_fb_ops;
+
+	/* screen */
+	info->flags |= FBINFO_VIRTFB; /* system memory */
+	if (dma_obj->map_noncoherent)
+		info->flags |= FBINFO_READS_FAST; /* signal caching */
+	info->screen_size = sizes->surface_height * fb->pitches[0];
+	info->screen_buffer = map.vaddr;
+	if (!(info->flags & FBINFO_HIDE_SMEM_START)) {
+		if (!drm_WARN_ON(dev, is_vmalloc_addr(info->screen_buffer)))
+			info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer));
+	}
+	info->fix.smem_len = info->screen_size;
+
+	return 0;
+}
+
+static int drm_fbdev_dma_driver_fbdev_probe_tail_deferred(struct drm_fb_helper *fb_helper,
+							  struct drm_fb_helper_surface_size *sizes)
+{
+	struct drm_client_buffer *buffer = fb_helper->buffer;
+	struct fb_info *info = fb_helper->info;
+	size_t screen_size = buffer->gem->size;
+	void *screen_buffer;
+	int ret;
+
+	/*
+	 * Deferred I/O requires struct page for framebuffer memory,
+	 * which is not guaranteed for all DMA ranges. We thus create
+	 * a shadow buffer in system memory.
+	 */
+	screen_buffer = vzalloc(screen_size);
+	if (!screen_buffer)
+		return -ENOMEM;
+
+	info->fbops = &drm_fbdev_dma_deferred_fb_ops;
+
+	/* screen */
+	info->flags |= FBINFO_VIRTFB; /* system memory */
+	info->flags |= FBINFO_READS_FAST; /* signal caching */
+	info->screen_buffer = screen_buffer;
+	info->fix.smem_len = screen_size;
+
+	fb_helper->fbdefio.delay = HZ / 20;
+	fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
+
+	info->fbdefio = &fb_helper->fbdefio;
+	ret = fb_deferred_io_init(info);
+	if (ret)
+		goto err_vfree;
+
+	return 0;
+
+err_vfree:
+	vfree(screen_buffer);
+	return ret;
+}
+
 int drm_fbdev_dma_driver_fbdev_probe(struct drm_fb_helper *fb_helper,
 				     struct drm_fb_helper_surface_size *sizes)
 {
 	struct drm_client_dev *client = &fb_helper->client;
 	struct drm_device *dev = fb_helper->dev;
-	bool use_deferred_io = false;
 	struct drm_client_buffer *buffer;
-	struct drm_gem_dma_object *dma_obj;
 	struct drm_framebuffer *fb;
 	struct fb_info *info;
 	u32 format;
@@ -152,19 +288,9 @@ int drm_fbdev_dma_driver_fbdev_probe(struct drm_fb_helper *fb_helper,
 					       sizes->surface_height, format);
 	if (IS_ERR(buffer))
 		return PTR_ERR(buffer);
-	dma_obj = to_drm_gem_dma_obj(buffer->gem);
 
 	fb = buffer->fb;
 
-	/*
-	 * Deferred I/O requires struct page for framebuffer memory,
-	 * which is not guaranteed for all DMA ranges. We thus only
-	 * install deferred I/O if we have a framebuffer that requires
-	 * it.
-	 */
-	if (fb->funcs->dirty)
-		use_deferred_io = true;
-
 	ret = drm_client_buffer_vmap(buffer, &map);
 	if (ret) {
 		goto err_drm_client_buffer_delete;
@@ -185,45 +311,12 @@ int drm_fbdev_dma_driver_fbdev_probe(struct drm_fb_helper *fb_helper,
 
 	drm_fb_helper_fill_info(info, fb_helper, sizes);
 
-	if (use_deferred_io)
-		info->fbops = &drm_fbdev_dma_deferred_fb_ops;
+	if (fb->funcs->dirty)
+		ret = drm_fbdev_dma_driver_fbdev_probe_tail_deferred(fb_helper, sizes);
 	else
-		info->fbops = &drm_fbdev_dma_fb_ops;
-
-	/* screen */
-	info->flags |= FBINFO_VIRTFB; /* system memory */
-	if (dma_obj->map_noncoherent)
-		info->flags |= FBINFO_READS_FAST; /* signal caching */
-	info->screen_size = sizes->surface_height * fb->pitches[0];
-	info->screen_buffer = map.vaddr;
-	if (!(info->flags & FBINFO_HIDE_SMEM_START)) {
-		if (!drm_WARN_ON(dev, is_vmalloc_addr(info->screen_buffer)))
-			info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer));
-	}
-	info->fix.smem_len = info->screen_size;
-
-	/*
-	 * Only set up deferred I/O if the screen buffer supports
-	 * it. If this disagrees with the previous test for ->dirty,
-	 * mmap on the /dev/fb file might not work correctly.
-	 */
-	if (!is_vmalloc_addr(info->screen_buffer) && info->fix.smem_start) {
-		unsigned long pfn = info->fix.smem_start >> PAGE_SHIFT;
-
-		if (drm_WARN_ON(dev, !pfn_to_page(pfn)))
-			use_deferred_io = false;
-	}
-
-	/* deferred I/O */
-	if (use_deferred_io) {
-		fb_helper->fbdefio.delay = HZ / 20;
-		fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
-
-		info->fbdefio = &fb_helper->fbdefio;
-		ret = fb_deferred_io_init(info);
-		if (ret)
-			goto err_drm_fb_helper_release_info;
-	}
+		ret = drm_fbdev_dma_driver_fbdev_probe_tail(fb_helper, sizes);
+	if (ret)
+		goto err_drm_fb_helper_release_info;
 
 	return 0;
 
-- 
2.47.0