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@xxxxxxx>
> ---
> 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@xxxxxxxx>
> - /*
> - * 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
