Re: [Intel-gfx] [PATCH 2/7] drm/i915: Move context special case to get()

2013-04-05 Thread Ville Syrjälä 
On Thu, Apr 04, 2013 at 04:41:48PM -0700, Ben Widawsky wrote:
 This allows us to make upcoming refcounting code a bit simpler, and
 cleaner. In addition, I think it makes the interface a bit nicer if the
 caller doesn't need to figure out default contexts and such.
 
 The interface works very similarly to the gem object ref counting, and
 I believe it makes sense to do so as we'll use it in a very similar way
 to objects (we currently use objects as a somewhat hackish way to manage
 context lifecycles).
 
 Signed-off-by: Ben Widawsky b...@bwidawsk.net
 ---
  drivers/gpu/drm/i915/i915_gem_context.c | 34 
 +++--
  1 file changed, 20 insertions(+), 14 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
 b/drivers/gpu/drm/i915/i915_gem_context.c
 index aa080ea..6211637 100644
 --- a/drivers/gpu/drm/i915/i915_gem_context.c
 +++ b/drivers/gpu/drm/i915/i915_gem_context.c
 @@ -95,8 +95,6 @@
   */
  #define CONTEXT_ALIGN (6410)
  
 -static struct i915_hw_context *
 -i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
  static int do_switch(struct i915_hw_context *to);
  
  static int get_context_size(struct drm_device *dev)
 @@ -291,9 +289,18 @@ void i915_gem_context_close(struct drm_device *dev, 
 struct drm_file *file)
  }
  
  static struct i915_hw_context *
 -i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 +i915_gem_context_get(struct intel_ring_buffer *ring,
 +  struct drm_i915_file_private *file_priv, u32 id)
  {
 - return (struct i915_hw_context *)idr_find(file_priv-context_idr, id);
 + struct i915_hw_context *ctx;
 +
 + if (id == DEFAULT_CONTEXT_ID)
 + ctx = ring-default_context;
 + else
 + ctx = (struct i915_hw_context *)
 + idr_find(file_priv-context_idr, id);
 +
 + return ctx;
  }
  
  static inline int
 @@ -440,16 +447,12 @@ int i915_switch_context(struct intel_ring_buffer *ring,
   if (ring != dev_priv-ring[RCS])
   return 0;
  
 - if (to_id == DEFAULT_CONTEXT_ID) {
 - to = ring-default_context;
 - } else {
 - if (file == NULL)
 - return -EINVAL;
 + if (file == NULL)
 + return -EINVAL;

This looks wrong. Before the NULL check was only done when to_id !=
DEFAULT_CONTEXT_ID. Now it's done always, which means the call from
i915_gpu_idle() will always fail.

  
 - to = i915_gem_context_get(file-driver_priv, to_id);
 - if (to == NULL)
 - return -ENOENT;
 - }
 + to = i915_gem_context_get(ring, file-driver_priv, to_id);
 + if (to == NULL)
 + return -ENOENT;
  
   return do_switch(to);
  }
 @@ -495,11 +498,14 @@ int i915_gem_context_destroy_ioctl(struct drm_device 
 *dev, void *data,
   if (!(dev-driver-driver_features  DRIVER_GEM))
   return -ENODEV;
  
 + if (args-ctx_id == DEFAULT_CONTEXT_ID)
 + return -ENOENT;
 +
   ret = i915_mutex_lock_interruptible(dev);
   if (ret)
   return ret;
  
 - ctx = i915_gem_context_get(file_priv, args-ctx_id);
 + ctx = i915_gem_context_get(NULL, file_priv, args-ctx_id);
   if (!ctx) {
   mutex_unlock(dev-struct_mutex);
   return -ENOENT;
 -- 
 1.8.2
 
 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Move context special case to get()

2013-04-05 Thread Ben Widawsky 
On Fri, Apr 05, 2013 at 01:41:11PM +0300, Ville Syrjälä wrote:
 On Thu, Apr 04, 2013 at 04:41:48PM -0700, Ben Widawsky wrote:
  This allows us to make upcoming refcounting code a bit simpler, and
  cleaner. In addition, I think it makes the interface a bit nicer if the
  caller doesn't need to figure out default contexts and such.
  
  The interface works very similarly to the gem object ref counting, and
  I believe it makes sense to do so as we'll use it in a very similar way
  to objects (we currently use objects as a somewhat hackish way to manage
  context lifecycles).
  
  Signed-off-by: Ben Widawsky b...@bwidawsk.net
  ---
   drivers/gpu/drm/i915/i915_gem_context.c | 34 
  +++--
   1 file changed, 20 insertions(+), 14 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
  b/drivers/gpu/drm/i915/i915_gem_context.c
  index aa080ea..6211637 100644
  --- a/drivers/gpu/drm/i915/i915_gem_context.c
  +++ b/drivers/gpu/drm/i915/i915_gem_context.c
  @@ -95,8 +95,6 @@
*/
   #define CONTEXT_ALIGN (6410)
   
  -static struct i915_hw_context *
  -i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
   static int do_switch(struct i915_hw_context *to);
   
   static int get_context_size(struct drm_device *dev)
  @@ -291,9 +289,18 @@ void i915_gem_context_close(struct drm_device *dev, 
  struct drm_file *file)
   }
   
   static struct i915_hw_context *
  -i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
  +i915_gem_context_get(struct intel_ring_buffer *ring,
  +struct drm_i915_file_private *file_priv, u32 id)
   {
  -   return (struct i915_hw_context *)idr_find(file_priv-context_idr, id);
  +   struct i915_hw_context *ctx;
  +
  +   if (id == DEFAULT_CONTEXT_ID)
  +   ctx = ring-default_context;
  +   else
  +   ctx = (struct i915_hw_context *)
  +   idr_find(file_priv-context_idr, id);
  +
  +   return ctx;
   }
   
   static inline int
  @@ -440,16 +447,12 @@ int i915_switch_context(struct intel_ring_buffer 
  *ring,
  if (ring != dev_priv-ring[RCS])
  return 0;
   
  -   if (to_id == DEFAULT_CONTEXT_ID) {
  -   to = ring-default_context;
  -   } else {
  -   if (file == NULL)
  -   return -EINVAL;
  +   if (file == NULL)
  +   return -EINVAL;
 
 This looks wrong. Before the NULL check was only done when to_id !=
 DEFAULT_CONTEXT_ID. Now it's done always, which means the call from
 i915_gpu_idle() will always fail.
 

Yeah, this definitely is incorrect. I wonder why I didn't hit any
problems on the i-g-t suite. Thanks for finding it. I'll come up with
some fix locally.

   
  -   to = i915_gem_context_get(file-driver_priv, to_id);
  -   if (to == NULL)
  -   return -ENOENT;
  -   }
  +   to = i915_gem_context_get(ring, file-driver_priv, to_id);
  +   if (to == NULL)
  +   return -ENOENT;
   
  return do_switch(to);
   }
  @@ -495,11 +498,14 @@ int i915_gem_context_destroy_ioctl(struct drm_device 
  *dev, void *data,
  if (!(dev-driver-driver_features  DRIVER_GEM))
  return -ENODEV;
   
  +   if (args-ctx_id == DEFAULT_CONTEXT_ID)
  +   return -ENOENT;
  +
  ret = i915_mutex_lock_interruptible(dev);
  if (ret)
  return ret;
   
  -   ctx = i915_gem_context_get(file_priv, args-ctx_id);
  +   ctx = i915_gem_context_get(NULL, file_priv, args-ctx_id);
  if (!ctx) {
  mutex_unlock(dev-struct_mutex);
  return -ENOENT;
  -- 
  1.8.2
  
  ___
  Intel-gfx mailing list
  Intel-gfx@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/intel-gfx
 
 -- 
 Ville Syrjälä
 Intel OTC

-- 
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/7] drm/i915: Move context special case to get()

2013-04-04 Thread Ben Widawsky 
This allows us to make upcoming refcounting code a bit simpler, and
cleaner. In addition, I think it makes the interface a bit nicer if the
caller doesn't need to figure out default contexts and such.

The interface works very similarly to the gem object ref counting, and
I believe it makes sense to do so as we'll use it in a very similar way
to objects (we currently use objects as a somewhat hackish way to manage
context lifecycles).

Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
 drivers/gpu/drm/i915/i915_gem_context.c | 34 +++--
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index aa080ea..6211637 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -95,8 +95,6 @@
  */
 #define CONTEXT_ALIGN (6410)
 
-static struct i915_hw_context *
-i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
 static int do_switch(struct i915_hw_context *to);
 
 static int get_context_size(struct drm_device *dev)
@@ -291,9 +289,18 @@ void i915_gem_context_close(struct drm_device *dev, struct 
drm_file *file)
 }
 
 static struct i915_hw_context *
-i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
+i915_gem_context_get(struct intel_ring_buffer *ring,
+struct drm_i915_file_private *file_priv, u32 id)
 {
-   return (struct i915_hw_context *)idr_find(file_priv-context_idr, id);
+   struct i915_hw_context *ctx;
+
+   if (id == DEFAULT_CONTEXT_ID)
+   ctx = ring-default_context;
+   else
+   ctx = (struct i915_hw_context *)
+   idr_find(file_priv-context_idr, id);
+
+   return ctx;
 }
 
 static inline int
@@ -440,16 +447,12 @@ int i915_switch_context(struct intel_ring_buffer *ring,
if (ring != dev_priv-ring[RCS])
return 0;
 
-   if (to_id == DEFAULT_CONTEXT_ID) {
-   to = ring-default_context;
-   } else {
-   if (file == NULL)
-   return -EINVAL;
+   if (file == NULL)
+   return -EINVAL;
 
-   to = i915_gem_context_get(file-driver_priv, to_id);
-   if (to == NULL)
-   return -ENOENT;
-   }
+   to = i915_gem_context_get(ring, file-driver_priv, to_id);
+   if (to == NULL)
+   return -ENOENT;
 
return do_switch(to);
 }
@@ -495,11 +498,14 @@ int i915_gem_context_destroy_ioctl(struct drm_device 
*dev, void *data,
if (!(dev-driver-driver_features  DRIVER_GEM))
return -ENODEV;
 
+   if (args-ctx_id == DEFAULT_CONTEXT_ID)
+   return -ENOENT;
+
ret = i915_mutex_lock_interruptible(dev);
if (ret)
return ret;
 
-   ctx = i915_gem_context_get(file_priv, args-ctx_id);
+   ctx = i915_gem_context_get(NULL, file_priv, args-ctx_id);
if (!ctx) {
mutex_unlock(dev-struct_mutex);
return -ENOENT;
-- 
1.8.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx