| Message ID | 1407330298-27065-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > Sent: Wednesday, August 06, 2014 2:05 PM > To: Intel Graphics Development > Cc: Daniel Vetter; Thierry, Michel; Chris Wilson > Subject: [PATCH 02/15] drm/i915: Some cleanups for the ppgtt lifetime > handling > > So when reviewing Michel's patch I've noticed a few things and cleaned > them up: > - The early checks in ppgtt_release are now redundant: The inactive > list should always be empty now, so we can ditch these checks. Even > for the aliasing ppgtt (though that's a different confusion) since > we tear that down after all the objects are gone. > - The ppgtt handling functions are splattered all over. Consolidate > them in i915_gem_gtt.c, give them OCD prefixes and add wrappers for > get/put. > - There was a bit a confusion in ppgtt_release about whether it cares > about the active or inactive list. It should care about them both, > so augment the WARNINGs to check for both. > > There's still create_vm_for_ctx left to do, put that is blocked on the > removal of ppgtt->ctx. Once that's done we can rename it to > i915_ppgtt_create and move it to its siblings for handling ppgtts. > > v2: Move the ppgtt checks into the inline get/put functions as > suggested by Chris. > > v3: Inline the now redundant ppgtt local variable. > > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/i915_gem.c | 5 +---- > drivers/gpu/drm/i915/i915_gem_context.c | 36 ++++---------------------------- > - > drivers/gpu/drm/i915/i915_gem_gtt.c | 20 +++++++++++++----- > drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++++++- > 5 files changed, 33 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index 5a18680011da..d349dd75ed69 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2512,7 +2512,6 @@ void i915_gem_object_ggtt_unpin(struct > drm_i915_gem_object *obj); > #define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, > base) > #define vm_to_ppgtt(vm) container_of(vm, struct i915_hw_ppgtt, base) > int __must_check i915_gem_context_init(struct drm_device *dev); > -void ppgtt_release(struct kref *kref); > void i915_gem_context_fini(struct drm_device *dev); > void i915_gem_context_reset(struct drm_device *dev); > int i915_gem_context_open(struct drm_device *dev, struct drm_file *file); > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c > index 25a32b9c9b4b..b33a677b4b1e 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4500,7 +4500,6 @@ struct i915_vma *i915_gem_obj_to_vma(struct > drm_i915_gem_object *obj, > void i915_gem_vma_destroy(struct i915_vma *vma) > { > struct i915_address_space *vm = NULL; > - struct i915_hw_ppgtt *ppgtt = NULL; > WARN_ON(vma->node.allocated); > > /* Keep the vma as a placeholder in the execbuffer reservation lists > */ > @@ -4508,10 +4507,8 @@ void i915_gem_vma_destroy(struct i915_vma > *vma) > return; > > vm = vma->vm; > - ppgtt = vm_to_ppgtt(vm); > > - if (ppgtt) > - kref_put(&ppgtt->ref, ppgtt_release); > + i915_ppgtt_put(vm_to_ppgtt(vm)); > > list_del(&vma->vma_link); > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index ae706cba05ae..899c6a7a5920 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -96,33 +96,6 @@ > #define GEN6_CONTEXT_ALIGN (64<<10) > #define GEN7_CONTEXT_ALIGN 4096 > > -static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt) > -{ > - struct drm_device *dev = ppgtt->base.dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - struct i915_address_space *vm = &ppgtt->base; > - > - if (ppgtt == dev_priv->mm.aliasing_ppgtt || > - (list_empty(&vm->active_list) && list_empty(&vm->inactive_list))) > { > - ppgtt->base.cleanup(&ppgtt->base); > - return; > - } > - > - /* vmas should already be unbound */ > - WARN_ON(!list_empty(&vm->active_list)); > - > - ppgtt->base.cleanup(&ppgtt->base); > -} > - > -void ppgtt_release(struct kref *kref) > -{ > - struct i915_hw_ppgtt *ppgtt = > - container_of(kref, struct i915_hw_ppgtt, ref); > - > - do_ppgtt_cleanup(ppgtt); > - kfree(ppgtt); > -} > - > static size_t get_context_alignment(struct drm_device *dev) > { > if (IS_GEN6(dev)) > @@ -171,8 +144,7 @@ void i915_gem_context_free(struct kref *ctx_ref) > ppgtt = ctx_to_ppgtt(ctx); > } > > - if (ppgtt) > - kref_put(&ppgtt->ref, ppgtt_release); > + i915_ppgtt_put(ppgtt); > if (ctx->legacy_hw_ctx.rcs_state) > drm_gem_object_unreference(&ctx- > >legacy_hw_ctx.rcs_state->base); > list_del(&ctx->link); > @@ -219,7 +191,7 @@ create_vm_for_ctx(struct drm_device *dev, struct > intel_context *ctx) > if (!ppgtt) > return ERR_PTR(-ENOMEM); > > - ret = i915_gem_init_ppgtt(dev, ppgtt); > + ret = i915_ppgtt_init(dev, ppgtt); > if (ret) { > kfree(ppgtt); > return ERR_PTR(ret); > @@ -231,7 +203,7 @@ create_vm_for_ctx(struct drm_device *dev, struct > intel_context *ctx) > > static struct intel_context * > __create_hw_context(struct drm_device *dev, > - struct drm_i915_file_private *file_priv) > + struct drm_i915_file_private *file_priv) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_context *ctx; > @@ -339,7 +311,7 @@ i915_gem_create_context(struct drm_device *dev, > /* For platforms which only have aliasing PPGTT, we fake the > * address space and refcounting. */ > ctx->vm = &dev_priv->mm.aliasing_ppgtt->base; > - kref_get(&dev_priv->mm.aliasing_ppgtt->ref); > + i915_ppgtt_get(dev_priv->mm.aliasing_ppgtt); > } else > ctx->vm = &dev_priv->gtt.base; > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 90c3d0fae3f1..1a20e1b4f052 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -1191,7 +1191,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt > *ppgtt) > return 0; > } > > -int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt > *ppgtt) > +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) > { > struct drm_i915_private *dev_priv = dev->dev_private; > int ret = 0; > @@ -1222,6 +1222,19 @@ int i915_gem_init_ppgtt(struct drm_device *dev, > struct i915_hw_ppgtt *ppgtt) > return ret; > } > > +void i915_ppgtt_release(struct kref *kref) > +{ > + struct i915_hw_ppgtt *ppgtt = > + container_of(kref, struct i915_hw_ppgtt, ref); > + > + /* vmas should already be unbound */ > + WARN_ON(!list_empty(&ppgtt->base.active_list)); > + WARN_ON(!list_empty(&ppgtt->base.inactive_list)); > + > + ppgtt->base.cleanup(&ppgtt->base); > + kfree(ppgtt); > +} > + > static void > ppgtt_bind_vma(struct i915_vma *vma, > enum i915_cache_level cache_level, > @@ -2159,15 +2172,12 @@ i915_gem_obj_lookup_or_create_vma(struct > drm_i915_gem_object *obj, > struct i915_address_space *vm) > { > struct i915_vma *vma; > - struct i915_hw_ppgtt *ppgtt = NULL; > > vma = i915_gem_obj_to_vma(obj, vm); > if (!vma) > vma = __i915_gem_vma_create(obj, vm); > > - ppgtt = vm_to_ppgtt(vm); > - if (ppgtt) > - kref_get(&ppgtt->ref); > + i915_ppgtt_get(vm_to_ppgtt(vm)); > > return vma; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > b/drivers/gpu/drm/i915/i915_gem_gtt.h > index 8d6f7c18c404..57d401343e8d 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -273,7 +273,19 @@ void i915_gem_setup_global_gtt(struct drm_device > *dev, unsigned long start, > unsigned long mappable_end, unsigned long > end); > > bool intel_enable_ppgtt(struct drm_device *dev, bool full); > -int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt > *ppgtt); > + > +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt); > +void i915_ppgtt_release(struct kref *kref); > +static inline void i915_ppgtt_get(struct i915_hw_ppgtt *ppgtt) > +{ > + if (ppgtt) > + kref_get(&ppgtt->ref); > +} > +static inline void i915_ppgtt_put(struct i915_hw_ppgtt *ppgtt) > +{ > + if (ppgtt) > + kref_put(&ppgtt->ref, i915_ppgtt_release); > +} > > void i915_check_and_clear_faults(struct drm_device *dev); > void i915_gem_suspend_gtt_mappings(struct drm_device *dev); > -- > 1.9.3 Reviewed-by: Michel Thierry <michel.thierry@intel.com>
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5a18680011da..d349dd75ed69 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2512,7 +2512,6 @@ void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj); #define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, base) #define vm_to_ppgtt(vm) container_of(vm, struct i915_hw_ppgtt, base) int __must_check i915_gem_context_init(struct drm_device *dev); -void ppgtt_release(struct kref *kref); void i915_gem_context_fini(struct drm_device *dev); void i915_gem_context_reset(struct drm_device *dev); int i915_gem_context_open(struct drm_device *dev, struct drm_file *file); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 25a32b9c9b4b..b33a677b4b1e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4500,7 +4500,6 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, void i915_gem_vma_destroy(struct i915_vma *vma) { struct i915_address_space *vm = NULL; - struct i915_hw_ppgtt *ppgtt = NULL; WARN_ON(vma->node.allocated); /* Keep the vma as a placeholder in the execbuffer reservation lists */ @@ -4508,10 +4507,8 @@ void i915_gem_vma_destroy(struct i915_vma *vma) return; vm = vma->vm; - ppgtt = vm_to_ppgtt(vm); - if (ppgtt) - kref_put(&ppgtt->ref, ppgtt_release); + i915_ppgtt_put(vm_to_ppgtt(vm)); list_del(&vma->vma_link); diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index ae706cba05ae..899c6a7a5920 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -96,33 +96,6 @@ #define GEN6_CONTEXT_ALIGN (64<<10) #define GEN7_CONTEXT_ALIGN 4096 -static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt) -{ - struct drm_device *dev = ppgtt->base.dev; - struct drm_i915_private *dev_priv = dev->dev_private; - struct i915_address_space *vm = &ppgtt->base; - - if (ppgtt == dev_priv->mm.aliasing_ppgtt || - (list_empty(&vm->active_list) && list_empty(&vm->inactive_list))) { - ppgtt->base.cleanup(&ppgtt->base); - return; - } - - /* vmas should already be unbound */ - WARN_ON(!list_empty(&vm->active_list)); - - ppgtt->base.cleanup(&ppgtt->base); -} - -void ppgtt_release(struct kref *kref) -{ - struct i915_hw_ppgtt *ppgtt = - container_of(kref, struct i915_hw_ppgtt, ref); - - do_ppgtt_cleanup(ppgtt); - kfree(ppgtt); -} - static size_t get_context_alignment(struct drm_device *dev) { if (IS_GEN6(dev)) @@ -171,8 +144,7 @@ void i915_gem_context_free(struct kref *ctx_ref) ppgtt = ctx_to_ppgtt(ctx); } - if (ppgtt) - kref_put(&ppgtt->ref, ppgtt_release); + i915_ppgtt_put(ppgtt); if (ctx->legacy_hw_ctx.rcs_state) drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base); list_del(&ctx->link); @@ -219,7 +191,7 @@ create_vm_for_ctx(struct drm_device *dev, struct intel_context *ctx) if (!ppgtt) return ERR_PTR(-ENOMEM); - ret = i915_gem_init_ppgtt(dev, ppgtt); + ret = i915_ppgtt_init(dev, ppgtt); if (ret) { kfree(ppgtt); return ERR_PTR(ret); @@ -231,7 +203,7 @@ create_vm_for_ctx(struct drm_device *dev, struct intel_context *ctx) static struct intel_context * __create_hw_context(struct drm_device *dev, - struct drm_i915_file_private *file_priv) + struct drm_i915_file_private *file_priv) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_context *ctx; @@ -339,7 +311,7 @@ i915_gem_create_context(struct drm_device *dev, /* For platforms which only have aliasing PPGTT, we fake the * address space and refcounting. */ ctx->vm = &dev_priv->mm.aliasing_ppgtt->base; - kref_get(&dev_priv->mm.aliasing_ppgtt->ref); + i915_ppgtt_get(dev_priv->mm.aliasing_ppgtt); } else ctx->vm = &dev_priv->gtt.base; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 90c3d0fae3f1..1a20e1b4f052 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1191,7 +1191,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) return 0; } -int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) { struct drm_i915_private *dev_priv = dev->dev_private; int ret = 0; @@ -1222,6 +1222,19 @@ int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) return ret; } +void i915_ppgtt_release(struct kref *kref) +{ + struct i915_hw_ppgtt *ppgtt = + container_of(kref, struct i915_hw_ppgtt, ref); + + /* vmas should already be unbound */ + WARN_ON(!list_empty(&ppgtt->base.active_list)); + WARN_ON(!list_empty(&ppgtt->base.inactive_list)); + + ppgtt->base.cleanup(&ppgtt->base); + kfree(ppgtt); +} + static void ppgtt_bind_vma(struct i915_vma *vma, enum i915_cache_level cache_level, @@ -2159,15 +2172,12 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj, struct i915_address_space *vm) { struct i915_vma *vma; - struct i915_hw_ppgtt *ppgtt = NULL; vma = i915_gem_obj_to_vma(obj, vm); if (!vma) vma = __i915_gem_vma_create(obj, vm); - ppgtt = vm_to_ppgtt(vm); - if (ppgtt) - kref_get(&ppgtt->ref); + i915_ppgtt_get(vm_to_ppgtt(vm)); return vma; } diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 8d6f7c18c404..57d401343e8d 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -273,7 +273,19 @@ void i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start, unsigned long mappable_end, unsigned long end); bool intel_enable_ppgtt(struct drm_device *dev, bool full); -int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt); + +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt); +void i915_ppgtt_release(struct kref *kref); +static inline void i915_ppgtt_get(struct i915_hw_ppgtt *ppgtt) +{ + if (ppgtt) + kref_get(&ppgtt->ref); +} +static inline void i915_ppgtt_put(struct i915_hw_ppgtt *ppgtt) +{ + if (ppgtt) + kref_put(&ppgtt->ref, i915_ppgtt_release); +} void i915_check_and_clear_faults(struct drm_device *dev); void i915_gem_suspend_gtt_mappings(struct drm_device *dev);
So when reviewing Michel's patch I've noticed a few things and cleaned them up: - The early checks in ppgtt_release are now redundant: The inactive list should always be empty now, so we can ditch these checks. Even for the aliasing ppgtt (though that's a different confusion) since we tear that down after all the objects are gone. - The ppgtt handling functions are splattered all over. Consolidate them in i915_gem_gtt.c, give them OCD prefixes and add wrappers for get/put. - There was a bit a confusion in ppgtt_release about whether it cares about the active or inactive list. It should care about them both, so augment the WARNINGs to check for both. There's still create_vm_for_ctx left to do, put that is blocked on the removal of ppgtt->ctx. Once that's done we can rename it to i915_ppgtt_create and move it to its siblings for handling ppgtts. v2: Move the ppgtt checks into the inline get/put functions as suggested by Chris. v3: Inline the now redundant ppgtt local variable. Cc: Michel Thierry <michel.thierry@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/i915_gem.c | 5 +---- drivers/gpu/drm/i915/i915_gem_context.c | 36 ++++----------------------------- drivers/gpu/drm/i915/i915_gem_gtt.c | 20 +++++++++++++----- drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++++++- 5 files changed, 33 insertions(+), 43 deletions(-)