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

[Lukas Wunner <lukas@wunner.de>]

Hi,

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:
> > Currently if intelfb_create() errors out, it unrefs the bo even though
> > the fb now owns that reference. (Spotted by Ville Syrjälä.) We should
> > unref the fb instead of the bo.
> > 
> > However the fb was not necessarily allocated by intelfb_create(),
> > it could be inherited from BIOS (the fb struct was then allocated by
> > dev_priv->display.get_initial_plane_config()) and be in active use by
> > a crtc. In this case we should call drm_framebuffer_remove() instead
> > of _unreference() to also disable the crtc.
> > 
> > Daniel Vetter suggested that "fbdev teardown code will take care of it.
> > The correct approach is probably to not unref anything at all".
> > 
> > But if fbdev initialization fails, the fbdev isn't torn down and
> > occupies memory even though it's unusable. Therefore clobber it in
> > intel_fbdev_initial_config(). (Currently we ignore a negative return
> > value there.) The idea is that if fbdev initialization fails, the driver
> > behaves as if CONFIG_DRM_FBDEV_EMULATION wasn't set. Should X11 manage
> > to start up without errors, it will at least be able to use the memory
> > that would otherwise be hogged by the unusable fbdev.
> > 
> > Also, log errors in intelfb_create().
> > 
> > Don't call async_synchronize_full() in intel_fbdev_fini() when called
> > from intel_fbdev_initial_config() to avoid deadlock.
> > 
> > v2: Instead of calling drm_framebuffer_unreference() (if fb was not
> >     inherited from BIOS), call intel_fbdev_fini().
> > 
> > v3: Rebase on e00bf69644ba (drm/i915: Move the fbdev async_schedule()
> >     into intel_fbdev.c), call async_synchronize_full() conditionally
> >     instead of moving it into i915_driver_unload().
> > 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 98772d3..cd345c5 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -227,6 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  
> >  	info = drm_fb_helper_alloc_fbi(helper);
> >  	if (IS_ERR(info)) {
> > +		DRM_ERROR("Failed to allocate fb_info\n");
> >  		ret = PTR_ERR(info);
> >  		goto out_unpin;
> >  	}
> > @@ -253,6 +254,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
> >  			   size);
> >  	if (!info->screen_base) {
> > +		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> >  		ret = -ENOSPC;
> >  		goto out_destroy_fbi;
> >  	}
> > @@ -285,7 +287,6 @@ out_destroy_fbi:
> >  	drm_fb_helper_release_fbi(helper);
> >  out_unpin:
> >  	i915_gem_object_ggtt_unpin(obj);
> > -	drm_gem_object_unreference(&obj->base);
> >  	mutex_unlock(&dev->struct_mutex);
> >  	return ret;
> >  }
> > @@ -711,7 +712,9 @@ static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
> >  	struct intel_fbdev *ifbdev = dev_priv->fbdev;
> >  
> >  	/* Due to peculiar init order wrt to hpd handling this is separate. */
> > -	drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
> > +	if (drm_fb_helper_initial_config(&ifbdev->helper,
> > +					 ifbdev->preferred_bpp))
> > +		intel_fbdev_fini(dev_priv->dev);
> >  }
> >  
> >  void intel_fbdev_initial_config_async(struct drm_device *dev)
> > @@ -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?

That was my original suggestion but Ville didn't like it... :-)

Message-ID: <20151109110050.GW4437@intel.com>
Link: http://lists.freedesktop.org/archives/intel-gfx/2015-November/079728.html

As for merging being tricky, if Tejun only acks the other patch maybe it
can be merged through drm-intel-next-queued (barring any objections
against the patch itself of course).

Best regards,

Lukas

> -Daniel
> 
> >  	intel_fbdev_destroy(dev, dev_priv->fbdev);
> >  	kfree(dev_priv->fbdev);
> >  	dev_priv->fbdev = NULL;
> > -- 
> > 2.1.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx