All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>,
	Ben Widawsky <benjamin.widawsky@intel.com>
Subject: Re: [RFC] [PATCH] drm/i915: Require HW contexts (when possible)
Date: Thu, 17 Oct 2013 20:52:17 +0300	[thread overview]
Message-ID: <20131017175217.GI13047@intel.com> (raw)
In-Reply-To: <1382029183-24410-1-git-send-email-ben@bwidawsk.net>

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

  parent reply	other threads:[~2013-10-17 17:52 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-17 16:59 [RFC] [PATCH] drm/i915: Require HW contexts (when possible) Ben Widawsky
2013-10-17 17:19 ` Kenneth Graunke
2013-10-17 17:52 ` Ville Syrjälä [this message]
2013-10-17 18:01   ` Ben Widawsky
2013-10-17 21:49   ` [PATCH] [v2] " Ben Widawsky
2013-10-17 22:11     ` Ben Widawsky
2013-10-17 22:39     ` [PATCH] [v3] " Ben Widawsky
2013-10-18  7:39       ` Ville Syrjälä

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131017175217.GI13047@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=ben@bwidawsk.net \
    --cc=benjamin.widawsky@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.