[Intel-gfx] [PATCH v3 1/2] drm/i915: Tear down fbdev if initialization fails

[Lukas Wunner]

Hi Chris,

On Thu, Nov 19, 2015 at 09:46:34PM +0000, Chris Wilson wrote:
> On Thu, Nov 19, 2015 at 04:58:44PM +0100, Daniel Vetter wrote:
> > On Wed, Nov 18, 2015 at 04:29:51PM +0100, Lukas Wunner wrote:
> > > @@ -727,7 +730,8 @@ void intel_fbdev_fini(struct drm_device *dev)
> > >  
> > >  	flush_work(&dev_priv->fbdev_suspend_work);
> > >  
> > > -	async_synchronize_full();
> > > +	if (!current_is_async())
> > > +		async_synchronize_full();
> > 
> > I think this is a bit too fragile, and the core depency will make merging
> > tricky. Can't we just push the async_synchronize_full into module unload
> > for now?
> 
> (intel_fbdev_fini() is already module unload, right?

With my patch it'd be called from a 2nd site: intel_fbdev_initial_config().
To tear down the fbdev if initialization failed.

However since that function is run asynchronously, async_synchronize_full()
deadlocks as it waits forever for itself to finish.


> Do you mean just
> move the async handling into i915_driver_unload() so that we have a
> single spot for all future potential users of the async framework?)

That precisely was the motivation for Ville's cleanup e00bf69644ba
a few days ago, to consolidate things in one place. However he chose
to move everything into intel_fbdev.c. That leaves three options to
avoid the deadlock:

(a) call async_synchronize_full() conditionally if (!current_is_async()),
    that's what I did. That way it only gets called when the function
    is entered from i915_driver_unload(), not when it's entered from
    intel_fbdev_initial_config().

(b) split intel_fbdev_fini() in two, first part is called only called
    on module unload.

(c) revert Ville's patch, consolidate the async stuff in i915_dma.c.


> And optimising module unload to avoid one potential grace period when we
> already have a bunch of grace period waits seems overkill.
> 
> The alternative to using async_synchronize_full() would be to use an
> async-domain.

But an async domain wouldn't solve the deadlock, would it?


Best regards,

Lukas