[PATCH 0/3] drm/fb-helper: Restore damage worker

[PATCH 0/3] drm/fb-helper: Restore damage worker

[Thomas Zimmermann]

It turns out that the removal of the damage worker can lead to
locking cycles. The display update now runs while holding the
fbdefio lock, which conflicts with locks in the modesetting code
that updates the display. There's no easy fix for this, so let's
restore the damage worker. The display update will then again run
outside of the fbdefio lock.

See [1] for bug reports.

[1] https://intel-gfx-ci.01.org/tree/drm-tip/fi-kbl-8809g.html

Thomas Zimmermann (3):
  Revert "drm/fb-helper: Remove damage worker"
  Revert "drm/fb-helper: Schedule deferred-I/O worker after writing to
    framebuffer"
  Revert "drm/fb-helper: Perform damage handling in deferred-I/O helper"

 drivers/gpu/drm/drm_fb_helper.c     | 30 +++++++++++++----------------
 drivers/video/fbdev/core/fb_defio.c | 16 ---------------
 include/drm/drm_fb_helper.h         |  2 ++
 include/linux/fb.h                  |  1 -
 4 files changed, 15 insertions(+), 34 deletions(-)

-- 
2.38.1

[PATCH 1/3] Revert "drm/fb-helper: Remove damage worker"

[Thomas Zimmermann]

This reverts commit 27c3e9452d552ea86369a94f23287a9675f2d7a1.

Needed to restore the fbdev damage worker. There have been bug reports
about locking order [1] and incorrectly takens branches. [2] Restore
the damage worker until these problems have been resovled.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://intel-gfx-ci.01.org/tree/drm-tip/fi-kbl-8809g.html # 1
Link: https://lore.kernel.org/dri-devel/20221115115819.23088-6-tzimmermann@suse.de/T/#m06eedc0a468940e4cbbd14ca026733b639bc445a # 2
---
 drivers/gpu/drm/drm_fb_helper.c | 9 +++++++++
 include/drm/drm_fb_helper.h     | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a1f86e436ae8e..c0e9a977a3b3c 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -403,6 +403,13 @@ static void drm_fb_helper_fb_dirty(struct drm_fb_helper *helper)
 	spin_unlock_irqrestore(&helper->damage_lock, flags);
 }
 
+static void drm_fb_helper_damage_work(struct work_struct *work)
+{
+	struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper, damage_work);
+
+	drm_fb_helper_fb_dirty(helper);
+}
+
 /**
  * drm_fb_helper_prepare - setup a drm_fb_helper structure
  * @dev: DRM device
@@ -418,6 +425,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
 	INIT_LIST_HEAD(&helper->kernel_fb_list);
 	spin_lock_init(&helper->damage_lock);
 	INIT_WORK(&helper->resume_work, drm_fb_helper_resume_worker);
+	INIT_WORK(&helper->damage_work, drm_fb_helper_damage_work);
 	helper->damage_clip.x1 = helper->damage_clip.y1 = ~0;
 	mutex_init(&helper->lock);
 	helper->funcs = funcs;
@@ -549,6 +557,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 		return;
 
 	cancel_work_sync(&fb_helper->resume_work);
+	cancel_work_sync(&fb_helper->damage_work);
 
 	info = fb_helper->info;
 	if (info) {
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 455f6c2b8117a..b111dc7ada78d 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -116,6 +116,7 @@ struct drm_fb_helper_funcs {
  * @damage_clip: clip rectangle used with deferred_io to accumulate damage to
  *                the screen buffer
  * @damage_lock: spinlock protecting @damage_clip
+ * @damage_work: worker used to flush the framebuffer
  * @resume_work: worker used during resume if the console lock is already taken
  *
  * This is the main structure used by the fbdev helpers. Drivers supporting
@@ -145,6 +146,7 @@ struct drm_fb_helper {
 	u32 pseudo_palette[17];
 	struct drm_clip_rect damage_clip;
 	spinlock_t damage_lock;
+	struct work_struct damage_work;
 	struct work_struct resume_work;
 
 	/**
-- 
2.38.1

[PATCH 2/3] Revert "drm/fb-helper: Schedule deferred-I/O worker after writing to framebuffer"

[Thomas Zimmermann]

This reverts commit 7f5cc4a3e5e4c5a38e5748defc952e45278f7a70.

Needed to restore the fbdev damage worker. There have been bug reports
about locking order [1] and incorrectly takens branches. [2] Restore
the damage worker until these problems have been resovled.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://intel-gfx-ci.01.org/tree/drm-tip/fi-kbl-8809g.html # 1
Link: https://lore.kernel.org/dri-devel/20221115115819.23088-6-tzimmermann@suse.de/T/#m06eedc0a468940e4cbbd14ca026733b639bc445a # 2
---
 drivers/gpu/drm/drm_fb_helper.c     |  9 +--------
 drivers/video/fbdev/core/fb_defio.c | 16 ----------------
 include/linux/fb.h                  |  1 -
 3 files changed, 1 insertion(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index c0e9a977a3b3c..cdbf03e941b2b 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -599,16 +599,9 @@ static void drm_fb_helper_add_damage_clip(struct drm_fb_helper *helper, u32 x, u
 static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
 				 u32 width, u32 height)
 {
-	struct fb_info *info = helper->info;
-
 	drm_fb_helper_add_damage_clip(helper, x, y, width, height);
 
-	/*
-	 * The current fbdev emulation only flushes buffers if a damage
-	 * update is necessary. And we can assume that deferred I/O has
-	 * been enabled as damage updates require deferred I/O for mmap.
-	 */
-	fb_deferred_io_schedule_flush(info);
+	schedule_work(&helper->damage_work);
 }
 
 /*
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index dec678f72a42f..c730253ab85ce 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -332,19 +332,3 @@ void fb_deferred_io_cleanup(struct fb_info *info)
 	mutex_destroy(&fbdefio->lock);
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
-
-void fb_deferred_io_schedule_flush(struct fb_info *info)
-{
-	struct fb_deferred_io *fbdefio = info->fbdefio;
-
-	if (WARN_ON_ONCE(!fbdefio))
-		return; /* bug in driver logic */
-
-	/*
-	 * There's no requirement from callers to schedule the
-	 * flush immediately. Rather schedule the worker with a
-	 * delay and let a few more writes pile up.
-	 */
-	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
-}
-EXPORT_SYMBOL_GPL(fb_deferred_io_schedule_flush);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 3a822e4357b12..e6b8e7ad7ac80 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -663,7 +663,6 @@ extern void fb_deferred_io_open(struct fb_info *info,
 				struct inode *inode,
 				struct file *file);
 extern void fb_deferred_io_cleanup(struct fb_info *info);
-extern void fb_deferred_io_schedule_flush(struct fb_info *info);
 extern int fb_deferred_io_fsync(struct file *file, loff_t start,
 				loff_t end, int datasync);
 
-- 
2.38.1

[PATCH 3/3] Revert "drm/fb-helper: Perform damage handling in deferred-I/O helper"

[Thomas Zimmermann]

This reverts commit 5fc586a058babc71c82a038477581b7bfe1c6e2a.

Needed to restore the fbdev damage worker. There have been bug reports
about locking order [1] and incorrectly takens branches. [2] Restore
the damage worker until these problems have been resovled.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://intel-gfx-ci.01.org/tree/drm-tip/fi-kbl-8809g.html # 1
Link: https://lore.kernel.org/dri-devel/20221115115819.23088-6-tzimmermann@suse.de/T/#m06eedc0a468940e4cbbd14ca026733b639bc445a # 2
---
 drivers/gpu/drm/drm_fb_helper.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index cdbf03e941b2b..b3a731b9170a6 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -666,16 +666,10 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
 
 	if (min_off < max_off) {
 		drm_fb_helper_memory_range_to_clip(info, min_off, max_off - min_off, &damage_area);
-		drm_fb_helper_add_damage_clip(helper, damage_area.x1, damage_area.y1,
-					      drm_rect_width(&damage_area),
-					      drm_rect_height(&damage_area));
+		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
+				     drm_rect_width(&damage_area),
+				     drm_rect_height(&damage_area));
 	}
-
-	/*
-	 * Flushes all dirty pages from mmap's pageref list and the
-	 * areas that have been written by struct fb_ops callbacks.
-	 */
-	drm_fb_helper_fb_dirty(helper);
 }
 EXPORT_SYMBOL(drm_fb_helper_deferred_io);
 
-- 
2.38.1

Re: [PATCH 0/3] drm/fb-helper: Restore damage worker

[Daniel Vetter]

On Fri, Nov 18, 2022 at 02:35:32PM +0100, Thomas Zimmermann wrote:
> It turns out that the removal of the damage worker can lead to
> locking cycles. The display update now runs while holding the
> fbdefio lock, which conflicts with locks in the modesetting code
> that updates the display. There's no easy fix for this, so let's
> restore the damage worker. The display update will then again run
> outside of the fbdefio lock.
> 
> See [1] for bug reports.
> 
> [1] https://intel-gfx-ci.01.org/tree/drm-tip/fi-kbl-8809g.html

:-(
 
> Thomas Zimmermann (3):
>   Revert "drm/fb-helper: Remove damage worker"
>   Revert "drm/fb-helper: Schedule deferred-I/O worker after writing to
>     framebuffer"
>   Revert "drm/fb-helper: Perform damage handling in deferred-I/O helper"

On the series: Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Apologies for the late ack, I wasn't around yesterday afternoon.
-Daniel

> 
>  drivers/gpu/drm/drm_fb_helper.c     | 30 +++++++++++++----------------
>  drivers/video/fbdev/core/fb_defio.c | 16 ---------------
>  include/drm/drm_fb_helper.h         |  2 ++
>  include/linux/fb.h                  |  1 -
>  4 files changed, 15 insertions(+), 34 deletions(-)
> 
> -- 
> 2.38.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch