Re: [Intel-gfx] [PATCH 05/12] drm/i915: Rename i915_gem_context_reference/unreference()

[Daniel Vetter]

On Mon, May 23, 2016 at 10:17:01AM +0100, Tvrtko Ursulin wrote:
> 
> On 22/05/16 14:02, Chris Wilson wrote:
> >As these are wrappers around kref_get/kref_put() it is preferable to
> >follow the naming convention and use the same verb get/put in our
> >wrapper names for manipulating a reference to the context.
> 
> Not so sure about this one. We got objects, framebuffers and requests using
> (un)reference naming so don't see that making contexts different is a good
> idea.

And drm_blob too. For core modeset stuff I think converting to _get/put
might be worth it, too, but needs 3-stage approach: Add new _get/put
funcs. 2) cocci-generated patches for each driver/file to convert over. 3)
drop _reference/unreference variants once no longer used.
-Daniel

> 
> Regards,
> 
> Tvrtko
> 
> 
> >Signed-off-by: Chris Wilson 
> >Cc: Tvrtko Ursulin 
> >Cc: Joonas Lahtinen 
> >---
> >  drivers/gpu/drm/i915/i915_drv.h|  6 --
> >  drivers/gpu/drm/i915/i915_gem.c|  7 +++
> >  drivers/gpu/drm/i915/i915_gem_context.c| 22 ++
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  6 +++---
> >  drivers/gpu/drm/i915/intel_lrc.c   |  4 ++--
> >  5 files changed, 22 insertions(+), 23 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> >b/drivers/gpu/drm/i915/i915_drv.h
> >index e4e3614335ec..0a3442fcd93e 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -3435,12 +3435,14 @@ i915_gem_context_lookup(struct drm_i915_file_private 
> >*file_priv, u32 id)
> > return ctx;
> >  }
> >
> >-static inline void i915_gem_context_reference(struct i915_gem_context *ctx)
> >+static inline struct i915_gem_context *
> >+i915_gem_context_get(struct i915_gem_context *ctx)
> >  {
> > kref_get(>ref);
> >+return ctx;
> >  }
> >
> >-static inline void i915_gem_context_unreference(struct i915_gem_context 
> >*ctx)
> >+static inline void i915_gem_context_put(struct i915_gem_context *ctx)
> >  {
> > lockdep_assert_held(>i915->dev->struct_mutex);
> > kref_put(>ref, i915_gem_context_free);
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> >b/drivers/gpu/drm/i915/i915_gem.c
> >index 728b66911840..d7ae030e5a0b 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -1418,7 +1418,7 @@ static void i915_gem_request_retire(struct 
> >drm_i915_gem_request *request)
> >request->engine);
> > }
> >
> >-i915_gem_context_unreference(request->ctx);
> >+i915_gem_context_put(request->ctx);
> > i915_gem_request_unreference(request);
> >  }
> >
> >@@ -2775,8 +2775,7 @@ __i915_gem_request_alloc(struct intel_engine_cs 
> >*engine,
> > req->i915 = dev_priv;
> > req->engine = engine;
> > req->reset_counter = reset_counter;
> >-req->ctx  = ctx;
> >-i915_gem_context_reference(req->ctx);
> >+req->ctx = i915_gem_context_get(ctx);
> >
> > /*
> >  * Reserve space in the ring buffer for all the commands required to
> >@@ -2798,7 +2797,7 @@ __i915_gem_request_alloc(struct intel_engine_cs 
> >*engine,
> > return 0;
> >
> >  err_ctx:
> >-i915_gem_context_unreference(ctx);
> >+i915_gem_context_put(ctx);
> >  err:
> > kmem_cache_free(dev_priv->requests, req);
> > return ret;
> >diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
> >b/drivers/gpu/drm/i915/i915_gem_context.c
> >index aa329175f73a..a4c6377eea32 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_context.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_context.c
> >@@ -290,7 +290,7 @@ __create_hw_context(struct drm_device *dev,
> > return ctx;
> >
> >  err_out:
> >-i915_gem_context_unreference(ctx);
> >+i915_gem_context_put(ctx);
> > return ERR_PTR(ret);
> >  }
> >
> >@@ -351,7 +351,7 @@ err_unpin:
> > i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
> >  err_destroy:
> > idr_remove(_priv->context_idr, ctx->user_handle);
> >-i915_gem_context_unreference(ctx);
> >+i915_gem_context_put(ctx);
> > return ERR_PTR(ret);
> >  }
> >
> >@@ -363,7 +363,7 @@ static void i915_gem_context_unpin(struct 
> >i915_gem_context *ctx,
> > } else {
> > if (engine->id == RCS && ctx->legacy_hw_ctx.rcs_state)
> > 
> > i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
> >-i915_gem_context_unreference(ctx);
> >+i915_gem_context_put(ctx);
> > }
> >  }
> >
> >@@ -463,7 +463,7 @@ void i915_gem_context_fini(struct drm_device *dev)
> > if (dctx->legacy_hw_ctx.rcs_state)
> > i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
> >
> >-i915_gem_context_unreference(dctx);
> >+i915_gem_context_put(dctx);
> > dev_priv->kernel_context = NULL;
> >
> > ida_destroy(_priv->context_hw_ida);
> >@@ -473,7 +473,7 @@ static int context_idr_cleanup(int id, 

Re: [Intel-gfx] [PATCH 05/12] drm/i915: Rename i915_gem_context_reference/unreference()

[Chris Wilson]

On Mon, May 23, 2016 at 10:17:01AM +0100, Tvrtko Ursulin wrote:
> 
> On 22/05/16 14:02, Chris Wilson wrote:
> >As these are wrappers around kref_get/kref_put() it is preferable to
> >follow the naming convention and use the same verb get/put in our
> >wrapper names for manipulating a reference to the context.
> 
> Not so sure about this one. We got objects, framebuffers and
> requests using (un)reference naming so don't see that making
> contexts different is a good idea.

I've converted objects and requests in the lockless patches. So contexts
were a bit of an eyesore in amongst the *_get()/*_put().
-Chris

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

Re: [Intel-gfx] [PATCH 05/12] drm/i915: Rename i915_gem_context_reference/unreference()

[Tvrtko Ursulin]

On 22/05/16 14:02, Chris Wilson wrote:

As these are wrappers around kref_get/kref_put() it is preferable to
follow the naming convention and use the same verb get/put in our
wrapper names for manipulating a reference to the context.


Not so sure about this one. We got objects, framebuffers and requests 
using (un)reference naming so don't see that making contexts different 
is a good idea.


Regards,

Tvrtko



Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
  drivers/gpu/drm/i915/i915_drv.h|  6 --
  drivers/gpu/drm/i915/i915_gem.c|  7 +++
  drivers/gpu/drm/i915/i915_gem_context.c| 22 ++
  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  6 +++---
  drivers/gpu/drm/i915/intel_lrc.c   |  4 ++--
  5 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4e3614335ec..0a3442fcd93e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3435,12 +3435,14 @@ i915_gem_context_lookup(struct drm_i915_file_private 
*file_priv, u32 id)
return ctx;
  }

-static inline void i915_gem_context_reference(struct i915_gem_context *ctx)
+static inline struct i915_gem_context *
+i915_gem_context_get(struct i915_gem_context *ctx)
  {
kref_get(>ref);
+   return ctx;
  }

-static inline void i915_gem_context_unreference(struct i915_gem_context *ctx)
+static inline void i915_gem_context_put(struct i915_gem_context *ctx)
  {
lockdep_assert_held(>i915->dev->struct_mutex);
kref_put(>ref, i915_gem_context_free);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 728b66911840..d7ae030e5a0b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1418,7 +1418,7 @@ static void i915_gem_request_retire(struct 
drm_i915_gem_request *request)
   request->engine);
}

-   i915_gem_context_unreference(request->ctx);
+   i915_gem_context_put(request->ctx);
i915_gem_request_unreference(request);
  }

@@ -2775,8 +2775,7 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
req->i915 = dev_priv;
req->engine = engine;
req->reset_counter = reset_counter;
-   req->ctx  = ctx;
-   i915_gem_context_reference(req->ctx);
+   req->ctx = i915_gem_context_get(ctx);

/*
 * Reserve space in the ring buffer for all the commands required to
@@ -2798,7 +2797,7 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
return 0;

  err_ctx:
-   i915_gem_context_unreference(ctx);
+   i915_gem_context_put(ctx);
  err:
kmem_cache_free(dev_priv->requests, req);
return ret;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index aa329175f73a..a4c6377eea32 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -290,7 +290,7 @@ __create_hw_context(struct drm_device *dev,
return ctx;

  err_out:
-   i915_gem_context_unreference(ctx);
+   i915_gem_context_put(ctx);
return ERR_PTR(ret);
  }

@@ -351,7 +351,7 @@ err_unpin:
i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
  err_destroy:
idr_remove(_priv->context_idr, ctx->user_handle);
-   i915_gem_context_unreference(ctx);
+   i915_gem_context_put(ctx);
return ERR_PTR(ret);
  }

@@ -363,7 +363,7 @@ static void i915_gem_context_unpin(struct i915_gem_context 
*ctx,
} else {
if (engine->id == RCS && ctx->legacy_hw_ctx.rcs_state)

i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
-   i915_gem_context_unreference(ctx);
+   i915_gem_context_put(ctx);
}
  }

@@ -463,7 +463,7 @@ void i915_gem_context_fini(struct drm_device *dev)
if (dctx->legacy_hw_ctx.rcs_state)
i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);

-   i915_gem_context_unreference(dctx);
+   i915_gem_context_put(dctx);
dev_priv->kernel_context = NULL;

ida_destroy(_priv->context_hw_ida);
@@ -473,7 +473,7 @@ static int context_idr_cleanup(int id, void *p, void *data)
  {
struct i915_gem_context *ctx = p;

-   i915_gem_context_unreference(ctx);
+   i915_gem_context_put(ctx);
return 0;
  }

@@ -785,10 +785,9 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)

/* obj is kept alive until the next request by its active ref */
i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state);
-   i915_gem_context_unreference(from);
+   i915_gem_context_put(from);
}
-   i915_gem_context_reference(to);
-   engine->last_context = to;
+