| Message ID | 1443085320-4579-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 24, 2015 at 10:02:00AM +0100, Chris Wilson wrote: > When preallocating a stolen object during early initialisation, we may > be running before we have setup the the global GTT VM state, in > particular before we have initialised the range manager and associated > lists. As this is the case, we defer binding the stolen object until we > call i915_gem_setup_global_gtt(). Not only should we defer the binding, > but we should also defer the VM list manipulation. > > Fixes regression from commit a2cad9dff4dd44d0244b966d980de9d602d87593 > Author: Micha? Winiarski <michal.winiarski@intel.com> > Date: Wed Sep 16 11:49:00 2015 +0200 > > drm/i915/gtt: Do not initialize drm_mm twice. For the record, this is not the root cause of the bug just the trigger fr the current breakage. s/from/uncovered by/ ? > Whilst I am here remove the duplicate work leaving dangling pointers > from the error path... > > Reported-by: Jesse Barnes <jesse@virtuousgeek.org> Reported-by: Jesse Barnes <jbarnes@virtuousgeek.org> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jesse Barnes <jesse@virtuousgeek.org> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Micha? Winiarski <michal.winiarski@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> -Chris
On Thu, 24 Sep 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote: > When preallocating a stolen object during early initialisation, we may > be running before we have setup the the global GTT VM state, in > particular before we have initialised the range manager and associated > lists. As this is the case, we defer binding the stolen object until we > call i915_gem_setup_global_gtt(). Not only should we defer the binding, > but we should also defer the VM list manipulation. > > Fixes regression from commit a2cad9dff4dd44d0244b966d980de9d602d87593 > Author: Micha? Winiarski <michal.winiarski@intel.com> > Date: Wed Sep 16 11:49:00 2015 +0200 > > drm/i915/gtt: Do not initialize drm_mm twice. > > Whilst I am here remove the duplicate work leaving dangling pointers > from the error path... > > Reported-by: Jesse Barnes <jesse@virtuousgeek.org> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jesse Barnes <jesse@virtuousgeek.org> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Micha? Winiarski <michal.winiarski@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> CC [M] drivers/gpu/drm/i915/i915_gem_stolen.o drivers/gpu/drm/i915/i915_gem_gtt.c: In function ‘i915_gem_setup_global_gtt’: drivers/gpu/drm/i915/i915_gem_gtt.c:2659:33: error: ‘ggtt’ undeclared (first use in this function) list_add_tail(&vma->mm_list, &ggtt->inactive_list); ^ Jani. > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- > drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++---------- > 2 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 01f3521e77d3..9cc68624410e 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -2533,7 +2533,6 @@ static int ggtt_bind_vma(struct i915_vma *vma, > * the bound flag ourselves. > */ > vma->bound |= GLOBAL_BIND; > - > } > > if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) { > @@ -2657,6 +2656,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev, > return ret; > } > vma->bound |= GLOBAL_BIND; > + list_add_tail(&vma->mm_list, &ggtt->inactive_list); > } > > /* Clear any non-preallocated blocks */ > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 55df6ce34751..15207796e1b3 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -584,7 +584,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, > vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt); > if (IS_ERR(vma)) { > ret = PTR_ERR(vma); > - goto err_out; > + goto err; > } > > /* To simplify the initialisation sequence between KMS and GTT, > @@ -598,23 +598,19 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, > ret = drm_mm_reserve_node(&ggtt->mm, &vma->node); > if (ret) { > DRM_DEBUG_KMS("failed to allocate stolen GTT space\n"); > - goto err_vma; > + goto err; > } > - } > > - vma->bound |= GLOBAL_BIND; > + vma->bound |= GLOBAL_BIND; > + list_add_tail(&vma->mm_list, &ggtt->inactive_list); > + } > > list_add_tail(&obj->global_list, &dev_priv->mm.bound_list); > - list_add_tail(&vma->mm_list, &ggtt->inactive_list); > i915_gem_object_pin_pages(obj); > > return obj; > > -err_vma: > - i915_gem_vma_destroy(vma); > -err_out: > - i915_gem_stolen_remove_node(dev_priv, stolen); > - kfree(stolen); > +err: > drm_gem_object_unreference(&obj->base); > return NULL; > } > -- > 2.5.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 01f3521e77d3..9cc68624410e 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2533,7 +2533,6 @@ static int ggtt_bind_vma(struct i915_vma *vma, * the bound flag ourselves. */ vma->bound |= GLOBAL_BIND; - } if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) { @@ -2657,6 +2656,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev, return ret; } vma->bound |= GLOBAL_BIND; + list_add_tail(&vma->mm_list, &ggtt->inactive_list); } /* Clear any non-preallocated blocks */ diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 55df6ce34751..15207796e1b3 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -584,7 +584,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt); if (IS_ERR(vma)) { ret = PTR_ERR(vma); - goto err_out; + goto err; } /* To simplify the initialisation sequence between KMS and GTT, @@ -598,23 +598,19 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, ret = drm_mm_reserve_node(&ggtt->mm, &vma->node); if (ret) { DRM_DEBUG_KMS("failed to allocate stolen GTT space\n"); - goto err_vma; + goto err; } - } - vma->bound |= GLOBAL_BIND; + vma->bound |= GLOBAL_BIND; + list_add_tail(&vma->mm_list, &ggtt->inactive_list); + } list_add_tail(&obj->global_list, &dev_priv->mm.bound_list); - list_add_tail(&vma->mm_list, &ggtt->inactive_list); i915_gem_object_pin_pages(obj); return obj; -err_vma: - i915_gem_vma_destroy(vma); -err_out: - i915_gem_stolen_remove_node(dev_priv, stolen); - kfree(stolen); +err: drm_gem_object_unreference(&obj->base); return NULL; }
When preallocating a stolen object during early initialisation, we may be running before we have setup the the global GTT VM state, in particular before we have initialised the range manager and associated lists. As this is the case, we defer binding the stolen object until we call i915_gem_setup_global_gtt(). Not only should we defer the binding, but we should also defer the VM list manipulation. Fixes regression from commit a2cad9dff4dd44d0244b966d980de9d602d87593 Author: Micha? Winiarski <michal.winiarski@intel.com> Date: Wed Sep 16 11:49:00 2015 +0200 drm/i915/gtt: Do not initialize drm_mm twice. Whilst I am here remove the duplicate work leaving dangling pointers from the error path... Reported-by: Jesse Barnes <jesse@virtuousgeek.org> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jesse Barnes <jesse@virtuousgeek.org> Cc: Michel Thierry <michel.thierry@intel.com> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Micha? Winiarski <michal.winiarski@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++---------- 2 files changed, 7 insertions(+), 11 deletions(-)