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

Submitted by Thomas Zimmermann on Sept. 12, 2019, 6:42 a.m.

Details

Message ID 20190912064230.27972-2-tzimmermann@suse.de
State New
Headers show
Series "Rate-limit shadow-FB-to-console-update to screen refresh" ( rev: 2 ) in DRI devel

Not browsing as part of any series.

Commit Message

Thomas Zimmermann Sept. 12, 2019, 6:42 a.m.
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@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Patch hide | download patch | download mbox

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);
+		drm_crtc_vblank_put(crtc);
+	}
 
 	spin_lock_irqsave(&helper->dirty_lock, flags);
 	clip_copy = *clip;

Comments

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@suse.de>#

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

> ---
>  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);
> +		drm_crtc_vblank_put(crtc);
> +	}
>  
>  	spin_lock_irqsave(&helper->dirty_lock, flags);
>  	clip_copy = *clip;
> -- 
> 2.23.0
>
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@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
>