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

Rodrigo Vivi
2013-11-29T13:03:41Z

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