| Message ID | 1449845949-4161-1-git-send-email-nicholas.hoath@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 11, 2015 at 02:59:09PM +0000, Nick Hoath wrote: > Swap the order of context & engine cleanup, so that it is now > contexts, then engines. > This allows the context clean up code to do things like confirm > that ring->dev->struct_mutex is locked without a NULL pointer > dereference. > This came about as a result of the 'intel_ring_initialized() must > be simple and inline' patch now using ring->dev as an initialised > flag. > Rename the cleanup function to reflect what it actually does. > Also clean up some very annoying whitespace issues at the same time. > > v2: Also make the fix in i915_load_modeset_init, not just > in i915_driver_unload (Chris Wilson) > > Signed-off-by: Nick Hoath <nicholas.hoath@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: David Gordon <david.s.gordon@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> Queued for -next, thanks for the patch. -Daniel > --- > drivers/gpu/drm/i915/i915_dma.c | 4 ++-- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++----------- > 3 files changed, 15 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 84e2b20..4dad121 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -449,8 +449,8 @@ static int i915_load_modeset_init(struct drm_device *dev) > > cleanup_gem: > mutex_lock(&dev->struct_mutex); > - i915_gem_cleanup_ringbuffer(dev); > i915_gem_context_fini(dev); > + i915_gem_cleanup_engines(dev); > mutex_unlock(&dev->struct_mutex); > cleanup_irq: > intel_guc_ucode_fini(dev); > @@ -1188,8 +1188,8 @@ int i915_driver_unload(struct drm_device *dev) > > intel_guc_ucode_fini(dev); > mutex_lock(&dev->struct_mutex); > - i915_gem_cleanup_ringbuffer(dev); > i915_gem_context_fini(dev); > + i915_gem_cleanup_engines(dev); > mutex_unlock(&dev->struct_mutex); > intel_fbc_cleanup_cfb(dev_priv); > i915_gem_cleanup_stolen(dev); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 5edd393..e317f88 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3016,7 +3016,7 @@ int i915_gem_init_rings(struct drm_device *dev); > int __must_check i915_gem_init_hw(struct drm_device *dev); > int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice); > void i915_gem_init_swizzling(struct drm_device *dev); > -void i915_gem_cleanup_ringbuffer(struct drm_device *dev); > +void i915_gem_cleanup_engines(struct drm_device *dev); > int __must_check i915_gpu_idle(struct drm_device *dev); > int __must_check i915_gem_suspend(struct drm_device *dev); > void __i915_add_request(struct drm_i915_gem_request *req, > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 8e2acde..04a22db 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4823,7 +4823,7 @@ i915_gem_init_hw(struct drm_device *dev) > > ret = i915_gem_request_alloc(ring, ring->default_context, &req); > if (ret) { > - i915_gem_cleanup_ringbuffer(dev); > + i915_gem_cleanup_engines(dev); > goto out; > } > > @@ -4836,7 +4836,7 @@ i915_gem_init_hw(struct drm_device *dev) > if (ret && ret != -EIO) { > DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret); > i915_gem_request_cancel(req); > - i915_gem_cleanup_ringbuffer(dev); > + i915_gem_cleanup_engines(dev); > goto out; > } > > @@ -4844,7 +4844,7 @@ i915_gem_init_hw(struct drm_device *dev) > if (ret && ret != -EIO) { > DRM_ERROR("Context enable ring #%d failed %d\n", i, ret); > i915_gem_request_cancel(req); > - i915_gem_cleanup_ringbuffer(dev); > + i915_gem_cleanup_engines(dev); > goto out; > } > > @@ -4919,7 +4919,7 @@ out_unlock: > } > > void > -i915_gem_cleanup_ringbuffer(struct drm_device *dev) > +i915_gem_cleanup_engines(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_engine_cs *ring; > @@ -4928,13 +4928,14 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev) > for_each_ring(ring, dev_priv, i) > dev_priv->gt.cleanup_ring(ring); > > - if (i915.enable_execlists) > - /* > - * Neither the BIOS, ourselves or any other kernel > - * expects the system to be in execlists mode on startup, > - * so we need to reset the GPU back to legacy mode. > - */ > - intel_gpu_reset(dev); > + if (i915.enable_execlists) { > + /* > + * Neither the BIOS, ourselves or any other kernel > + * expects the system to be in execlists mode on startup, > + * so we need to reset the GPU back to legacy mode. > + */ > + intel_gpu_reset(dev); > + } > } > > static void > -- > 1.9.1 >
On Fri, Dec 11, 2015 at 05:26:19PM +0100, Daniel Vetter wrote: > On Fri, Dec 11, 2015 at 02:59:09PM +0000, Nick Hoath wrote: > > Swap the order of context & engine cleanup, so that it is now > > contexts, then engines. > > This allows the context clean up code to do things like confirm > > that ring->dev->struct_mutex is locked without a NULL pointer > > dereference. > > This came about as a result of the 'intel_ring_initialized() must > > be simple and inline' patch now using ring->dev as an initialised > > flag. > > Rename the cleanup function to reflect what it actually does. > > Also clean up some very annoying whitespace issues at the same time. > > > > v2: Also make the fix in i915_load_modeset_init, not just > > in i915_driver_unload (Chris Wilson) > > > > Signed-off-by: Nick Hoath <nicholas.hoath@intel.com> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: David Gordon <david.s.gordon@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Queued for -next, thanks for the patch. Well looked at BAT results still in time before pushing and dropped. Sounds like you&Mika are looking into what's going on here. -Daniel
A collection of small patches to fix some incorrect failure paths and generally tidy up the corresponding teardown code, mostly relating to contexts, and in particular the global default context that's created at startup. These should make subsequent reorganisation of other startup/teardown code easier and safer.
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 84e2b20..4dad121 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -449,8 +449,8 @@ static int i915_load_modeset_init(struct drm_device *dev) cleanup_gem: mutex_lock(&dev->struct_mutex); - i915_gem_cleanup_ringbuffer(dev); i915_gem_context_fini(dev); + i915_gem_cleanup_engines(dev); mutex_unlock(&dev->struct_mutex); cleanup_irq: intel_guc_ucode_fini(dev); @@ -1188,8 +1188,8 @@ int i915_driver_unload(struct drm_device *dev) intel_guc_ucode_fini(dev); mutex_lock(&dev->struct_mutex); - i915_gem_cleanup_ringbuffer(dev); i915_gem_context_fini(dev); + i915_gem_cleanup_engines(dev); mutex_unlock(&dev->struct_mutex); intel_fbc_cleanup_cfb(dev_priv); i915_gem_cleanup_stolen(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5edd393..e317f88 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3016,7 +3016,7 @@ int i915_gem_init_rings(struct drm_device *dev); int __must_check i915_gem_init_hw(struct drm_device *dev); int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice); void i915_gem_init_swizzling(struct drm_device *dev); -void i915_gem_cleanup_ringbuffer(struct drm_device *dev); +void i915_gem_cleanup_engines(struct drm_device *dev); int __must_check i915_gpu_idle(struct drm_device *dev); int __must_check i915_gem_suspend(struct drm_device *dev); void __i915_add_request(struct drm_i915_gem_request *req, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8e2acde..04a22db 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4823,7 +4823,7 @@ i915_gem_init_hw(struct drm_device *dev) ret = i915_gem_request_alloc(ring, ring->default_context, &req); if (ret) { - i915_gem_cleanup_ringbuffer(dev); + i915_gem_cleanup_engines(dev); goto out; } @@ -4836,7 +4836,7 @@ i915_gem_init_hw(struct drm_device *dev) if (ret && ret != -EIO) { DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret); i915_gem_request_cancel(req); - i915_gem_cleanup_ringbuffer(dev); + i915_gem_cleanup_engines(dev); goto out; } @@ -4844,7 +4844,7 @@ i915_gem_init_hw(struct drm_device *dev) if (ret && ret != -EIO) { DRM_ERROR("Context enable ring #%d failed %d\n", i, ret); i915_gem_request_cancel(req); - i915_gem_cleanup_ringbuffer(dev); + i915_gem_cleanup_engines(dev); goto out; } @@ -4919,7 +4919,7 @@ out_unlock: } void -i915_gem_cleanup_ringbuffer(struct drm_device *dev) +i915_gem_cleanup_engines(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_engine_cs *ring; @@ -4928,13 +4928,14 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev) for_each_ring(ring, dev_priv, i) dev_priv->gt.cleanup_ring(ring); - if (i915.enable_execlists) - /* - * Neither the BIOS, ourselves or any other kernel - * expects the system to be in execlists mode on startup, - * so we need to reset the GPU back to legacy mode. - */ - intel_gpu_reset(dev); + if (i915.enable_execlists) { + /* + * Neither the BIOS, ourselves or any other kernel + * expects the system to be in execlists mode on startup, + * so we need to reset the GPU back to legacy mode. + */ + intel_gpu_reset(dev); + } } static void
Swap the order of context & engine cleanup, so that it is now contexts, then engines. This allows the context clean up code to do things like confirm that ring->dev->struct_mutex is locked without a NULL pointer dereference. This came about as a result of the 'intel_ring_initialized() must be simple and inline' patch now using ring->dev as an initialised flag. Rename the cleanup function to reflect what it actually does. Also clean up some very annoying whitespace issues at the same time. v2: Also make the fix in i915_load_modeset_init, not just in i915_driver_unload (Chris Wilson) Signed-off-by: Nick Hoath <nicholas.hoath@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Gordon <david.s.gordon@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_dma.c | 4 ++-- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++++++----------- 3 files changed, 15 insertions(+), 14 deletions(-)