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

[Lukas Wunner <lukas@wunner.de>]

Hi again,

On Thu, Nov 19, 2015 at 05:02:04PM +0100, Daniel Vetter wrote:
> On Wed, Nov 18, 2015 at 01:43:20PM +0100, Lukas Wunner wrote:
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -408,7 +408,10 @@ static void intel_connector_add_to_fbdev(struct intel_connector *connector)
> >  {
> >  #ifdef CONFIG_DRM_FBDEV_EMULATION
> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > -	drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper, &connector->base);
> > +
> > +	if (dev_priv->fbdev)
> > +		drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper,
> > +						&connector->base);
> >  #endif
> >  }
> >  
> > @@ -416,7 +419,10 @@ static void intel_connector_remove_from_fbdev(struct intel_connector *connector)
> >  {
> >  #ifdef CONFIG_DRM_FBDEV_EMULATION
> >  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > -	drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper, &connector->base);
> > +
> > +	if (dev_priv->fbdev)
> > +		drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper,
> > +						   &connector->base);
> >  #endif
> >  }
> >  
> Queued for -next, thanks for the patch. Aside, with this patch and the
> static inline dummies from Archit I think we can drop most of the #ifdef
> blocks (not the one in debugfs though). Care for a follow-up patch to
> remove them around add/remove_one_connector?

Looking at it once more I've realized that the fbdev member in struct
drm_i915_private is enclosed in an #ifdef CONFIG_DRM_FBDEV_EMULATION,
so we can't remove the #ifdefs here: The compiler would complain about
non-existence of the dev_priv->fbdev member.


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?

Thinking about this a bit longer I believe that if anything the change
should make it more robust rather than fragile. After all we eliminate
the source of a deadlock that could occur here (async_synchronize_full()
waiting forever for itself to finish).

That said I'm not married to this solution. If you do find it concerning
I could change it according to Ville's suggestion, i.e. splitting
intel_fbdev_fini() in two parts.

Moving the async_synchronize_full() to i915_driver_unload() would be
contrary to the clarity Ville had sought by consolidating everything
in intel_fbdev.c.

Thanks & best regards,

Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx