Re: [PATCH 07/19] drm/i915: add runtime put/get calls at the basic places

[Rodrigo Vivi <rodrigo.vivi@gmail.com>]

On Wed, Nov 27, 2013 at 06:20:34PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If I add code to enable runtime PM on my Haswell machine, start a
> desktop environment, then enable runtime PM, these functions will
> complain that they're trying to read/write registers while the
> graphics card is suspended.
> 
> v2: - Simplify i915_gem_fault changes.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c            | 27 +++++++++++++++++++++------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  6 ++++++
>  drivers/gpu/drm/i915/i915_irq.c            |  6 ++++++
>  3 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 40d9dcf..430b875 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1380,6 +1380,8 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  	int ret = 0;
>  	bool write = !!(vmf->flags & FAULT_FLAG_WRITE);
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	/* We don't use vmf->pgoff since that has the fake offset */
>  	page_offset = ((unsigned long)vmf->virtual_address - vma->vm_start) >>
>  		PAGE_SHIFT;
> @@ -1427,8 +1429,10 @@ out:
>  		/* If this -EIO is due to a gpu hang, give the reset code a
>  		 * chance to clean up the mess. Otherwise return the proper
>  		 * SIGBUS. */
> -		if (i915_terminally_wedged(&dev_priv->gpu_error))
> -			return VM_FAULT_SIGBUS;
> +		if (i915_terminally_wedged(&dev_priv->gpu_error)) {
> +			ret = VM_FAULT_SIGBUS;
> +			break;
> +		}
>  	case -EAGAIN:
>  		/*
>  		 * EAGAIN means the gpu is hung and we'll wait for the error
> @@ -1443,15 +1447,22 @@ out:
>  		 * EBUSY is ok: this just means that another thread
>  		 * already did the job.
>  		 */
> -		return VM_FAULT_NOPAGE;
> +		ret = VM_FAULT_NOPAGE;
> +		break;
>  	case -ENOMEM:
> -		return VM_FAULT_OOM;
> +		ret = VM_FAULT_OOM;
> +		break;
>  	case -ENOSPC:
> -		return VM_FAULT_SIGBUS;
> +		ret = VM_FAULT_SIGBUS;
> +		break;
>  	default:
>  		WARN_ONCE(ret, "unhandled error in i915_gem_fault: %i\n", ret);
> -		return VM_FAULT_SIGBUS;
> +		ret = VM_FAULT_SIGBUS;
> +		break;
>  	}
> +
> +	intel_runtime_pm_put(dev_priv);
> +	return ret;
>  }
>  
>  /**
> @@ -4165,6 +4176,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct i915_vma *vma, *next;
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	trace_i915_gem_object_destroy(obj);
>  
>  	if (obj->phys_obj)
> @@ -4209,6 +4222,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  
>  	kfree(obj->bit_17);
>  	i915_gem_object_free(obj);
> +
> +	intel_runtime_pm_put(dev_priv);
>  }
>  
>  struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 3c90dd1..260c147 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1095,6 +1095,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		}
>  	}
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	ret = i915_mutex_lock_interruptible(dev);
>  	if (ret)
>  		goto pre_mutex_err;
> @@ -1229,6 +1231,10 @@ err:
>  
>  pre_mutex_err:
>  	kfree(cliprects);
> +
> +	/* intel_gpu_busy should also get a ref, so it will free when the device
> +	 * is really idle. */
> +	intel_runtime_pm_put(dev_priv);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2715600..70c4cef 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2477,6 +2477,12 @@ static void i915_hangcheck_elapsed(unsigned long data)
>  	if (!i915_enable_hangcheck)
>  		return;
>  
> +	/* Just postpone in case we're completely idle... */
> +	if (dev_priv->pm.suspended) {
> +		i915_queue_hangcheck(dev);
> +		return;
> +	}

I got the idea here, just not sure if it fits on this patch or it is just the commit message.

Anyway,
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

> +
>  	for_each_ring(ring, dev_priv, i) {
>  		u32 seqno, acthd;
>  		bool busy = true;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx