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

Message ID	1365118914-15753-3-git-send-email-ben@bwidawsk.net (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org> From: Ben Widawsky <ben@bwidawsk.net> To: intel-gfx@lists.freedesktop.org Date: Thu, 4 Apr 2013 16:41:48 -0700 Message-Id: <1365118914-15753-3-git-send-email-ben@bwidawsk.net> In-Reply-To: <1365118914-15753-1-git-send-email-ben@bwidawsk.net> References: <1365118914-15753-1-git-send-email-ben@bwidawsk.net> Cc: Ben Widawsky <ben@bwidawsk.net> Subject: [Intel-gfx] [PATCH 2/7] drm/i915: Move context special case to get() Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org

Message ID

1365118914-15753-3-git-send-email-ben@bwidawsk.net (mailing list archive)

State

New, archived

Headers

From: Ben Widawsky <ben@bwidawsk.net>
To: intel-gfx@lists.freedesktop.org
Date: Thu,  4 Apr 2013 16:41:48 -0700
Message-Id: <1365118914-15753-3-git-send-email-ben@bwidawsk.net>
In-Reply-To: <1365118914-15753-1-git-send-email-ben@bwidawsk.net>
References: <1365118914-15753-1-git-send-email-ben@bwidawsk.net>
Cc: Ben Widawsky <ben@bwidawsk.net>
Subject: [Intel-gfx] [PATCH 2/7] drm/i915: Move context special case to get()
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Sender: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org
Errors-To: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org

Commit Message

Ben Widawsky April 4, 2013, 11:41 p.m. UTC

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 <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 34 +++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

Comments

Ville Syrjälä April 5, 2013, 10:41 a.m. UTC | #1

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 <ben@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 (64<<10)
>  
> -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

Ben Widawsky April 5, 2013, 4:49 p.m. UTC | #2

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 <ben@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 (64<<10)
> >  
> > -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

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 (64<<10)
 
-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;

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

Commit Message

Comments

Patch