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);
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@xxxxxxxx>
> + 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
