[PATCH 7/9] drm/omap: fix omap_crtc_flush() to handle the workqueue

[Daniel Vetter]

On Wed, Sep 03, 2014 at 02:55:08PM +0300, Tomi Valkeinen wrote:
> omap_crtc_flush() is used to wait for scheduled work to be done for the
> give crtc. However, it's not quite right at the moment.
> 
> omap_crtc_flush() does wait for work that is ran via vsync irq to be
> done. However, work is also queued to the driver's priv->wq workqueue,
> which is not handled by omap_crtc_flush().
> 
> Improve omap_crtc_flush() to flush the workqueue so that work there will
> be ran.
> 
> This fixes a race issue on module unload, where an unpin work may be on
> the work queue, but does not get ran before drm core starts tearing the
> driver down, leading to a WARN.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ti.com>

I didn't really dig into details, but isn't that the same workqueue as
used by the async modeset code? So the same deadlocks might happen ...

lockdep won't complain though since you essentially open-code a
workqueue_flush, and lockdep also doesn't complain about all possible
deadlocks (due to some design issues with lockdep).
-Daniel

> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 3261fbf94957..506cf8bc804f 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -655,21 +655,42 @@ static void omap_crtc_post_apply(struct omap_drm_apply *apply)
>  	/* nothing needed for post-apply */
>  }
>  
> +static bool omap_crtc_work_pending(struct omap_crtc *omap_crtc)
> +{
> +	return !list_empty(&omap_crtc->pending_applies) ||
> +		!list_empty(&omap_crtc->queued_applies) ||
> +		omap_crtc->event || omap_crtc->old_fb;
> +}
> +
> +/*
> + * Wait for any work on the workqueue to be finished, and any work which will
> + * be run via vsync irq to be done.
> + *
> + * Note that work on workqueue could schedule new vsync work, and vice versa.
> + */
>  void omap_crtc_flush(struct drm_crtc *crtc)
>  {
>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> +	struct omap_drm_private *priv = crtc->dev->dev_private;
>  	int loops = 0;
>  
> -	while (!list_empty(&omap_crtc->pending_applies) ||
> -		!list_empty(&omap_crtc->queued_applies) ||
> -		omap_crtc->event || omap_crtc->old_fb) {
> +	while (true) {
> +		/* first flush the wq, so that any scheduled work is done */
> +		flush_workqueue(priv->wq);
> +
> +		/* if we have nothing queued for this crtc, we're done */
> +		if (!omap_crtc_work_pending(omap_crtc))
> +			break;
>  
>  		if (++loops > 10) {
> -			dev_err(crtc->dev->dev,
> -				"omap_crtc_flush() timeout\n");
> +			dev_err(crtc->dev->dev, "omap_crtc_flush() timeout\n");
>  			break;
>  		}
>  
> +		/*
> +		 * wait for a bit so that a vsync has (probably) happened, and
> +		 * that the crtc work is (probably) done.
> +		 */
>  		schedule_timeout_uninterruptible(msecs_to_jiffies(20));
>  	}
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch