[PATCH v3 4/4] drm/fb-helper: Synchronize dirty worker with vblank

[Emil Velikov]

Hi Thomas,

On Thu, 5 Dec 2019 at 16:01, Thomas Zimmermann <tzimmermann at suse.de> wrote:
>
> Before updating the display from the console's shadow buffer, the dirty
> worker now waits for a vblank. This allows several screen updates to pile
> up and acts as a rate limiter. If a DRM master is present, it could
> interfere with the vblank. Don't wait in this case.
>
> v3:
>         * add back helper->lock
>         * acquire DRM master status while waiting for vblank
> 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 | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index fb9bff0f4581..ba20ad92fb90 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -404,8 +404,29 @@ 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 to vblank. If there's a DRM master
> +        * present, it could interfere while we're waiting for the vblank
> +        * event. Don't wait in this case.
> +        */
> +       mutex_lock(&helper->lock);
> +       if (!drm_master_internal_acquire(helper->dev)) {
> +               goto unlock;
> +       }
> +       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);
> +       }
> +       drm_master_internal_release(helper->dev);
> +unlock:
> +       mutex_unlock(&helper->lock);
>

This hunk seems surprisingly similar to the FBIO_WAITFORVSYNC code in
drm_fb_helper_ioctl().
Modulo the really neat comment, which answers the "why do we use
modeset[0]" instead of any other (or all).

Can I suggest fleshing that function out (alongside all the locking)
into a helper and reusing it here.
Pretty sure we can live with the (very unlikely) EBUSY -> ENOTTY
change, in drm_fb_helper_ioctl()

HTH
Emil