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

[Daniel Vetter <daniel@ffwll.ch>]

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