[RFC] [PATCH] drm/i915: Require HW contexts (when possible)

[RFC] [PATCH] drm/i915: Require HW contexts (when possible)

[Ben Widawsky]

From: Ben Widawsky <benjamin.widawsky@intel.com>

Only compile tested

CC: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c         |  5 +---
 drivers/gpu/drm/i915/i915_drv.h         |  3 +--
 drivers/gpu/drm/i915/i915_gem.c         |  7 +++++-
 drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++++++++------------------
 drivers/gpu/drm/i915/i915_sysfs.c       |  6 ++---
 5 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1060a96..8c90559 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -750,14 +750,11 @@ int i915_reset(struct drm_device *dev)
 	 */
 	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
 			!dev_priv->ums.mm_suspended) {
-		bool hw_contexts_disabled = dev_priv->hw_contexts_disabled;
 		dev_priv->ums.mm_suspended = 0;
 
 		ret = i915_gem_init_hw(dev);
-		if (!hw_contexts_disabled && dev_priv->hw_contexts_disabled)
-			DRM_ERROR("HW contexts didn't survive reset\n");
-		mutex_unlock(&dev->struct_mutex);
 		if (ret) {
+			mutex_unlock(&dev->struct_mutex);
 			DRM_ERROR("Failed hw init on reset %d\n", ret);
 			return ret;
 		}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4ff8e9..21b2c37 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1411,7 +1411,6 @@ typedef struct drm_i915_private {
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
-	bool hw_contexts_disabled;
 	uint32_t hw_context_size;
 	struct list_head context_list;
 
@@ -2140,7 +2139,7 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
 }
 
 /* i915_gem_context.c */
-void i915_gem_context_init(struct drm_device *dev);
+int __must_check i915_gem_context_init(struct drm_device *dev);
 void i915_gem_context_fini(struct drm_device *dev);
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
 int i915_switch_context(struct intel_ring_buffer *ring,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 34df59b..0df40e6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4463,7 +4463,12 @@ i915_gem_init_hw(struct drm_device *dev)
 	 * XXX: There was some w/a described somewhere suggesting loading
 	 * contexts before PPGTT.
 	 */
-	i915_gem_context_init(dev);
+	ret = i915_gem_context_init(dev);
+	if (ret) {
+		DRM_ERROR("Context initialization failed %d\n", ret);
+		return ret;
+	}
+
 	if (dev_priv->mm.aliasing_ppgtt) {
 		ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
 		if (ret) {
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index cc619c1..4625670 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -244,36 +244,34 @@ err_destroy:
 	return ret;
 }
 
-void i915_gem_context_init(struct drm_device *dev)
+int i915_gem_context_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
 
-	if (!HAS_HW_CONTEXTS(dev)) {
-		dev_priv->hw_contexts_disabled = true;
-		DRM_DEBUG_DRIVER("Disabling HW Contexts; old hardware\n");
-		return;
-	}
+	if (!HAS_HW_CONTEXTS(dev))
+		return 0;
 
 	/* If called from reset, or thaw... we've been here already */
-	if (dev_priv->hw_contexts_disabled ||
-	    dev_priv->ring[RCS].default_context)
-		return;
+	if (dev_priv->ring[RCS].default_context)
+		return 0;
 
 	dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
 
 	if (dev_priv->hw_context_size > (1<<20)) {
-		dev_priv->hw_contexts_disabled = true;
 		DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
-		return;
+		return -E2BIG;
 	}
 
-	if (create_default_context(dev_priv)) {
-		dev_priv->hw_contexts_disabled = true;
-		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed\n");
-		return;
+	ret = create_default_context(dev_priv);
+	if (ret) {
+		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %d\n",
+				 ret);
+		return ret;
 	}
 
 	DRM_DEBUG_DRIVER("HW context support initialized\n");
+	return 0;
 }
 
 void i915_gem_context_fini(struct drm_device *dev)
@@ -281,7 +279,7 @@ void i915_gem_context_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
 
-	if (dev_priv->hw_contexts_disabled)
+	if (!HAS_HW_CONTEXTS(dev))
 		return;
 
 	/* The only known way to stop the gpu from accessing the hw context is
@@ -324,16 +322,16 @@ i915_gem_context_get_hang_stats(struct drm_device *dev,
 				struct drm_file *file,
 				u32 id)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct i915_hw_context *ctx;
 
 	if (id == DEFAULT_CONTEXT_ID)
 		return &file_priv->hang_stats;
 
-	ctx = NULL;
-	if (!dev_priv->hw_contexts_disabled)
-		ctx = i915_gem_context_get(file->driver_priv, id);
+	if (!HAS_HW_CONTEXTS(dev))
+		return ERR_PTR(-ENOENT);
+
+	ctx = i915_gem_context_get(file->driver_priv, id);
 	if (ctx == NULL)
 		return ERR_PTR(-ENOENT);
 
@@ -506,7 +504,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	struct i915_hw_context *to;
 
-	if (dev_priv->hw_contexts_disabled)
+	if (!HAS_HW_CONTEXTS(ring->dev))
 		return 0;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
@@ -531,7 +529,6 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *file)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_context_create *args = data;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct i915_hw_context *ctx;
@@ -540,7 +537,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	if (!(dev->driver->driver_features & DRIVER_GEM))
 		return -ENODEV;
 
-	if (dev_priv->hw_contexts_disabled)
+	if (!HAS_HW_CONTEXTS(dev))
 		return -ENODEV;
 
 	ret = i915_mutex_lock_interruptible(dev);
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index bb31ff3..47bc51b 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -183,13 +183,13 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
 	int slice = (int)(uintptr_t)attr->private;
 	int ret;
 
+	if (!HAS_HW_CONTEXTS(drm_dev))
+		return -ENXIO;
+
 	ret = l3_access_valid(drm_dev, offset);
 	if (ret)
 		return ret;
 
-	if (dev_priv->hw_contexts_disabled)
-		return -ENXIO;
-
 	ret = i915_mutex_lock_interruptible(drm_dev);
 	if (ret)
 		return ret;
-- 
1.8.4.1

Re: [RFC] [PATCH] drm/i915: Require HW contexts (when possible)

[Kenneth Graunke]

On 10/17/2013 09:59 AM, Ben Widawsky wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> Only compile tested
> 
> CC: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

The code looks good to me (but I didn't test it either).  I'm in favor
of this happening.

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

Re: [RFC] [PATCH] drm/i915: Require HW contexts (when possible)

[Ville Syrjälä]

On Thu, Oct 17, 2013 at 09:59:43AM -0700, Ben Widawsky wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> Only compile tested
> 
> CC: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  5 +---
>  drivers/gpu/drm/i915/i915_drv.h         |  3 +--
>  drivers/gpu/drm/i915/i915_gem.c         |  7 +++++-
>  drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++++++++------------------
>  drivers/gpu/drm/i915/i915_sysfs.c       |  6 ++---
>  5 files changed, 31 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1060a96..8c90559 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -750,14 +750,11 @@ int i915_reset(struct drm_device *dev)
>  	 */
>  	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
>  			!dev_priv->ums.mm_suspended) {
> -		bool hw_contexts_disabled = dev_priv->hw_contexts_disabled;
>  		dev_priv->ums.mm_suspended = 0;
>  
>  		ret = i915_gem_init_hw(dev);
> -		if (!hw_contexts_disabled && dev_priv->hw_contexts_disabled)
> -			DRM_ERROR("HW contexts didn't survive reset\n");
> -		mutex_unlock(&dev->struct_mutex);
>  		if (ret) {
> +			mutex_unlock(&dev->struct_mutex);

Botched locking.

>  			DRM_ERROR("Failed hw init on reset %d\n", ret);
>  			return ret;
>  		}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4ff8e9..21b2c37 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1411,7 +1411,6 @@ typedef struct drm_i915_private {
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
>  
> -	bool hw_contexts_disabled;
>  	uint32_t hw_context_size;
>  	struct list_head context_list;
>  
> @@ -2140,7 +2139,7 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
>  }
>  
>  /* i915_gem_context.c */
> -void i915_gem_context_init(struct drm_device *dev);
> +int __must_check i915_gem_context_init(struct drm_device *dev);
>  void i915_gem_context_fini(struct drm_device *dev);
>  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
>  int i915_switch_context(struct intel_ring_buffer *ring,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 34df59b..0df40e6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4463,7 +4463,12 @@ i915_gem_init_hw(struct drm_device *dev)
>  	 * XXX: There was some w/a described somewhere suggesting loading
>  	 * contexts before PPGTT.
>  	 */
> -	i915_gem_context_init(dev);
> +	ret = i915_gem_context_init(dev);
> +	if (ret) {

i915_gem_cleanup_ringbuffer() ?

Damn our error unwinding is hard to follow, which means it's probably
broken somewhere. From i915_load_modeset_init() we call
i915_gem_init() which does some stuff directly and then calls
i915_gem_init_hw(). And if we get some error later in i915_load_modeset_init(),
we manually unwind all the stuff that i915_gem_init() and i915_gem_init_hw()
supposedly did. This could sure use some i915_gem_fini() and i915_gem_fini_hw()
to keep the do+undo code close together.

> +		DRM_ERROR("Context initialization failed %d\n", ret);
> +		return ret;
> +	}
> +
>  	if (dev_priv->mm.aliasing_ppgtt) {
>  		ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
>  		if (ret) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index cc619c1..4625670 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -244,36 +244,34 @@ err_destroy:
>  	return ret;
>  }
>  
> -void i915_gem_context_init(struct drm_device *dev)
> +int i915_gem_context_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
>  
> -	if (!HAS_HW_CONTEXTS(dev)) {
> -		dev_priv->hw_contexts_disabled = true;
> -		DRM_DEBUG_DRIVER("Disabling HW Contexts; old hardware\n");
> -		return;
> -	}
> +	if (!HAS_HW_CONTEXTS(dev))
> +		return 0;
>  
>  	/* If called from reset, or thaw... we've been here already */
> -	if (dev_priv->hw_contexts_disabled ||
> -	    dev_priv->ring[RCS].default_context)
> -		return;
> +	if (dev_priv->ring[RCS].default_context)
> +		return 0;
>  
>  	dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
>  
>  	if (dev_priv->hw_context_size > (1<<20)) {
> -		dev_priv->hw_contexts_disabled = true;
>  		DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
> -		return;
> +		return -E2BIG;
>  	}
>  
> -	if (create_default_context(dev_priv)) {
> -		dev_priv->hw_contexts_disabled = true;
> -		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed\n");
> -		return;
> +	ret = create_default_context(dev_priv);
> +	if (ret) {
> +		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %d\n",
> +				 ret);
> +		return ret;
>  	}
>  
>  	DRM_DEBUG_DRIVER("HW context support initialized\n");
> +	return 0;
>  }
>  
>  void i915_gem_context_fini(struct drm_device *dev)
> @@ -281,7 +279,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
>  
> -	if (dev_priv->hw_contexts_disabled)
> +	if (!HAS_HW_CONTEXTS(dev))
>  		return;
>  
>  	/* The only known way to stop the gpu from accessing the hw context is
> @@ -324,16 +322,16 @@ i915_gem_context_get_hang_stats(struct drm_device *dev,
>  				struct drm_file *file,
>  				u32 id)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  	struct i915_hw_context *ctx;
>  
>  	if (id == DEFAULT_CONTEXT_ID)
>  		return &file_priv->hang_stats;
>  
> -	ctx = NULL;
> -	if (!dev_priv->hw_contexts_disabled)
> -		ctx = i915_gem_context_get(file->driver_priv, id);
> +	if (!HAS_HW_CONTEXTS(dev))
> +		return ERR_PTR(-ENOENT);
> +
> +	ctx = i915_gem_context_get(file->driver_priv, id);
>  	if (ctx == NULL)
>  		return ERR_PTR(-ENOENT);
>  
> @@ -506,7 +504,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  	struct i915_hw_context *to;
>  
> -	if (dev_priv->hw_contexts_disabled)
> +	if (!HAS_HW_CONTEXTS(ring->dev))
>  		return 0;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> @@ -531,7 +529,6 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  				  struct drm_file *file)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_context_create *args = data;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  	struct i915_hw_context *ctx;
> @@ -540,7 +537,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  	if (!(dev->driver->driver_features & DRIVER_GEM))
>  		return -ENODEV;
>  
> -	if (dev_priv->hw_contexts_disabled)
> +	if (!HAS_HW_CONTEXTS(dev))
>  		return -ENODEV;
>  
>  	ret = i915_mutex_lock_interruptible(dev);
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index bb31ff3..47bc51b 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -183,13 +183,13 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
>  	int slice = (int)(uintptr_t)attr->private;
>  	int ret;
>  
> +	if (!HAS_HW_CONTEXTS(drm_dev))
> +		return -ENXIO;
> +
>  	ret = l3_access_valid(drm_dev, offset);
>  	if (ret)
>  		return ret;
>  
> -	if (dev_priv->hw_contexts_disabled)
> -		return -ENXIO;
> -
>  	ret = i915_mutex_lock_interruptible(drm_dev);
>  	if (ret)
>  		return ret;
> -- 
> 1.8.4.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] [PATCH] drm/i915: Require HW contexts (when possible)

[Ben Widawsky]

On Thu, Oct 17, 2013 at 08:52:17PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 17, 2013 at 09:59:43AM -0700, Ben Widawsky wrote:
> > From: Ben Widawsky <benjamin.widawsky@intel.com>
> > 
> > Only compile tested
> > 
> > CC: Kenneth Graunke <kenneth@whitecape.org>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c         |  5 +---
> >  drivers/gpu/drm/i915/i915_drv.h         |  3 +--
> >  drivers/gpu/drm/i915/i915_gem.c         |  7 +++++-
> >  drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++++++++------------------
> >  drivers/gpu/drm/i915/i915_sysfs.c       |  6 ++---
> >  5 files changed, 31 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 1060a96..8c90559 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -750,14 +750,11 @@ int i915_reset(struct drm_device *dev)
> >  	 */
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
> >  			!dev_priv->ums.mm_suspended) {
> > -		bool hw_contexts_disabled = dev_priv->hw_contexts_disabled;
> >  		dev_priv->ums.mm_suspended = 0;
> >  
> >  		ret = i915_gem_init_hw(dev);
> > -		if (!hw_contexts_disabled && dev_priv->hw_contexts_disabled)
> > -			DRM_ERROR("HW contexts didn't survive reset\n");
> > -		mutex_unlock(&dev->struct_mutex);
> >  		if (ret) {
> > +			mutex_unlock(&dev->struct_mutex);
> 
> Botched locking.

My mistake was intentional at least, and not the usual copy/paste.
Somehow this looked right to me earlier.

Thanks.

> 
> >  			DRM_ERROR("Failed hw init on reset %d\n", ret);
> >  			return ret;
> >  		}
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index e4ff8e9..21b2c37 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1411,7 +1411,6 @@ typedef struct drm_i915_private {
> >  	struct drm_property *broadcast_rgb_property;
> >  	struct drm_property *force_audio_property;
> >  
> > -	bool hw_contexts_disabled;
> >  	uint32_t hw_context_size;
> >  	struct list_head context_list;
> >  
> > @@ -2140,7 +2139,7 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
> >  }
> >  
> >  /* i915_gem_context.c */
> > -void i915_gem_context_init(struct drm_device *dev);
> > +int __must_check i915_gem_context_init(struct drm_device *dev);
> >  void i915_gem_context_fini(struct drm_device *dev);
> >  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> >  int i915_switch_context(struct intel_ring_buffer *ring,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 34df59b..0df40e6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4463,7 +4463,12 @@ i915_gem_init_hw(struct drm_device *dev)
> >  	 * XXX: There was some w/a described somewhere suggesting loading
> >  	 * contexts before PPGTT.
> >  	 */
> > -	i915_gem_context_init(dev);
> > +	ret = i915_gem_context_init(dev);
> > +	if (ret) {
> 
> i915_gem_cleanup_ringbuffer() ?
> 
> Damn our error unwinding is hard to follow, which means it's probably
> broken somewhere. From i915_load_modeset_init() we call
> i915_gem_init() which does some stuff directly and then calls
> i915_gem_init_hw(). And if we get some error later in i915_load_modeset_init(),
> we manually unwind all the stuff that i915_gem_init() and i915_gem_init_hw()
> supposedly did. This could sure use some i915_gem_fini() and i915_gem_fini_hw()
> to keep the do+undo code close together.
> 

It's definitely a mess. TBF, it's been changing a lot more recently than
it had in the past.

But yeah this is missing. I wonder if I missed this last time I wrote
the patch too...? Thanks.

> > +		DRM_ERROR("Context initialization failed %d\n", ret);
> > +		return ret;
> > +	}
> > +
> >  	if (dev_priv->mm.aliasing_ppgtt) {
> >  		ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
> >  		if (ret) {
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index cc619c1..4625670 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -244,36 +244,34 @@ err_destroy:
> >  	return ret;
> >  }
> >  
> > -void i915_gem_context_init(struct drm_device *dev)
> > +int i915_gem_context_init(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret;
> >  
> > -	if (!HAS_HW_CONTEXTS(dev)) {
> > -		dev_priv->hw_contexts_disabled = true;
> > -		DRM_DEBUG_DRIVER("Disabling HW Contexts; old hardware\n");
> > -		return;
> > -	}
> > +	if (!HAS_HW_CONTEXTS(dev))
> > +		return 0;
> >  
> >  	/* If called from reset, or thaw... we've been here already */
> > -	if (dev_priv->hw_contexts_disabled ||
> > -	    dev_priv->ring[RCS].default_context)
> > -		return;
> > +	if (dev_priv->ring[RCS].default_context)
> > +		return 0;
> >  
> >  	dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
> >  
> >  	if (dev_priv->hw_context_size > (1<<20)) {
> > -		dev_priv->hw_contexts_disabled = true;
> >  		DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
> > -		return;
> > +		return -E2BIG;
> >  	}
> >  
> > -	if (create_default_context(dev_priv)) {
> > -		dev_priv->hw_contexts_disabled = true;
> > -		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed\n");
> > -		return;
> > +	ret = create_default_context(dev_priv);
> > +	if (ret) {
> > +		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %d\n",
> > +				 ret);
> > +		return ret;
> >  	}
> >  
> >  	DRM_DEBUG_DRIVER("HW context support initialized\n");
> > +	return 0;
> >  }
> >  
> >  void i915_gem_context_fini(struct drm_device *dev)
> > @@ -281,7 +279,7 @@ void i915_gem_context_fini(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
> >  
> > -	if (dev_priv->hw_contexts_disabled)
> > +	if (!HAS_HW_CONTEXTS(dev))
> >  		return;
> >  
> >  	/* The only known way to stop the gpu from accessing the hw context is
> > @@ -324,16 +322,16 @@ i915_gem_context_get_hang_stats(struct drm_device *dev,
> >  				struct drm_file *file,
> >  				u32 id)
> >  {
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_file_private *file_priv = file->driver_priv;
> >  	struct i915_hw_context *ctx;
> >  
> >  	if (id == DEFAULT_CONTEXT_ID)
> >  		return &file_priv->hang_stats;
> >  
> > -	ctx = NULL;
> > -	if (!dev_priv->hw_contexts_disabled)
> > -		ctx = i915_gem_context_get(file->driver_priv, id);
> > +	if (!HAS_HW_CONTEXTS(dev))
> > +		return ERR_PTR(-ENOENT);
> > +
> > +	ctx = i915_gem_context_get(file->driver_priv, id);
> >  	if (ctx == NULL)
> >  		return ERR_PTR(-ENOENT);
> >  
> > @@ -506,7 +504,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> >  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >  	struct i915_hw_context *to;
> >  
> > -	if (dev_priv->hw_contexts_disabled)
> > +	if (!HAS_HW_CONTEXTS(ring->dev))
> >  		return 0;
> >  
> >  	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> > @@ -531,7 +529,6 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> >  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> >  				  struct drm_file *file)
> >  {
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_context_create *args = data;
> >  	struct drm_i915_file_private *file_priv = file->driver_priv;
> >  	struct i915_hw_context *ctx;
> > @@ -540,7 +537,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> >  	if (!(dev->driver->driver_features & DRIVER_GEM))
> >  		return -ENODEV;
> >  
> > -	if (dev_priv->hw_contexts_disabled)
> > +	if (!HAS_HW_CONTEXTS(dev))
> >  		return -ENODEV;
> >  
> >  	ret = i915_mutex_lock_interruptible(dev);
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > index bb31ff3..47bc51b 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -183,13 +183,13 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
> >  	int slice = (int)(uintptr_t)attr->private;
> >  	int ret;
> >  
> > +	if (!HAS_HW_CONTEXTS(drm_dev))
> > +		return -ENXIO;
> > +
> >  	ret = l3_access_valid(drm_dev, offset);
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (dev_priv->hw_contexts_disabled)
> > -		return -ENXIO;
> > -
> >  	ret = i915_mutex_lock_interruptible(drm_dev);
> >  	if (ret)
> >  		return ret;
> > -- 
> > 1.8.4.1
> > 

Nobody is commenting on whether or not this is okay, which is what I was
really interesting.

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

[PATCH] [v2] drm/i915: Require HW contexts (when possible)

[Ben Widawsky]

v2: Fixed the botched locking on init_hw failure in i915_reset (Ville)
Call cleanup_ringbuffer on failed context create in init_hw (Ville)

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c         |  3 ---
 drivers/gpu/drm/i915/i915_drv.h         |  3 +--
 drivers/gpu/drm/i915/i915_gem.c         |  8 +++++-
 drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++++++++------------------
 drivers/gpu/drm/i915/i915_sysfs.c       |  6 ++---
 5 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1060a96..839925b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -750,12 +750,9 @@ int i915_reset(struct drm_device *dev)
 	 */
 	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
 			!dev_priv->ums.mm_suspended) {
-		bool hw_contexts_disabled = dev_priv->hw_contexts_disabled;
 		dev_priv->ums.mm_suspended = 0;
 
 		ret = i915_gem_init_hw(dev);
-		if (!hw_contexts_disabled && dev_priv->hw_contexts_disabled)
-			DRM_ERROR("HW contexts didn't survive reset\n");
 		mutex_unlock(&dev->struct_mutex);
 		if (ret) {
 			DRM_ERROR("Failed hw init on reset %d\n", ret);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4ff8e9..21b2c37 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1411,7 +1411,6 @@ typedef struct drm_i915_private {
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
-	bool hw_contexts_disabled;
 	uint32_t hw_context_size;
 	struct list_head context_list;
 
@@ -2140,7 +2139,7 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
 }
 
 /* i915_gem_context.c */
-void i915_gem_context_init(struct drm_device *dev);
+int __must_check i915_gem_context_init(struct drm_device *dev);
 void i915_gem_context_fini(struct drm_device *dev);
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
 int i915_switch_context(struct intel_ring_buffer *ring,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 34df59b..f8e9d40 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4463,7 +4463,13 @@ i915_gem_init_hw(struct drm_device *dev)
 	 * XXX: There was some w/a described somewhere suggesting loading
 	 * contexts before PPGTT.
 	 */
-	i915_gem_context_init(dev);
+	ret = i915_gem_context_init(dev);
+	if (ret) {
+		i915_gem_cleanup_ringbuffer();
+		DRM_ERROR("Context initialization failed %d\n", ret);
+		return ret;
+	}
+
 	if (dev_priv->mm.aliasing_ppgtt) {
 		ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
 		if (ret) {
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index cc619c1..4625670 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -244,36 +244,34 @@ err_destroy:
 	return ret;
 }
 
-void i915_gem_context_init(struct drm_device *dev)
+int i915_gem_context_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
 
-	if (!HAS_HW_CONTEXTS(dev)) {
-		dev_priv->hw_contexts_disabled = true;
-		DRM_DEBUG_DRIVER("Disabling HW Contexts; old hardware\n");
-		return;
-	}
+	if (!HAS_HW_CONTEXTS(dev))
+		return 0;
 
 	/* If called from reset, or thaw... we've been here already */
-	if (dev_priv->hw_contexts_disabled ||
-	    dev_priv->ring[RCS].default_context)
-		return;
+	if (dev_priv->ring[RCS].default_context)
+		return 0;
 
 	dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
 
 	if (dev_priv->hw_context_size > (1<<20)) {
-		dev_priv->hw_contexts_disabled = true;
 		DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
-		return;
+		return -E2BIG;
 	}
 
-	if (create_default_context(dev_priv)) {
-		dev_priv->hw_contexts_disabled = true;
-		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed\n");
-		return;
+	ret = create_default_context(dev_priv);
+	if (ret) {
+		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %d\n",
+				 ret);
+		return ret;
 	}
 
 	DRM_DEBUG_DRIVER("HW context support initialized\n");
+	return 0;
 }
 
 void i915_gem_context_fini(struct drm_device *dev)
@@ -281,7 +279,7 @@ void i915_gem_context_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
 
-	if (dev_priv->hw_contexts_disabled)
+	if (!HAS_HW_CONTEXTS(dev))
 		return;
 
 	/* The only known way to stop the gpu from accessing the hw context is
@@ -324,16 +322,16 @@ i915_gem_context_get_hang_stats(struct drm_device *dev,
 				struct drm_file *file,
 				u32 id)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct i915_hw_context *ctx;
 
 	if (id == DEFAULT_CONTEXT_ID)
 		return &file_priv->hang_stats;
 
-	ctx = NULL;
-	if (!dev_priv->hw_contexts_disabled)
-		ctx = i915_gem_context_get(file->driver_priv, id);
+	if (!HAS_HW_CONTEXTS(dev))
+		return ERR_PTR(-ENOENT);
+
+	ctx = i915_gem_context_get(file->driver_priv, id);
 	if (ctx == NULL)
 		return ERR_PTR(-ENOENT);
 
@@ -506,7 +504,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	struct i915_hw_context *to;
 
-	if (dev_priv->hw_contexts_disabled)
+	if (!HAS_HW_CONTEXTS(ring->dev))
 		return 0;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
@@ -531,7 +529,6 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *file)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_context_create *args = data;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct i915_hw_context *ctx;
@@ -540,7 +537,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	if (!(dev->driver->driver_features & DRIVER_GEM))
 		return -ENODEV;
 
-	if (dev_priv->hw_contexts_disabled)
+	if (!HAS_HW_CONTEXTS(dev))
 		return -ENODEV;
 
 	ret = i915_mutex_lock_interruptible(dev);
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index bb31ff3..47bc51b 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -183,13 +183,13 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
 	int slice = (int)(uintptr_t)attr->private;
 	int ret;
 
+	if (!HAS_HW_CONTEXTS(drm_dev))
+		return -ENXIO;
+
 	ret = l3_access_valid(drm_dev, offset);
 	if (ret)
 		return ret;
 
-	if (dev_priv->hw_contexts_disabled)
-		return -ENXIO;
-
 	ret = i915_mutex_lock_interruptible(drm_dev);
 	if (ret)
 		return ret;
-- 
1.8.4.1

Re: [PATCH] [v2] drm/i915: Require HW contexts (when possible)

[Ben Widawsky]

On Thu, Oct 17, 2013 at 02:49:54PM -0700, Ben Widawsky wrote:
> v2: Fixed the botched locking on init_hw failure in i915_reset (Ville)
> Call cleanup_ringbuffer on failed context create in init_hw (Ville)
> 
> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  3 ---
>  drivers/gpu/drm/i915/i915_drv.h         |  3 +--
>  drivers/gpu/drm/i915/i915_gem.c         |  8 +++++-
>  drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++++++++------------------
>  drivers/gpu/drm/i915/i915_sysfs.c       |  6 ++---
>  5 files changed, 31 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1060a96..839925b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -750,12 +750,9 @@ int i915_reset(struct drm_device *dev)
>  	 */
>  	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
>  			!dev_priv->ums.mm_suspended) {
> -		bool hw_contexts_disabled = dev_priv->hw_contexts_disabled;
>  		dev_priv->ums.mm_suspended = 0;
>  
>  		ret = i915_gem_init_hw(dev);
> -		if (!hw_contexts_disabled && dev_priv->hw_contexts_disabled)
> -			DRM_ERROR("HW contexts didn't survive reset\n");
>  		mutex_unlock(&dev->struct_mutex);
>  		if (ret) {
>  			DRM_ERROR("Failed hw init on reset %d\n", ret);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4ff8e9..21b2c37 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1411,7 +1411,6 @@ typedef struct drm_i915_private {
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
>  
> -	bool hw_contexts_disabled;
>  	uint32_t hw_context_size;
>  	struct list_head context_list;
>  
> @@ -2140,7 +2139,7 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
>  }
>  
>  /* i915_gem_context.c */
> -void i915_gem_context_init(struct drm_device *dev);
> +int __must_check i915_gem_context_init(struct drm_device *dev);
>  void i915_gem_context_fini(struct drm_device *dev);
>  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
>  int i915_switch_context(struct intel_ring_buffer *ring,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 34df59b..f8e9d40 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4463,7 +4463,13 @@ i915_gem_init_hw(struct drm_device *dev)
>  	 * XXX: There was some w/a described somewhere suggesting loading
>  	 * contexts before PPGTT.
>  	 */
> -	i915_gem_context_init(dev);
> +	ret = i915_gem_context_init(dev);
> +	if (ret) {
> +		i915_gem_cleanup_ringbuffer();
> +		DRM_ERROR("Context initialization failed %d\n", ret);
> +		return ret;
> +	}
> +

Oops. I clearly didn't build this. Will resend.

>  	if (dev_priv->mm.aliasing_ppgtt) {
>  		ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
>  		if (ret) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index cc619c1..4625670 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -244,36 +244,34 @@ err_destroy:
>  	return ret;
>  }
>  
> -void i915_gem_context_init(struct drm_device *dev)
> +int i915_gem_context_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
>  
> -	if (!HAS_HW_CONTEXTS(dev)) {
> -		dev_priv->hw_contexts_disabled = true;
> -		DRM_DEBUG_DRIVER("Disabling HW Contexts; old hardware\n");
> -		return;
> -	}
> +	if (!HAS_HW_CONTEXTS(dev))
> +		return 0;
>  
>  	/* If called from reset, or thaw... we've been here already */
> -	if (dev_priv->hw_contexts_disabled ||
> -	    dev_priv->ring[RCS].default_context)
> -		return;
> +	if (dev_priv->ring[RCS].default_context)
> +		return 0;
>  
>  	dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
>  
>  	if (dev_priv->hw_context_size > (1<<20)) {
> -		dev_priv->hw_contexts_disabled = true;
>  		DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
> -		return;
> +		return -E2BIG;
>  	}
>  
> -	if (create_default_context(dev_priv)) {
> -		dev_priv->hw_contexts_disabled = true;
> -		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed\n");
> -		return;
> +	ret = create_default_context(dev_priv);
> +	if (ret) {
> +		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %d\n",
> +				 ret);
> +		return ret;
>  	}
>  
>  	DRM_DEBUG_DRIVER("HW context support initialized\n");
> +	return 0;
>  }
>  
>  void i915_gem_context_fini(struct drm_device *dev)
> @@ -281,7 +279,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
>  
> -	if (dev_priv->hw_contexts_disabled)
> +	if (!HAS_HW_CONTEXTS(dev))
>  		return;
>  
>  	/* The only known way to stop the gpu from accessing the hw context is
> @@ -324,16 +322,16 @@ i915_gem_context_get_hang_stats(struct drm_device *dev,
>  				struct drm_file *file,
>  				u32 id)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  	struct i915_hw_context *ctx;
>  
>  	if (id == DEFAULT_CONTEXT_ID)
>  		return &file_priv->hang_stats;
>  
> -	ctx = NULL;
> -	if (!dev_priv->hw_contexts_disabled)
> -		ctx = i915_gem_context_get(file->driver_priv, id);
> +	if (!HAS_HW_CONTEXTS(dev))
> +		return ERR_PTR(-ENOENT);
> +
> +	ctx = i915_gem_context_get(file->driver_priv, id);
>  	if (ctx == NULL)
>  		return ERR_PTR(-ENOENT);
>  
> @@ -506,7 +504,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  	struct i915_hw_context *to;
>  
> -	if (dev_priv->hw_contexts_disabled)
> +	if (!HAS_HW_CONTEXTS(ring->dev))
>  		return 0;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> @@ -531,7 +529,6 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  				  struct drm_file *file)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_context_create *args = data;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  	struct i915_hw_context *ctx;
> @@ -540,7 +537,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  	if (!(dev->driver->driver_features & DRIVER_GEM))
>  		return -ENODEV;
>  
> -	if (dev_priv->hw_contexts_disabled)
> +	if (!HAS_HW_CONTEXTS(dev))
>  		return -ENODEV;
>  
>  	ret = i915_mutex_lock_interruptible(dev);
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index bb31ff3..47bc51b 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -183,13 +183,13 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
>  	int slice = (int)(uintptr_t)attr->private;
>  	int ret;
>  
> +	if (!HAS_HW_CONTEXTS(drm_dev))
> +		return -ENXIO;
> +
>  	ret = l3_access_valid(drm_dev, offset);
>  	if (ret)
>  		return ret;
>  
> -	if (dev_priv->hw_contexts_disabled)
> -		return -ENXIO;
> -
>  	ret = i915_mutex_lock_interruptible(drm_dev);
>  	if (ret)
>  		return ret;
> -- 
> 1.8.4.1
> 

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH] [v3] drm/i915: Require HW contexts (when possible)

[Ben Widawsky]

v2: Fixed the botched locking on init_hw failure in i915_reset (Ville)
Call cleanup_ringbuffer on failed context create in init_hw (Ville)

v3: Add dev argument ti clean_ringbuffer

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c         |  3 ---
 drivers/gpu/drm/i915/i915_drv.h         |  3 +--
 drivers/gpu/drm/i915/i915_gem.c         |  8 +++++-
 drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++++++++------------------
 drivers/gpu/drm/i915/i915_sysfs.c       |  6 ++---
 5 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1060a96..839925b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -750,12 +750,9 @@ int i915_reset(struct drm_device *dev)
 	 */
 	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
 			!dev_priv->ums.mm_suspended) {
-		bool hw_contexts_disabled = dev_priv->hw_contexts_disabled;
 		dev_priv->ums.mm_suspended = 0;
 
 		ret = i915_gem_init_hw(dev);
-		if (!hw_contexts_disabled && dev_priv->hw_contexts_disabled)
-			DRM_ERROR("HW contexts didn't survive reset\n");
 		mutex_unlock(&dev->struct_mutex);
 		if (ret) {
 			DRM_ERROR("Failed hw init on reset %d\n", ret);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4ff8e9..21b2c37 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1411,7 +1411,6 @@ typedef struct drm_i915_private {
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
-	bool hw_contexts_disabled;
 	uint32_t hw_context_size;
 	struct list_head context_list;
 
@@ -2140,7 +2139,7 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
 }
 
 /* i915_gem_context.c */
-void i915_gem_context_init(struct drm_device *dev);
+int __must_check i915_gem_context_init(struct drm_device *dev);
 void i915_gem_context_fini(struct drm_device *dev);
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
 int i915_switch_context(struct intel_ring_buffer *ring,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 34df59b..b43cd30 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4463,7 +4463,13 @@ i915_gem_init_hw(struct drm_device *dev)
 	 * XXX: There was some w/a described somewhere suggesting loading
 	 * contexts before PPGTT.
 	 */
-	i915_gem_context_init(dev);
+	ret = i915_gem_context_init(dev);
+	if (ret) {
+		i915_gem_cleanup_ringbuffer(dev);
+		DRM_ERROR("Context initialization failed %d\n", ret);
+		return ret;
+	}
+
 	if (dev_priv->mm.aliasing_ppgtt) {
 		ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
 		if (ret) {
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index cc619c1..4625670 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -244,36 +244,34 @@ err_destroy:
 	return ret;
 }
 
-void i915_gem_context_init(struct drm_device *dev)
+int i915_gem_context_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
 
-	if (!HAS_HW_CONTEXTS(dev)) {
-		dev_priv->hw_contexts_disabled = true;
-		DRM_DEBUG_DRIVER("Disabling HW Contexts; old hardware\n");
-		return;
-	}
+	if (!HAS_HW_CONTEXTS(dev))
+		return 0;
 
 	/* If called from reset, or thaw... we've been here already */
-	if (dev_priv->hw_contexts_disabled ||
-	    dev_priv->ring[RCS].default_context)
-		return;
+	if (dev_priv->ring[RCS].default_context)
+		return 0;
 
 	dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
 
 	if (dev_priv->hw_context_size > (1<<20)) {
-		dev_priv->hw_contexts_disabled = true;
 		DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
-		return;
+		return -E2BIG;
 	}
 
-	if (create_default_context(dev_priv)) {
-		dev_priv->hw_contexts_disabled = true;
-		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed\n");
-		return;
+	ret = create_default_context(dev_priv);
+	if (ret) {
+		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %d\n",
+				 ret);
+		return ret;
 	}
 
 	DRM_DEBUG_DRIVER("HW context support initialized\n");
+	return 0;
 }
 
 void i915_gem_context_fini(struct drm_device *dev)
@@ -281,7 +279,7 @@ void i915_gem_context_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
 
-	if (dev_priv->hw_contexts_disabled)
+	if (!HAS_HW_CONTEXTS(dev))
 		return;
 
 	/* The only known way to stop the gpu from accessing the hw context is
@@ -324,16 +322,16 @@ i915_gem_context_get_hang_stats(struct drm_device *dev,
 				struct drm_file *file,
 				u32 id)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct i915_hw_context *ctx;
 
 	if (id == DEFAULT_CONTEXT_ID)
 		return &file_priv->hang_stats;
 
-	ctx = NULL;
-	if (!dev_priv->hw_contexts_disabled)
-		ctx = i915_gem_context_get(file->driver_priv, id);
+	if (!HAS_HW_CONTEXTS(dev))
+		return ERR_PTR(-ENOENT);
+
+	ctx = i915_gem_context_get(file->driver_priv, id);
 	if (ctx == NULL)
 		return ERR_PTR(-ENOENT);
 
@@ -506,7 +504,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	struct i915_hw_context *to;
 
-	if (dev_priv->hw_contexts_disabled)
+	if (!HAS_HW_CONTEXTS(ring->dev))
 		return 0;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
@@ -531,7 +529,6 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *file)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_context_create *args = data;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct i915_hw_context *ctx;
@@ -540,7 +537,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	if (!(dev->driver->driver_features & DRIVER_GEM))
 		return -ENODEV;
 
-	if (dev_priv->hw_contexts_disabled)
+	if (!HAS_HW_CONTEXTS(dev))
 		return -ENODEV;
 
 	ret = i915_mutex_lock_interruptible(dev);
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index bb31ff3..47bc51b 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -183,13 +183,13 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
 	int slice = (int)(uintptr_t)attr->private;
 	int ret;
 
+	if (!HAS_HW_CONTEXTS(drm_dev))
+		return -ENXIO;
+
 	ret = l3_access_valid(drm_dev, offset);
 	if (ret)
 		return ret;
 
-	if (dev_priv->hw_contexts_disabled)
-		return -ENXIO;
-
 	ret = i915_mutex_lock_interruptible(drm_dev);
 	if (ret)
 		return ret;
-- 
1.8.4.1

Re: [PATCH] [v3] drm/i915: Require HW contexts (when possible)

[Ville Syrjälä]

On Thu, Oct 17, 2013 at 03:39:06PM -0700, Ben Widawsky wrote:
> v2: Fixed the botched locking on init_hw failure in i915_reset (Ville)
> Call cleanup_ringbuffer on failed context create in init_hw (Ville)
> 
> v3: Add dev argument ti clean_ringbuffer
> 
> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  3 ---
>  drivers/gpu/drm/i915/i915_drv.h         |  3 +--
>  drivers/gpu/drm/i915/i915_gem.c         |  8 +++++-
>  drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++++++++------------------
>  drivers/gpu/drm/i915/i915_sysfs.c       |  6 ++---
>  5 files changed, 31 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1060a96..839925b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -750,12 +750,9 @@ int i915_reset(struct drm_device *dev)
>  	 */
>  	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
>  			!dev_priv->ums.mm_suspended) {
> -		bool hw_contexts_disabled = dev_priv->hw_contexts_disabled;
>  		dev_priv->ums.mm_suspended = 0;
>  
>  		ret = i915_gem_init_hw(dev);
> -		if (!hw_contexts_disabled && dev_priv->hw_contexts_disabled)
> -			DRM_ERROR("HW contexts didn't survive reset\n");
>  		mutex_unlock(&dev->struct_mutex);
>  		if (ret) {
>  			DRM_ERROR("Failed hw init on reset %d\n", ret);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4ff8e9..21b2c37 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1411,7 +1411,6 @@ typedef struct drm_i915_private {
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
>  
> -	bool hw_contexts_disabled;
>  	uint32_t hw_context_size;
>  	struct list_head context_list;
>  
> @@ -2140,7 +2139,7 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
>  }
>  
>  /* i915_gem_context.c */
> -void i915_gem_context_init(struct drm_device *dev);
> +int __must_check i915_gem_context_init(struct drm_device *dev);
>  void i915_gem_context_fini(struct drm_device *dev);
>  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
>  int i915_switch_context(struct intel_ring_buffer *ring,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 34df59b..b43cd30 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4463,7 +4463,13 @@ i915_gem_init_hw(struct drm_device *dev)
>  	 * XXX: There was some w/a described somewhere suggesting loading
>  	 * contexts before PPGTT.
>  	 */
> -	i915_gem_context_init(dev);
> +	ret = i915_gem_context_init(dev);
> +	if (ret) {
> +		i915_gem_cleanup_ringbuffer(dev);
> +		DRM_ERROR("Context initialization failed %d\n", ret);
> +		return ret;
> +	}
> +
>  	if (dev_priv->mm.aliasing_ppgtt) {
>  		ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
>  		if (ret) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index cc619c1..4625670 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -244,36 +244,34 @@ err_destroy:
>  	return ret;
>  }
>  
> -void i915_gem_context_init(struct drm_device *dev)
> +int i915_gem_context_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
>  
> -	if (!HAS_HW_CONTEXTS(dev)) {
> -		dev_priv->hw_contexts_disabled = true;
> -		DRM_DEBUG_DRIVER("Disabling HW Contexts; old hardware\n");
> -		return;
> -	}
> +	if (!HAS_HW_CONTEXTS(dev))
> +		return 0;
>  
>  	/* If called from reset, or thaw... we've been here already */
> -	if (dev_priv->hw_contexts_disabled ||
> -	    dev_priv->ring[RCS].default_context)
> -		return;
> +	if (dev_priv->ring[RCS].default_context)
> +		return 0;
>  
>  	dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
>  
>  	if (dev_priv->hw_context_size > (1<<20)) {
> -		dev_priv->hw_contexts_disabled = true;
>  		DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
> -		return;
> +		return -E2BIG;
>  	}
>  
> -	if (create_default_context(dev_priv)) {
> -		dev_priv->hw_contexts_disabled = true;
> -		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed\n");
> -		return;
> +	ret = create_default_context(dev_priv);
> +	if (ret) {
> +		DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %d\n",
> +				 ret);
> +		return ret;
>  	}
>  
>  	DRM_DEBUG_DRIVER("HW context support initialized\n");
> +	return 0;
>  }
>  
>  void i915_gem_context_fini(struct drm_device *dev)
> @@ -281,7 +279,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
>  
> -	if (dev_priv->hw_contexts_disabled)
> +	if (!HAS_HW_CONTEXTS(dev))
>  		return;
>  
>  	/* The only known way to stop the gpu from accessing the hw context is
> @@ -324,16 +322,16 @@ i915_gem_context_get_hang_stats(struct drm_device *dev,
>  				struct drm_file *file,
>  				u32 id)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  	struct i915_hw_context *ctx;
>  
>  	if (id == DEFAULT_CONTEXT_ID)
>  		return &file_priv->hang_stats;
>  
> -	ctx = NULL;
> -	if (!dev_priv->hw_contexts_disabled)
> -		ctx = i915_gem_context_get(file->driver_priv, id);
> +	if (!HAS_HW_CONTEXTS(dev))
> +		return ERR_PTR(-ENOENT);
> +
> +	ctx = i915_gem_context_get(file->driver_priv, id);
>  	if (ctx == NULL)
>  		return ERR_PTR(-ENOENT);
>  
> @@ -506,7 +504,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  	struct i915_hw_context *to;
>  
> -	if (dev_priv->hw_contexts_disabled)
> +	if (!HAS_HW_CONTEXTS(ring->dev))
>  		return 0;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> @@ -531,7 +529,6 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  				  struct drm_file *file)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_context_create *args = data;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  	struct i915_hw_context *ctx;
> @@ -540,7 +537,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  	if (!(dev->driver->driver_features & DRIVER_GEM))
>  		return -ENODEV;
>  
> -	if (dev_priv->hw_contexts_disabled)
> +	if (!HAS_HW_CONTEXTS(dev))
>  		return -ENODEV;
>  
>  	ret = i915_mutex_lock_interruptible(dev);
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index bb31ff3..47bc51b 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -183,13 +183,13 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
>  	int slice = (int)(uintptr_t)attr->private;
>  	int ret;
>  
> +	if (!HAS_HW_CONTEXTS(drm_dev))
> +		return -ENXIO;
> +
>  	ret = l3_access_valid(drm_dev, offset);
>  	if (ret)
>  		return ret;
>  
> -	if (dev_priv->hw_contexts_disabled)
> -		return -ENXIO;
> -
>  	ret = i915_mutex_lock_interruptible(drm_dev);
>  	if (ret)
>  		return ret;
> -- 
> 1.8.4.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC