[Intel-gfx] [PATCH 02/15] drm/i915: Some cleanups for the ppgtt lifetime handling
Thierry, Michel
michel.thierry at intel.com
Fri Aug 8 15:00:32 CEST 2014
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at 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 at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter at 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 at intel.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 6656 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140808/81fb8031/attachment.bin>
More information about the Intel-gfx
mailing list