diff mbox

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

Message ID 49ce5f0daead24b7598ec78591731046c333c18d.1447938059.git.lukas@wunner.de (mailing list archive)
State New, archived
Headers show

Commit Message

Lukas Wunner Nov. 18, 2015, 3:29 p.m. UTC 
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(-)

Comments

Daniel Vetter Nov. 19, 2015, 3:58 p.m. UTC | #1 
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?
-Daniel

>  	intel_fbdev_destroy(dev, dev_priv->fbdev);
>  	kfree(dev_priv->fbdev);
>  	dev_priv->fbdev = NULL;
> -- 
> 2.1.0
>
Lukas Wunner Nov. 19, 2015, 4:08 p.m. UTC | #2 
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
Chris Wilson Nov. 19, 2015, 9:46 p.m. UTC | #3 
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? 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?)

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.
-Chris
Lukas Wunner Nov. 19, 2015, 10:25 p.m. UTC | #4 
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
diff mbox

Patch

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();
 	intel_fbdev_destroy(dev, dev_priv->fbdev);
 	kfree(dev_priv->fbdev);
 	dev_priv->fbdev = NULL;