[PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank

[Daniel Vetter]

On Thu, Sep 12, 2019 at 08:42:28AM +0200, Thomas Zimmermann wrote:
> Before updating the display from the console's shadow buffer, the dirty
> worker now waits for vblank. This allows several screen updates to pile
> up and acts as a rate limiter.
> 
> v2:
> 	* don't hold helper->lock while waiting for vblank
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a7ba5b4902d6..d0cb1fa4f909 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>  						    dirty_work);
>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>  	struct drm_clip_rect clip_copy;
> +	struct drm_crtc *crtc;
>  	unsigned long flags;
>  	void *vaddr;
> +	int ret;
> +
> +	/* rate-limit update frequency */
> +	crtc = helper->client.modesets[0].crtc;
> +	ret = drm_crtc_vblank_get(crtc);
> +	if (!ret) {
> +		drm_crtc_wait_one_vblank(crtc);

Without the locking (specifically, preventing other masters) this can go
boom since it again calls drm_vblank_get. If someone turned of the display
meanwhile that will fail, and result in an unsightly WARN backtrace.

I think we need a __drm_crtc_wait_one_vblank(crtc); which requires that
callers hold a full vblank reference already. The other issue is that we
might race with the disabling and hit the timeout, which again gives us an
unsightly WARNING backtrace. Both can happen without locks (that's why the
ioctl path needs them), but we need to avoid.
-Daniel
> +		drm_crtc_vblank_put(crtc);
> +	}
>  
>  	spin_lock_irqsave(&helper->dirty_lock, flags);
>  	clip_copy = *clip;
> -- 
> 2.23.0
> 

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