* [PATCH] drm/i915: Only unlink ggtt->global_link after i915_gem_load @ 2013-12-16 12:01 Chris Wilson 2013-12-16 18:58 ` Chris Wilson 0 siblings, 1 reply; 4+ messages in thread From: Chris Wilson @ 2013-12-16 12:01 UTC (permalink / raw) To: intel-gfx If i915_driver_load fails before i915_gem_load we try to unlink the ggtt prior to initialising the list and triggering an oops. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_dma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index df1410ecbb8b..6364c503f97d 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1683,11 +1683,11 @@ out_gem_unload: intel_teardown_gmbus(dev); intel_teardown_mchbar(dev); destroy_workqueue(dev_priv->wq); + list_del(&dev_priv->gtt.base.global_link); out_mtrrfree: arch_phys_wc_del(dev_priv->gtt.mtrr); io_mapping_free(dev_priv->gtt.mappable); out_gtt: - list_del(&dev_priv->gtt.base.global_link); drm_mm_takedown(&dev_priv->gtt.base.mm); dev_priv->gtt.base.cleanup(&dev_priv->gtt.base); out_regs: -- 1.8.5.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Only unlink ggtt->global_link after i915_gem_load 2013-12-16 12:01 [PATCH] drm/i915: Only unlink ggtt->global_link after i915_gem_load Chris Wilson @ 2013-12-16 18:58 ` Chris Wilson 2014-01-10 20:22 ` Ben Widawsky 0 siblings, 1 reply; 4+ messages in thread From: Chris Wilson @ 2013-12-16 18:58 UTC (permalink / raw) To: intel-gfx On Mon, Dec 16, 2013 at 12:01:07PM +0000, Chris Wilson wrote: > If i915_driver_load fails before i915_gem_load we try to unlink the ggtt > prior to initialising the list and triggering an oops. This is incomplete. > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_dma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index df1410ecbb8b..6364c503f97d 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1683,11 +1683,11 @@ out_gem_unload: > intel_teardown_gmbus(dev); > intel_teardown_mchbar(dev); > destroy_workqueue(dev_priv->wq); > + list_del(&dev_priv->gtt.base.global_link); + dev_priv->gtt.base.cleanup(&dev_priv->gtt.base); > out_mtrrfree: > arch_phys_wc_del(dev_priv->gtt.mtrr); > io_mapping_free(dev_priv->gtt.mappable); > out_gtt: > - list_del(&dev_priv->gtt.base.global_link); - drm_mm_takedown(&dev_priv->gtt.base.mm); - dev_priv->gtt.base.cleanup(&dev_priv->gtt.base); -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Only unlink ggtt->global_link after i915_gem_load 2013-12-16 18:58 ` Chris Wilson @ 2014-01-10 20:22 ` Ben Widawsky 2014-01-10 20:28 ` Chris Wilson 0 siblings, 1 reply; 4+ messages in thread From: Ben Widawsky @ 2014-01-10 20:22 UTC (permalink / raw) To: Chris Wilson, intel-gfx On Mon, Dec 16, 2013 at 06:58:20PM +0000, Chris Wilson wrote: > On Mon, Dec 16, 2013 at 12:01:07PM +0000, Chris Wilson wrote: > > If i915_driver_load fails before i915_gem_load we try to unlink the ggtt > > prior to initialising the list and triggering an oops. > > This is incomplete. > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_dma.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > index df1410ecbb8b..6364c503f97d 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -1683,11 +1683,11 @@ out_gem_unload: > > intel_teardown_gmbus(dev); > > intel_teardown_mchbar(dev); > > destroy_workqueue(dev_priv->wq); > > + list_del(&dev_priv->gtt.base.global_link); This is correct. > + dev_priv->gtt.base.cleanup(&dev_priv->gtt.base); I'm not convinced here. It should be safe [ideal] to call cleanup after we've setup the function pointer, which would be out_gtt; it's done in i915_gem_gtt_init(). If it's not safe to call for some reason, we should fix cleanup() > > out_mtrrfree: > > arch_phys_wc_del(dev_priv->gtt.mtrr); > > io_mapping_free(dev_priv->gtt.mappable); > > out_gtt: > > - list_del(&dev_priv->gtt.base.global_link); > - drm_mm_takedown(&dev_priv->gtt.base.mm); This is correct. > - dev_priv->gtt.base.cleanup(&dev_priv->gtt.base); > > -- > Chris Wilson, Intel Open Source Technology Centre -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Only unlink ggtt->global_link after i915_gem_load 2014-01-10 20:22 ` Ben Widawsky @ 2014-01-10 20:28 ` Chris Wilson 0 siblings, 0 replies; 4+ messages in thread From: Chris Wilson @ 2014-01-10 20:28 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On Fri, Jan 10, 2014 at 12:22:44PM -0800, Ben Widawsky wrote: > On Mon, Dec 16, 2013 at 06:58:20PM +0000, Chris Wilson wrote: > > On Mon, Dec 16, 2013 at 12:01:07PM +0000, Chris Wilson wrote: > > > If i915_driver_load fails before i915_gem_load we try to unlink the ggtt > > > prior to initialising the list and triggering an oops. > > > > This is incomplete. > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > drivers/gpu/drm/i915/i915_dma.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > index df1410ecbb8b..6364c503f97d 100644 > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > @@ -1683,11 +1683,11 @@ out_gem_unload: > > > intel_teardown_gmbus(dev); > > > intel_teardown_mchbar(dev); > > > destroy_workqueue(dev_priv->wq); > > > + list_del(&dev_priv->gtt.base.global_link); > > This is correct. > > > + dev_priv->gtt.base.cleanup(&dev_priv->gtt.base); > > I'm not convinced here. It should be safe [ideal] to call cleanup after > we've setup the function pointer, which would be out_gtt; it's done in > i915_gem_gtt_init(). If it's not safe to call for some reason, we should > fix cleanup() It blew up, hence the amendment, iirc. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-01-10 20:28 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-16 12:01 [PATCH] drm/i915: Only unlink ggtt->global_link after i915_gem_load Chris Wilson 2013-12-16 18:58 ` Chris Wilson 2014-01-10 20:22 ` Ben Widawsky 2014-01-10 20:28 ` Chris Wilson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox