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

Rodrigo Vivi
2013-11-29T13:32:35Z

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>

(resent with correct smtp)

On Fri, Nov 29, 2013 at 11:03 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br