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@xxxxxxx>
> ---
> 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
