[Intel-gfx] [PATCH 1/2] drm/i915: Some cleanups for the ppgtt lifetime handling

2014-07-30 Thread Daniel Vetter
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.

Cc: Michel Thierry michel.thie...@intel.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
 drivers/gpu/drm/i915/i915_drv.h |  1 -
 drivers/gpu/drm/i915/i915_gem.c |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c | 35 -
 drivers/gpu/drm/i915/i915_gem_gtt.c | 17 ++--
 drivers/gpu/drm/i915/i915_gem_gtt.h | 12 ++-
 5 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5fe3b1dfc34c..a89eb87e4af6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2497,7 +2497,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..1a46be5d2979 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4511,7 +4511,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma)
ppgtt = vm_to_ppgtt(vm);
 
if (ppgtt)
-   kref_put(ppgtt-ref, ppgtt_release);
+   i915_ppgtt_put(ppgtt);
 
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..603a227e3aaa 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 (6410)
 #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))
@@ -172,7 +145,7 @@ void i915_gem_context_free(struct kref *ctx_ref)
}
 
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 +192,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 +204,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 +312,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;
-   

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Some cleanups for the ppgtt lifetime handling

2014-07-30 Thread Chris Wilson
On Wed, Jul 30, 2014 at 12:44:05PM +0200, Daniel Vetter wrote:
 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.
 
 Cc: Michel Thierry michel.thie...@intel.com
 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
 ---
  drivers/gpu/drm/i915/i915_drv.h |  1 -
  drivers/gpu/drm/i915/i915_gem.c |  2 +-
  drivers/gpu/drm/i915/i915_gem_context.c | 35 
 -
  drivers/gpu/drm/i915/i915_gem_gtt.c | 17 ++--
  drivers/gpu/drm/i915/i915_gem_gtt.h | 12 ++-
  5 files changed, 31 insertions(+), 36 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
 index 5fe3b1dfc34c..a89eb87e4af6 100644
 --- a/drivers/gpu/drm/i915/i915_drv.h
 +++ b/drivers/gpu/drm/i915/i915_drv.h
 @@ -2497,7 +2497,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..1a46be5d2979 100644
 --- a/drivers/gpu/drm/i915/i915_gem.c
 +++ b/drivers/gpu/drm/i915/i915_gem.c
 @@ -4511,7 +4511,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma)
   ppgtt = vm_to_ppgtt(vm);
  
   if (ppgtt)
 - kref_put(ppgtt-ref, ppgtt_release);
 + i915_ppgtt_put(ppgtt);

Move the if(ppgtt) into i915_ppgtt_put(). And trust in the gcc DCE.
For get, do if (ppgtt)...; return ppgtt. And trust gcc.

Doing that can make callers much neater.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx