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

[Daniel Vetter]

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 at ffwll.ch>
> Signed-off-by: Lukas Wunner <lukas at 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?
-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