[PATCH 0/5] drm/fb-helper: Remove damage worker

[PATCH 0/5] drm/fb-helper: Remove damage worker

[Thomas Zimmermann]

The fbdev emulation runs a separate worker that collects all changes
to the framebuffer and runs DRM damage handling. But this can also be
performed by the worker of fbdev's deferred-I/O code. Move damage
handling there, remove the damage worker and reduce the latency.

I ran a simple benchmark with mplayer and Big Buck Bunny [1] on the
console to look for any changes to performance.

  mplayer -nosound -vo fbdev -benchmark big_buck_bunny_1080p_surround.avi

On my test system (i7-4790, simpledrm, 1024x768), this command always
takes around 95 seconds (57% VC, 36% VO, 7% sys). The difference in
internal scheduling appears to have no impact on performance.

[1] https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_1080p_surround.avi

Thomas Zimmermann (5):
  drm/fb-helper: Set damage-clip area in helper
  drm/fb-helper: Move dirty-fb update into helper function
  drm/fb-helper: Perform damage handling in deferred-I/O helper
  drm/fb-helper: Schedule deferred-I/O worker after writing to
    framebuffer
  drm/fb-helper: Remove damage worker

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


base-commit: 3aa97a74d622aa26fe79cf4bd819b6a4fd176e90
-- 
2.38.0

[PATCH 1/5] drm/fb-helper: Set damage-clip area in helper

[Thomas Zimmermann]

Set the damage area in the new helper drm_fb_helper_add_damage_clip().
It can now be updated without scheduling the damage worker. This change
will help to remove the damage worker entirely. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index e0384f967c0b3..178615565760e 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -576,8 +576,8 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 }
 EXPORT_SYMBOL(drm_fb_helper_fini);
 
-static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
-				 u32 width, u32 height)
+static void drm_fb_helper_add_damage_clip(struct drm_fb_helper *helper, u32 x, u32 y,
+					  u32 width, u32 height)
 {
 	struct drm_clip_rect *clip = &helper->damage_clip;
 	unsigned long flags;
@@ -588,6 +588,12 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
 	clip->x2 = max_t(u32, clip->x2, x + width);
 	clip->y2 = max_t(u32, clip->y2, y + height);
 	spin_unlock_irqrestore(&helper->damage_lock, flags);
+}
+
+static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
+				 u32 width, u32 height)
+{
+	drm_fb_helper_add_damage_clip(helper, x, y, width, height);
 
 	schedule_work(&helper->damage_work);
 }
-- 
2.38.0

[PATCH 2/5] drm/fb-helper: Move dirty-fb update into helper function

[Thomas Zimmermann]

Move the dirty-fb update from the damage-worker callback into the
new helper drm_fb_helper_fb_dirty(), so that it can run outside the
damage worker. This change will help to remove the damage worker
entirely. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 178615565760e..be8ecb5e50b56 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -367,9 +367,8 @@ static void drm_fb_helper_resume_worker(struct work_struct *work)
 	console_unlock();
 }
 
-static void drm_fb_helper_damage_work(struct work_struct *work)
+static void drm_fb_helper_fb_dirty(struct drm_fb_helper *helper)
 {
-	struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper, damage_work);
 	struct drm_device *dev = helper->dev;
 	struct drm_clip_rect *clip = &helper->damage_clip;
 	struct drm_clip_rect clip_copy;
@@ -404,6 +403,13 @@ static void drm_fb_helper_damage_work(struct work_struct *work)
 	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
-- 
2.38.0

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

[Thomas Zimmermann]

Call fb_dirty directly from drm_fb_helper_deferred_io() to avoid the
latency of running the damage worker.

The deferred-I/O helper drm_fb_helper_deferred_io() runs in a worker
thread at regular intervals as part of writing to mmaped framebuffer
memory. It used to schedule the fbdev damage worker to flush the
framebuffer. Changing this to flushing the framebuffer directly avoids
the latency introduced by the damage worker.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index be8ecb5e50b56..ebc44ed1bf4a2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -644,10 +644,14 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
 void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagereflist)
 {
 	struct drm_fb_helper *helper = info->par;
+	struct drm_device *dev = helper->dev;
 	unsigned long start, end, min_off, max_off;
 	struct fb_deferred_io_pageref *pageref;
 	struct drm_rect damage_area;
 
+	if (drm_WARN_ON(dev, !helper->funcs->fb_dirty))
+		return;
+
 	min_off = ULONG_MAX;
 	max_off = 0;
 	list_for_each_entry(pageref, pagereflist, list) {
@@ -656,22 +660,26 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
 		min_off = min(min_off, start);
 		max_off = max(max_off, end);
 	}
-	if (min_off >= max_off)
-		return;
 
-	if (helper->funcs->fb_dirty) {
-		/*
-		 * As we can only track pages, we might reach beyond the end
-		 * of the screen and account for non-existing scanlines. Hence,
-		 * keep the covered memory area within the screen buffer.
-		 */
-		max_off = min(max_off, info->screen_size);
+	/*
+	 * As we can only track pages, we might reach beyond the end
+	 * of the screen and account for non-existing scanlines. Hence,
+	 * keep the covered memory area within the screen buffer.
+	 */
+	max_off = min(max_off, info->screen_size);
 
+	if (min_off < max_off) {
 		drm_fb_helper_memory_range_to_clip(info, min_off, max_off - min_off, &damage_area);
-		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
-				     drm_rect_width(&damage_area),
-				     drm_rect_height(&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));
 	}
+
+	/*
+	 * 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.0

[PATCH 4/5] drm/fb-helper: Schedule deferred-I/O worker after writing to framebuffer

[Thomas Zimmermann]

Schedule the deferred-I/O worker instead of the damage worker after
writing to the fbdev framebuffer. The deferred-I/O worker then performs
the dirty-fb update. The fbdev emulation will initialize deferred I/O
for all drivers that require damage updates. It is therefore a valid
assumption that the deferred-I/O worker is present.

It would be possible to perform the damage handling directly from within
the write operation. But doing this could increase the overhead of the
write or interfere with a concurrently scheduled deferred-I/O worker.
Instead, scheduling the deferred-I/O worker with its regular delay of
50 ms removes load off the write operation and allows the deferred-I/O
worker to handle multiple write operations that arrived during the delay
time window.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c     | 81 ++++++++++++++++++++---------
 drivers/video/fbdev/core/fb_defio.c | 16 ++++++
 include/linux/fb.h                  |  1 +
 3 files changed, 72 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ebc44ed1bf4a2..8cb644e4ecf90 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -596,14 +596,6 @@ static void drm_fb_helper_add_damage_clip(struct drm_fb_helper *helper, u32 x, u
 	spin_unlock_irqrestore(&helper->damage_lock, flags);
 }
 
-static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
-				 u32 width, u32 height)
-{
-	drm_fb_helper_add_damage_clip(helper, x, y, width, height);
-
-	schedule_work(&helper->damage_work);
-}
-
 /*
  * Convert memory region into area of scanlines and pixels per
  * scanline. The parameters off and len must not reach beyond
@@ -683,6 +675,23 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
 }
 EXPORT_SYMBOL(drm_fb_helper_deferred_io);
 
+static void drm_fb_helper_flush(struct drm_fb_helper *helper)
+{
+	struct drm_device *dev = helper->dev;
+	struct fb_info *info = helper->info;
+
+	/*
+	 * For now, we assume that deferred I/O has been enabled as damage
+	 * updates require deferred I/O for a working mmap. The current
+	 * fbdev emulation does not flush buffers if no damage update is
+	 * necessary. So it's safe to assume fbdefio to be set.
+	 */
+	if (drm_WARN_ON_ONCE(dev, !info->fbdefio))
+		return;
+
+	fb_deferred_io_flush(info);
+}
+
 typedef ssize_t (*drm_fb_helper_read_screen)(struct fb_info *info, char __user *buf,
 					     size_t count, loff_t pos);
 
@@ -824,9 +833,10 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
 
 	if (helper->funcs->fb_dirty) {
 		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
-		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
-				     drm_rect_width(&damage_area),
-				     drm_rect_height(&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_flush(helper);
 	}
 
 	return ret;
@@ -847,8 +857,11 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info,
 
 	sys_fillrect(info, rect);
 
-	if (helper->funcs->fb_dirty)
-		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
+	if (helper->funcs->fb_dirty) {
+		drm_fb_helper_add_damage_clip(helper, rect->dx, rect->dy,
+					      rect->width, rect->height);
+		drm_fb_helper_flush(helper);
+	}
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);
 
@@ -866,8 +879,11 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info,
 
 	sys_copyarea(info, area);
 
-	if (helper->funcs->fb_dirty)
-		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
+	if (helper->funcs->fb_dirty) {
+		drm_fb_helper_add_damage_clip(helper, area->dx, area->dy,
+					      area->width, area->height);
+		drm_fb_helper_flush(helper);
+	}
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);
 
@@ -885,8 +901,11 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
 
 	sys_imageblit(info, image);
 
-	if (helper->funcs->fb_dirty)
-		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
+	if (helper->funcs->fb_dirty) {
+		drm_fb_helper_add_damage_clip(helper, image->dx, image->dy,
+					      image->width, image->height);
+		drm_fb_helper_flush(helper);
+	}
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
 
@@ -997,9 +1016,10 @@ ssize_t drm_fb_helper_cfb_write(struct fb_info *info, const char __user *buf,
 
 	if (helper->funcs->fb_dirty) {
 		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
-		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
-				     drm_rect_width(&damage_area),
-				     drm_rect_height(&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_flush(helper);
 	}
 
 	return ret;
@@ -1020,8 +1040,11 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info,
 
 	cfb_fillrect(info, rect);
 
-	if (helper->funcs->fb_dirty)
-		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
+	if (helper->funcs->fb_dirty) {
+		drm_fb_helper_add_damage_clip(helper, rect->dx, rect->dy,
+					      rect->width, rect->height);
+		drm_fb_helper_flush(helper);
+	}
 }
 EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
 
@@ -1039,8 +1062,11 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
 
 	cfb_copyarea(info, area);
 
-	if (helper->funcs->fb_dirty)
-		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
+	if (helper->funcs->fb_dirty) {
+		drm_fb_helper_add_damage_clip(helper, area->dx, area->dy,
+					      area->width, area->height);
+		drm_fb_helper_flush(helper);
+	}
 }
 EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);
 
@@ -1058,8 +1084,11 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
 
 	cfb_imageblit(info, image);
 
-	if (helper->funcs->fb_dirty)
-		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
+	if (helper->funcs->fb_dirty) {
+		drm_fb_helper_add_damage_clip(helper, image->dx, image->dy,
+					      image->width, image->height);
+		drm_fb_helper_flush(helper);
+	}
 }
 EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
 
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index c730253ab85ce..325d12c3a4d61 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -332,3 +332,19 @@ void fb_deferred_io_cleanup(struct fb_info *info)
 	mutex_destroy(&fbdefio->lock);
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
+
+void fb_deferred_io_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 to perform the flush immediately. So
+	 * 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_flush);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index bcb8658f5b64d..54b3b3e13522f 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -663,6 +663,7 @@ 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_flush(struct fb_info *info);
 extern int fb_deferred_io_fsync(struct file *file, loff_t start,
 				loff_t end, int datasync);
 
-- 
2.38.0

[PATCH 5/5] drm/fb-helper: Remove damage worker

[Thomas Zimmermann]

The fbdev damage worker is unused, so remove it.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 9 ---------
 include/drm/drm_fb_helper.h     | 2 --
 2 files changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 8cb644e4ecf90..47b8ef03a1f89 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -403,13 +403,6 @@ 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
@@ -425,7 +418,6 @@ 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;
@@ -557,7 +549,6 @@ 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 ecfcd2c56d95a..f37bb2832ba41 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -116,7 +116,6 @@ 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
@@ -146,7 +145,6 @@ 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.0

Re: [PATCH 4/5] drm/fb-helper: Schedule deferred-I/O worker after writing to framebuffer

[Daniel Vetter]

On Thu, Nov 10, 2022 at 02:55:18PM +0100, Thomas Zimmermann wrote:
> Schedule the deferred-I/O worker instead of the damage worker after
> writing to the fbdev framebuffer. The deferred-I/O worker then performs
> the dirty-fb update. The fbdev emulation will initialize deferred I/O
> for all drivers that require damage updates. It is therefore a valid
> assumption that the deferred-I/O worker is present.
> 
> It would be possible to perform the damage handling directly from within
> the write operation. But doing this could increase the overhead of the
> write or interfere with a concurrently scheduled deferred-I/O worker.
> Instead, scheduling the deferred-I/O worker with its regular delay of
> 50 ms removes load off the write operation and allows the deferred-I/O
> worker to handle multiple write operations that arrived during the delay
> time window.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fb_helper.c     | 81 ++++++++++++++++++++---------
>  drivers/video/fbdev/core/fb_defio.c | 16 ++++++
>  include/linux/fb.h                  |  1 +
>  3 files changed, 72 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index ebc44ed1bf4a2..8cb644e4ecf90 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -596,14 +596,6 @@ static void drm_fb_helper_add_damage_clip(struct drm_fb_helper *helper, u32 x, u
>  	spin_unlock_irqrestore(&helper->damage_lock, flags);
>  }
>  
> -static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
> -				 u32 width, u32 height)
> -{
> -	drm_fb_helper_add_damage_clip(helper, x, y, width, height);
> -
> -	schedule_work(&helper->damage_work);
> -}
> -
>  /*
>   * Convert memory region into area of scanlines and pixels per
>   * scanline. The parameters off and len must not reach beyond
> @@ -683,6 +675,23 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
>  }
>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>  
> +static void drm_fb_helper_flush(struct drm_fb_helper *helper)

Naming bikeshed, _flush usually means to run the worker and synchronously
block (i.e. flush out all pending things), which is not what's going on.
_schedule() or _queue() I think would be much better and more in line with
what linux wq and related subsystems use.
-Daniel

> +{
> +	struct drm_device *dev = helper->dev;
> +	struct fb_info *info = helper->info;
> +
> +	/*
> +	 * For now, we assume that deferred I/O has been enabled as damage
> +	 * updates require deferred I/O for a working mmap. The current
> +	 * fbdev emulation does not flush buffers if no damage update is
> +	 * necessary. So it's safe to assume fbdefio to be set.
> +	 */
> +	if (drm_WARN_ON_ONCE(dev, !info->fbdefio))
> +		return;
> +
> +	fb_deferred_io_flush(info);
> +}
> +
>  typedef ssize_t (*drm_fb_helper_read_screen)(struct fb_info *info, char __user *buf,
>  					     size_t count, loff_t pos);
>  
> @@ -824,9 +833,10 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
>  
>  	if (helper->funcs->fb_dirty) {
>  		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
> -		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
> -				     drm_rect_width(&damage_area),
> -				     drm_rect_height(&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_flush(helper);
>  	}
>  
>  	return ret;
> @@ -847,8 +857,11 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info,
>  
>  	sys_fillrect(info, rect);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, rect->dx, rect->dy,
> +					      rect->width, rect->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);
>  
> @@ -866,8 +879,11 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info,
>  
>  	sys_copyarea(info, area);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, area->dx, area->dy,
> +					      area->width, area->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);
>  
> @@ -885,8 +901,11 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
>  
>  	sys_imageblit(info, image);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, image->dx, image->dy,
> +					      image->width, image->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
>  
> @@ -997,9 +1016,10 @@ ssize_t drm_fb_helper_cfb_write(struct fb_info *info, const char __user *buf,
>  
>  	if (helper->funcs->fb_dirty) {
>  		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
> -		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
> -				     drm_rect_width(&damage_area),
> -				     drm_rect_height(&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_flush(helper);
>  	}
>  
>  	return ret;
> @@ -1020,8 +1040,11 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info,
>  
>  	cfb_fillrect(info, rect);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, rect->dx, rect->dy,
> +					      rect->width, rect->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
>  
> @@ -1039,8 +1062,11 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
>  
>  	cfb_copyarea(info, area);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, area->dx, area->dy,
> +					      area->width, area->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);
>  
> @@ -1058,8 +1084,11 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
>  
>  	cfb_imageblit(info, image);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, image->dx, image->dy,
> +					      image->width, image->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
>  
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index c730253ab85ce..325d12c3a4d61 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -332,3 +332,19 @@ void fb_deferred_io_cleanup(struct fb_info *info)
>  	mutex_destroy(&fbdefio->lock);
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
> +
> +void fb_deferred_io_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 to perform the flush immediately. So
> +	 * 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_flush);
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index bcb8658f5b64d..54b3b3e13522f 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -663,6 +663,7 @@ 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_flush(struct fb_info *info);
>  extern int fb_deferred_io_fsync(struct file *file, loff_t start,
>  				loff_t end, int datasync);
>  
> -- 
> 2.38.0
> 

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

Re: [PATCH 1/5] drm/fb-helper: Set damage-clip area in helper

[Daniel Vetter]

On Thu, Nov 10, 2022 at 02:55:15PM +0100, Thomas Zimmermann wrote:
> Set the damage area in the new helper drm_fb_helper_add_damage_clip().
> It can now be updated without scheduling the damage worker. This change
> will help to remove the damage worker entirely. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index e0384f967c0b3..178615565760e 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -576,8 +576,8 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_fini);
>  
> -static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
> -				 u32 width, u32 height)
> +static void drm_fb_helper_add_damage_clip(struct drm_fb_helper *helper, u32 x, u32 y,
> +					  u32 width, u32 height)
>  {
>  	struct drm_clip_rect *clip = &helper->damage_clip;
>  	unsigned long flags;
> @@ -588,6 +588,12 @@ static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
>  	clip->x2 = max_t(u32, clip->x2, x + width);
>  	clip->y2 = max_t(u32, clip->y2, y + height);
>  	spin_unlock_irqrestore(&helper->damage_lock, flags);
> +}
> +
> +static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
> +				 u32 width, u32 height)
> +{
> +	drm_fb_helper_add_damage_clip(helper, x, y, width, height);
>  
>  	schedule_work(&helper->damage_work);
>  }
> -- 
> 2.38.0
> 

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

Re: [PATCH 2/5] drm/fb-helper: Move dirty-fb update into helper function

[Daniel Vetter]

On Thu, Nov 10, 2022 at 02:55:16PM +0100, Thomas Zimmermann wrote:
> Move the dirty-fb update from the damage-worker callback into the
> new helper drm_fb_helper_fb_dirty(), so that it can run outside the
> damage worker. This change will help to remove the damage worker
> entirely. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 178615565760e..be8ecb5e50b56 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -367,9 +367,8 @@ static void drm_fb_helper_resume_worker(struct work_struct *work)
>  	console_unlock();
>  }
>  
> -static void drm_fb_helper_damage_work(struct work_struct *work)
> +static void drm_fb_helper_fb_dirty(struct drm_fb_helper *helper)
>  {
> -	struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper, damage_work);
>  	struct drm_device *dev = helper->dev;
>  	struct drm_clip_rect *clip = &helper->damage_clip;
>  	struct drm_clip_rect clip_copy;
> @@ -404,6 +403,13 @@ static void drm_fb_helper_damage_work(struct work_struct *work)
>  	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);

Was pondering a bit the naming, but it fits the ->fb_dirty callback, so
at least consistent with what we have. Sometimes I'm forgetting how many
layers of indirection we have just for historical reasons :-/

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +}
> +
>  /**
>   * drm_fb_helper_prepare - setup a drm_fb_helper structure
>   * @dev: DRM device
> -- 
> 2.38.0
> 

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

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

[Daniel Vetter]

On Thu, Nov 10, 2022 at 02:55:17PM +0100, Thomas Zimmermann wrote:
> Call fb_dirty directly from drm_fb_helper_deferred_io() to avoid the
> latency of running the damage worker.
> 
> The deferred-I/O helper drm_fb_helper_deferred_io() runs in a worker
> thread at regular intervals as part of writing to mmaped framebuffer
> memory. It used to schedule the fbdev damage worker to flush the
> framebuffer. Changing this to flushing the framebuffer directly avoids
> the latency introduced by the damage worker.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index be8ecb5e50b56..ebc44ed1bf4a2 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -644,10 +644,14 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
>  void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagereflist)
>  {
>  	struct drm_fb_helper *helper = info->par;
> +	struct drm_device *dev = helper->dev;
>  	unsigned long start, end, min_off, max_off;
>  	struct fb_deferred_io_pageref *pageref;
>  	struct drm_rect damage_area;
>  
> +	if (drm_WARN_ON(dev, !helper->funcs->fb_dirty))
> +		return;
> +
>  	min_off = ULONG_MAX;
>  	max_off = 0;
>  	list_for_each_entry(pageref, pagereflist, list) {
> @@ -656,22 +660,26 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
>  		min_off = min(min_off, start);
>  		max_off = max(max_off, end);
>  	}
> -	if (min_off >= max_off)
> -		return;
>  
> -	if (helper->funcs->fb_dirty) {

The replacement of this if() with the drm_WARN_ON above looks like it's a
leftover from 93e81e38e197 ("drm/fb_helper: Minimize damage-helper
overhead"), which hasn't pulled the ->fb_dirty check all the way out
fully. It confused me quite a bit until I stitched the story together.

I think splitting this out would be best, but minimally explain this in
the commit message. Either way

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -		/*
> -		 * As we can only track pages, we might reach beyond the end
> -		 * of the screen and account for non-existing scanlines. Hence,
> -		 * keep the covered memory area within the screen buffer.
> -		 */
> -		max_off = min(max_off, info->screen_size);
> +	/*
> +	 * As we can only track pages, we might reach beyond the end
> +	 * of the screen and account for non-existing scanlines. Hence,
> +	 * keep the covered memory area within the screen buffer.
> +	 */
> +	max_off = min(max_off, info->screen_size);
>  
> +	if (min_off < max_off) {
>  		drm_fb_helper_memory_range_to_clip(info, min_off, max_off - min_off, &damage_area);
> -		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
> -				     drm_rect_width(&damage_area),
> -				     drm_rect_height(&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));
>  	}
> +
> +	/*
> +	 * 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.0
> 

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

Re: [PATCH 4/5] drm/fb-helper: Schedule deferred-I/O worker after writing to framebuffer

[Daniel Vetter]

On Thu, Nov 10, 2022 at 02:55:18PM +0100, Thomas Zimmermann wrote:
> Schedule the deferred-I/O worker instead of the damage worker after
> writing to the fbdev framebuffer. The deferred-I/O worker then performs
> the dirty-fb update. The fbdev emulation will initialize deferred I/O
> for all drivers that require damage updates. It is therefore a valid
> assumption that the deferred-I/O worker is present.
> 
> It would be possible to perform the damage handling directly from within
> the write operation. But doing this could increase the overhead of the
> write or interfere with a concurrently scheduled deferred-I/O worker.
> Instead, scheduling the deferred-I/O worker with its regular delay of
> 50 ms removes load off the write operation and allows the deferred-I/O
> worker to handle multiple write operations that arrived during the delay
> time window.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fb_helper.c     | 81 ++++++++++++++++++++---------
>  drivers/video/fbdev/core/fb_defio.c | 16 ++++++
>  include/linux/fb.h                  |  1 +
>  3 files changed, 72 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index ebc44ed1bf4a2..8cb644e4ecf90 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -596,14 +596,6 @@ static void drm_fb_helper_add_damage_clip(struct drm_fb_helper *helper, u32 x, u
>  	spin_unlock_irqrestore(&helper->damage_lock, flags);
>  }
>  
> -static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
> -				 u32 width, u32 height)
> -{
> -	drm_fb_helper_add_damage_clip(helper, x, y, width, height);
> -
> -	schedule_work(&helper->damage_work);

I'm kinda not seeing the point in removing this, and ending up with 2
functions calls for every callsite. Replace the schedule_work with the
inlined drm_fb_helper_flush instead? That also avoids the naming bikeshed
in this case at least :-)


> -}
> -
>  /*
>   * Convert memory region into area of scanlines and pixels per
>   * scanline. The parameters off and len must not reach beyond
> @@ -683,6 +675,23 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
>  }
>  EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>  
> +static void drm_fb_helper_flush(struct drm_fb_helper *helper)
> +{
> +	struct drm_device *dev = helper->dev;
> +	struct fb_info *info = helper->info;
> +
> +	/*
> +	 * For now, we assume that deferred I/O has been enabled as damage
> +	 * updates require deferred I/O for a working mmap. The current
> +	 * fbdev emulation does not flush buffers if no damage update is
> +	 * necessary. So it's safe to assume fbdefio to be set.
> +	 */
> +	if (drm_WARN_ON_ONCE(dev, !info->fbdefio))
> +		return;
> +
> +	fb_deferred_io_flush(info);
> +}
> +
>  typedef ssize_t (*drm_fb_helper_read_screen)(struct fb_info *info, char __user *buf,
>  					     size_t count, loff_t pos);
>  
> @@ -824,9 +833,10 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
>  
>  	if (helper->funcs->fb_dirty) {
>  		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
> -		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
> -				     drm_rect_width(&damage_area),
> -				     drm_rect_height(&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_flush(helper);
>  	}
>  
>  	return ret;
> @@ -847,8 +857,11 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info,
>  
>  	sys_fillrect(info, rect);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, rect->dx, rect->dy,
> +					      rect->width, rect->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);
>  
> @@ -866,8 +879,11 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info,
>  
>  	sys_copyarea(info, area);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, area->dx, area->dy,
> +					      area->width, area->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);
>  
> @@ -885,8 +901,11 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
>  
>  	sys_imageblit(info, image);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, image->dx, image->dy,
> +					      image->width, image->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
>  
> @@ -997,9 +1016,10 @@ ssize_t drm_fb_helper_cfb_write(struct fb_info *info, const char __user *buf,
>  
>  	if (helper->funcs->fb_dirty) {
>  		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
> -		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
> -				     drm_rect_width(&damage_area),
> -				     drm_rect_height(&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_flush(helper);
>  	}
>  
>  	return ret;
> @@ -1020,8 +1040,11 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info,
>  
>  	cfb_fillrect(info, rect);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, rect->dx, rect->dy,
> +					      rect->width, rect->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
>  
> @@ -1039,8 +1062,11 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
>  
>  	cfb_copyarea(info, area);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, area->dx, area->dy,
> +					      area->width, area->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);
>  
> @@ -1058,8 +1084,11 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
>  
>  	cfb_imageblit(info, image);
>  
> -	if (helper->funcs->fb_dirty)
> -		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
> +	if (helper->funcs->fb_dirty) {
> +		drm_fb_helper_add_damage_clip(helper, image->dx, image->dy,
> +					      image->width, image->height);
> +		drm_fb_helper_flush(helper);
> +	}
>  }
>  EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
>  
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index c730253ab85ce..325d12c3a4d61 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -332,3 +332,19 @@ void fb_deferred_io_cleanup(struct fb_info *info)
>  	mutex_destroy(&fbdefio->lock);
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
> +
> +void fb_deferred_io_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 to perform the flush immediately. So
> +	 * schedule the worker with a delay and let a few more writes
> +	 * pile up.
> +	 */

So this part is wrong, because the drm callers do rely on this not
flushing anything immediately, but instead on scheduling the worker. Or at
least that's the reason why we have the damage worker in the first place.

So this comment here needs to go, and the functions need to make it clear
in their names that that they queue/schedule the flush.

Also not sure whether you want to split out the fbdev part or not, imo
overkill.

With comments addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> +	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
> +}
> +EXPORT_SYMBOL_GPL(fb_deferred_io_flush);
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index bcb8658f5b64d..54b3b3e13522f 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -663,6 +663,7 @@ 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_flush(struct fb_info *info);
>  extern int fb_deferred_io_fsync(struct file *file, loff_t start,
>  				loff_t end, int datasync);
>  
> -- 
> 2.38.0
> 

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

Re: [PATCH 5/5] drm/fb-helper: Remove damage worker

[Daniel Vetter]

On Thu, Nov 10, 2022 at 02:55:19PM +0100, Thomas Zimmermann wrote:
> The fbdev damage worker is unused, so remove it.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

I'd squash this into the previous patch, which gets rid of the
schedule_work(), if you limit the previous patch to really just that
change. But split out is fine too.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 9 ---------
>  include/drm/drm_fb_helper.h     | 2 --
>  2 files changed, 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 8cb644e4ecf90..47b8ef03a1f89 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -403,13 +403,6 @@ 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
> @@ -425,7 +418,6 @@ 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;
> @@ -557,7 +549,6 @@ 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 ecfcd2c56d95a..f37bb2832ba41 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -116,7 +116,6 @@ 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
> @@ -146,7 +145,6 @@ 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.0
> 

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

Re: [PATCH 4/5] drm/fb-helper: Schedule deferred-I/O worker after writing to framebuffer

[Thomas Zimmermann]

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

Hi

Am 11.11.22 um 10:28 schrieb Daniel Vetter:
> On Thu, Nov 10, 2022 at 02:55:18PM +0100, Thomas Zimmermann wrote:
>> Schedule the deferred-I/O worker instead of the damage worker after
>> writing to the fbdev framebuffer. The deferred-I/O worker then performs
>> the dirty-fb update. The fbdev emulation will initialize deferred I/O
>> for all drivers that require damage updates. It is therefore a valid
>> assumption that the deferred-I/O worker is present.
>>
>> It would be possible to perform the damage handling directly from within
>> the write operation. But doing this could increase the overhead of the
>> write or interfere with a concurrently scheduled deferred-I/O worker.
>> Instead, scheduling the deferred-I/O worker with its regular delay of
>> 50 ms removes load off the write operation and allows the deferred-I/O
>> worker to handle multiple write operations that arrived during the delay
>> time window.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c     | 81 ++++++++++++++++++++---------
>>   drivers/video/fbdev/core/fb_defio.c | 16 ++++++
>>   include/linux/fb.h                  |  1 +
>>   3 files changed, 72 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index ebc44ed1bf4a2..8cb644e4ecf90 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -596,14 +596,6 @@ static void drm_fb_helper_add_damage_clip(struct drm_fb_helper *helper, u32 x, u
>>   	spin_unlock_irqrestore(&helper->damage_lock, flags);
>>   }
>>   
>> -static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
>> -				 u32 width, u32 height)
>> -{
>> -	drm_fb_helper_add_damage_clip(helper, x, y, width, height);
>> -
>> -	schedule_work(&helper->damage_work);
> 
> I'm kinda not seeing the point in removing this, and ending up with 2
> functions calls for every callsite. Replace the schedule_work with the
> inlined drm_fb_helper_flush instead? That also avoids the naming bikeshed
> in this case at least :-)
> 
> 
>> -}
>> -
>>   /*
>>    * Convert memory region into area of scanlines and pixels per
>>    * scanline. The parameters off and len must not reach beyond
>> @@ -683,6 +675,23 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>>   
>> +static void drm_fb_helper_flush(struct drm_fb_helper *helper)
>> +{
>> +	struct drm_device *dev = helper->dev;
>> +	struct fb_info *info = helper->info;
>> +
>> +	/*
>> +	 * For now, we assume that deferred I/O has been enabled as damage
>> +	 * updates require deferred I/O for a working mmap. The current
>> +	 * fbdev emulation does not flush buffers if no damage update is
>> +	 * necessary. So it's safe to assume fbdefio to be set.
>> +	 */
>> +	if (drm_WARN_ON_ONCE(dev, !info->fbdefio))
>> +		return;
>> +
>> +	fb_deferred_io_flush(info);
>> +}
>> +
>>   typedef ssize_t (*drm_fb_helper_read_screen)(struct fb_info *info, char __user *buf,
>>   					     size_t count, loff_t pos);
>>   
>> @@ -824,9 +833,10 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
>>   
>>   	if (helper->funcs->fb_dirty) {
>>   		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
>> -		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
>> -				     drm_rect_width(&damage_area),
>> -				     drm_rect_height(&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_flush(helper);
>>   	}
>>   
>>   	return ret;
>> @@ -847,8 +857,11 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info,
>>   
>>   	sys_fillrect(info, rect);
>>   
>> -	if (helper->funcs->fb_dirty)
>> -		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
>> +	if (helper->funcs->fb_dirty) {
>> +		drm_fb_helper_add_damage_clip(helper, rect->dx, rect->dy,
>> +					      rect->width, rect->height);
>> +		drm_fb_helper_flush(helper);
>> +	}
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);
>>   
>> @@ -866,8 +879,11 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info,
>>   
>>   	sys_copyarea(info, area);
>>   
>> -	if (helper->funcs->fb_dirty)
>> -		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
>> +	if (helper->funcs->fb_dirty) {
>> +		drm_fb_helper_add_damage_clip(helper, area->dx, area->dy,
>> +					      area->width, area->height);
>> +		drm_fb_helper_flush(helper);
>> +	}
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);
>>   
>> @@ -885,8 +901,11 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
>>   
>>   	sys_imageblit(info, image);
>>   
>> -	if (helper->funcs->fb_dirty)
>> -		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
>> +	if (helper->funcs->fb_dirty) {
>> +		drm_fb_helper_add_damage_clip(helper, image->dx, image->dy,
>> +					      image->width, image->height);
>> +		drm_fb_helper_flush(helper);
>> +	}
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
>>   
>> @@ -997,9 +1016,10 @@ ssize_t drm_fb_helper_cfb_write(struct fb_info *info, const char __user *buf,
>>   
>>   	if (helper->funcs->fb_dirty) {
>>   		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
>> -		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
>> -				     drm_rect_width(&damage_area),
>> -				     drm_rect_height(&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_flush(helper);
>>   	}
>>   
>>   	return ret;
>> @@ -1020,8 +1040,11 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info,
>>   
>>   	cfb_fillrect(info, rect);
>>   
>> -	if (helper->funcs->fb_dirty)
>> -		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
>> +	if (helper->funcs->fb_dirty) {
>> +		drm_fb_helper_add_damage_clip(helper, rect->dx, rect->dy,
>> +					      rect->width, rect->height);
>> +		drm_fb_helper_flush(helper);
>> +	}
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
>>   
>> @@ -1039,8 +1062,11 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
>>   
>>   	cfb_copyarea(info, area);
>>   
>> -	if (helper->funcs->fb_dirty)
>> -		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
>> +	if (helper->funcs->fb_dirty) {
>> +		drm_fb_helper_add_damage_clip(helper, area->dx, area->dy,
>> +					      area->width, area->height);
>> +		drm_fb_helper_flush(helper);
>> +	}
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);
>>   
>> @@ -1058,8 +1084,11 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
>>   
>>   	cfb_imageblit(info, image);
>>   
>> -	if (helper->funcs->fb_dirty)
>> -		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
>> +	if (helper->funcs->fb_dirty) {
>> +		drm_fb_helper_add_damage_clip(helper, image->dx, image->dy,
>> +					      image->width, image->height);
>> +		drm_fb_helper_flush(helper);
>> +	}
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
>>   
>> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
>> index c730253ab85ce..325d12c3a4d61 100644
>> --- a/drivers/video/fbdev/core/fb_defio.c
>> +++ b/drivers/video/fbdev/core/fb_defio.c
>> @@ -332,3 +332,19 @@ void fb_deferred_io_cleanup(struct fb_info *info)
>>   	mutex_destroy(&fbdefio->lock);
>>   }
>>   EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
>> +
>> +void fb_deferred_io_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 to perform the flush immediately. So
>> +	 * schedule the worker with a delay and let a few more writes
>> +	 * pile up.
>> +	 */
> 
> So this part is wrong, because the drm callers do rely on this not
> flushing anything immediately, but instead on scheduling the worker. Or at
> least that's the reason why we have the damage worker in the first place.

That is badly worded. The comment means that the worker does not have to 
be scheduled immediately. I'll change that.

> 
> So this comment here needs to go, and the functions need to make it clear
> in their names that that they queue/schedule the flush.

fb_deferred_io_schedule_flush() seems like the most expressive name.

The current name came from glFlush() and glFinish(), where the former 
only schedules a flushand the letter really waits before returning.

Best regards
Thomas

> 
> Also not sure whether you want to split out the fbdev part or not, imo
> overkill.
> 
> With comments addressed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
>> +	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>> +}
>> +EXPORT_SYMBOL_GPL(fb_deferred_io_flush);
>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>> index bcb8658f5b64d..54b3b3e13522f 100644
>> --- a/include/linux/fb.h
>> +++ b/include/linux/fb.h
>> @@ -663,6 +663,7 @@ 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_flush(struct fb_info *info);
>>   extern int fb_deferred_io_fsync(struct file *file, loff_t start,
>>   				loff_t end, int datasync);
>>   
>> -- 
>> 2.38.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 5/5] drm/fb-helper: Remove damage worker

[Thomas Zimmermann]

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

Hi

Am 11.11.22 um 10:42 schrieb Daniel Vetter:
> On Thu, Nov 10, 2022 at 02:55:19PM +0100, Thomas Zimmermann wrote:
>> The fbdev damage worker is unused, so remove it.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> I'd squash this into the previous patch, which gets rid of the
> schedule_work(), if you limit the previous patch to really just that
> change. But split out is fine too.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks for reviewing the patchset. After going through all the comments, 
the patches look much better than before.

Best regards
Thomas

> 
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 9 ---------
>>   include/drm/drm_fb_helper.h     | 2 --
>>   2 files changed, 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 8cb644e4ecf90..47b8ef03a1f89 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -403,13 +403,6 @@ 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
>> @@ -425,7 +418,6 @@ 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;
>> @@ -557,7 +549,6 @@ 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 ecfcd2c56d95a..f37bb2832ba41 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -116,7 +116,6 @@ 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
>> @@ -146,7 +145,6 @@ 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.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 4/5] drm/fb-helper: Schedule deferred-I/O worker after writing to framebuffer

[Daniel Vetter]

On Tue, Nov 15, 2022 at 11:05:02AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.11.22 um 10:28 schrieb Daniel Vetter:
> > On Thu, Nov 10, 2022 at 02:55:18PM +0100, Thomas Zimmermann wrote:
> > > Schedule the deferred-I/O worker instead of the damage worker after
> > > writing to the fbdev framebuffer. The deferred-I/O worker then performs
> > > the dirty-fb update. The fbdev emulation will initialize deferred I/O
> > > for all drivers that require damage updates. It is therefore a valid
> > > assumption that the deferred-I/O worker is present.
> > > 
> > > It would be possible to perform the damage handling directly from within
> > > the write operation. But doing this could increase the overhead of the
> > > write or interfere with a concurrently scheduled deferred-I/O worker.
> > > Instead, scheduling the deferred-I/O worker with its regular delay of
> > > 50 ms removes load off the write operation and allows the deferred-I/O
> > > worker to handle multiple write operations that arrived during the delay
> > > time window.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > ---
> > >   drivers/gpu/drm/drm_fb_helper.c     | 81 ++++++++++++++++++++---------
> > >   drivers/video/fbdev/core/fb_defio.c | 16 ++++++
> > >   include/linux/fb.h                  |  1 +
> > >   3 files changed, 72 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index ebc44ed1bf4a2..8cb644e4ecf90 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -596,14 +596,6 @@ static void drm_fb_helper_add_damage_clip(struct drm_fb_helper *helper, u32 x, u
> > >   	spin_unlock_irqrestore(&helper->damage_lock, flags);
> > >   }
> > > -static void drm_fb_helper_damage(struct drm_fb_helper *helper, u32 x, u32 y,
> > > -				 u32 width, u32 height)
> > > -{
> > > -	drm_fb_helper_add_damage_clip(helper, x, y, width, height);
> > > -
> > > -	schedule_work(&helper->damage_work);
> > 
> > I'm kinda not seeing the point in removing this, and ending up with 2
> > functions calls for every callsite. Replace the schedule_work with the
> > inlined drm_fb_helper_flush instead? That also avoids the naming bikeshed
> > in this case at least :-)
> > 
> > 
> > > -}
> > > -
> > >   /*
> > >    * Convert memory region into area of scanlines and pixels per
> > >    * scanline. The parameters off and len must not reach beyond
> > > @@ -683,6 +675,23 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
> > >   }
> > >   EXPORT_SYMBOL(drm_fb_helper_deferred_io);
> > > +static void drm_fb_helper_flush(struct drm_fb_helper *helper)
> > > +{
> > > +	struct drm_device *dev = helper->dev;
> > > +	struct fb_info *info = helper->info;
> > > +
> > > +	/*
> > > +	 * For now, we assume that deferred I/O has been enabled as damage
> > > +	 * updates require deferred I/O for a working mmap. The current
> > > +	 * fbdev emulation does not flush buffers if no damage update is
> > > +	 * necessary. So it's safe to assume fbdefio to be set.
> > > +	 */
> > > +	if (drm_WARN_ON_ONCE(dev, !info->fbdefio))
> > > +		return;
> > > +
> > > +	fb_deferred_io_flush(info);
> > > +}
> > > +
> > >   typedef ssize_t (*drm_fb_helper_read_screen)(struct fb_info *info, char __user *buf,
> > >   					     size_t count, loff_t pos);
> > > @@ -824,9 +833,10 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
> > >   	if (helper->funcs->fb_dirty) {
> > >   		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
> > > -		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
> > > -				     drm_rect_width(&damage_area),
> > > -				     drm_rect_height(&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_flush(helper);
> > >   	}
> > >   	return ret;
> > > @@ -847,8 +857,11 @@ void drm_fb_helper_sys_fillrect(struct fb_info *info,
> > >   	sys_fillrect(info, rect);
> > > -	if (helper->funcs->fb_dirty)
> > > -		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
> > > +	if (helper->funcs->fb_dirty) {
> > > +		drm_fb_helper_add_damage_clip(helper, rect->dx, rect->dy,
> > > +					      rect->width, rect->height);
> > > +		drm_fb_helper_flush(helper);
> > > +	}
> > >   }
> > >   EXPORT_SYMBOL(drm_fb_helper_sys_fillrect);
> > > @@ -866,8 +879,11 @@ void drm_fb_helper_sys_copyarea(struct fb_info *info,
> > >   	sys_copyarea(info, area);
> > > -	if (helper->funcs->fb_dirty)
> > > -		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
> > > +	if (helper->funcs->fb_dirty) {
> > > +		drm_fb_helper_add_damage_clip(helper, area->dx, area->dy,
> > > +					      area->width, area->height);
> > > +		drm_fb_helper_flush(helper);
> > > +	}
> > >   }
> > >   EXPORT_SYMBOL(drm_fb_helper_sys_copyarea);
> > > @@ -885,8 +901,11 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
> > >   	sys_imageblit(info, image);
> > > -	if (helper->funcs->fb_dirty)
> > > -		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
> > > +	if (helper->funcs->fb_dirty) {
> > > +		drm_fb_helper_add_damage_clip(helper, image->dx, image->dy,
> > > +					      image->width, image->height);
> > > +		drm_fb_helper_flush(helper);
> > > +	}
> > >   }
> > >   EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
> > > @@ -997,9 +1016,10 @@ ssize_t drm_fb_helper_cfb_write(struct fb_info *info, const char __user *buf,
> > >   	if (helper->funcs->fb_dirty) {
> > >   		drm_fb_helper_memory_range_to_clip(info, pos, ret, &damage_area);
> > > -		drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
> > > -				     drm_rect_width(&damage_area),
> > > -				     drm_rect_height(&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_flush(helper);
> > >   	}
> > >   	return ret;
> > > @@ -1020,8 +1040,11 @@ void drm_fb_helper_cfb_fillrect(struct fb_info *info,
> > >   	cfb_fillrect(info, rect);
> > > -	if (helper->funcs->fb_dirty)
> > > -		drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width, rect->height);
> > > +	if (helper->funcs->fb_dirty) {
> > > +		drm_fb_helper_add_damage_clip(helper, rect->dx, rect->dy,
> > > +					      rect->width, rect->height);
> > > +		drm_fb_helper_flush(helper);
> > > +	}
> > >   }
> > >   EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
> > > @@ -1039,8 +1062,11 @@ void drm_fb_helper_cfb_copyarea(struct fb_info *info,
> > >   	cfb_copyarea(info, area);
> > > -	if (helper->funcs->fb_dirty)
> > > -		drm_fb_helper_damage(helper, area->dx, area->dy, area->width, area->height);
> > > +	if (helper->funcs->fb_dirty) {
> > > +		drm_fb_helper_add_damage_clip(helper, area->dx, area->dy,
> > > +					      area->width, area->height);
> > > +		drm_fb_helper_flush(helper);
> > > +	}
> > >   }
> > >   EXPORT_SYMBOL(drm_fb_helper_cfb_copyarea);
> > > @@ -1058,8 +1084,11 @@ void drm_fb_helper_cfb_imageblit(struct fb_info *info,
> > >   	cfb_imageblit(info, image);
> > > -	if (helper->funcs->fb_dirty)
> > > -		drm_fb_helper_damage(helper, image->dx, image->dy, image->width, image->height);
> > > +	if (helper->funcs->fb_dirty) {
> > > +		drm_fb_helper_add_damage_clip(helper, image->dx, image->dy,
> > > +					      image->width, image->height);
> > > +		drm_fb_helper_flush(helper);
> > > +	}
> > >   }
> > >   EXPORT_SYMBOL(drm_fb_helper_cfb_imageblit);
> > > diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> > > index c730253ab85ce..325d12c3a4d61 100644
> > > --- a/drivers/video/fbdev/core/fb_defio.c
> > > +++ b/drivers/video/fbdev/core/fb_defio.c
> > > @@ -332,3 +332,19 @@ void fb_deferred_io_cleanup(struct fb_info *info)
> > >   	mutex_destroy(&fbdefio->lock);
> > >   }
> > >   EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
> > > +
> > > +void fb_deferred_io_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 to perform the flush immediately. So
> > > +	 * schedule the worker with a delay and let a few more writes
> > > +	 * pile up.
> > > +	 */
> > 
> > So this part is wrong, because the drm callers do rely on this not
> > flushing anything immediately, but instead on scheduling the worker. Or at
> > least that's the reason why we have the damage worker in the first place.
> 
> That is badly worded. The comment means that the worker does not have to be
> scheduled immediately. I'll change that.
> 
> > 
> > So this comment here needs to go, and the functions need to make it clear
> > in their names that that they queue/schedule the flush.
> 
> fb_deferred_io_schedule_flush() seems like the most expressive name.
> 
> The current name came from glFlush() and glFinish(), where the former only
> schedules a flushand the letter really waits before returning.

Just an aside, but this actually meant the same thing 30 years ago, when
GL was done on the cpu. glFlush actually flushed out and finished all the
rendering, because gl was an immediate mode api back then.

But now we have bazillions of layers in between to keep up the old gl
illusions while actually trying to use the power of modern gpus, and so
you have stuff like dma-buf implicit sync to be able to keep up the
illusion that everything is synchronous, while in reality it is absolutely
not.

vk has this all much better, where the equivalent of glflush is a queue
command and you get a dma_fence (in some wrapper, usually syncobj) back
explicitly.

History aside, I like your choice of naming.
-Daniel

> 
> Best regards
> Thomas
> 
> > 
> > Also not sure whether you want to split out the fbdev part or not, imo
> > overkill.
> > 
> > With comments addressed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > 
> > > +	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
> > > +}
> > > +EXPORT_SYMBOL_GPL(fb_deferred_io_flush);
> > > diff --git a/include/linux/fb.h b/include/linux/fb.h
> > > index bcb8658f5b64d..54b3b3e13522f 100644
> > > --- a/include/linux/fb.h
> > > +++ b/include/linux/fb.h
> > > @@ -663,6 +663,7 @@ 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_flush(struct fb_info *info);
> > >   extern int fb_deferred_io_fsync(struct file *file, loff_t start,
> > >   				loff_t end, int datasync);
> > > -- 
> > > 2.38.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




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