[Intel-gfx] [PATCH] drm/i915: Accurately track when we mark the hardware as idle/busy

[Paulo Zanoni]

2014-02-18 16:25 GMT-03:00 Mika Kuoppala <mika.kuoppala at linux.intel.com>:
> Chris Wilson <chris at chris-wilson.co.uk> writes:
>
>> We currently call intel_mark_idle() too often, as we do so as a
>> side-effect of processing the request queue. However, we the calls to
>> intel_mark_idle() are expected to be paired with a call to
>> intel_mark_busy() (or else we try to idle the hardware by accessing
>> registers that are already disabled). Make the idle/busy tracking
>> explicit to prevent the multiple calls.
>>
>> Reported-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>

Thanks!

I tested it and this patch + another local patch I have fix the
problem that can be reproduced by the "gem-idle" subtest of pm_pc8.c
(I still did not commit the subtest, but will do it soon).

Also, I guess this patch deprecates dev_priv->pc8.gpu_idle. I had
plans to move it to dev_priv->pm.gpu_idle, but now I'll try to kill
it.

>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |    8 ++++++++
>>  drivers/gpu/drm/i915/i915_gem.c      |    4 +---
>>  drivers/gpu/drm/i915/intel_display.c |   11 +++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
>>  4 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 00222cc..8441c8a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1134,6 +1134,14 @@ struct i915_gem_mm {
>>        */
>>       bool interruptible;
>>
>> +     /**
>> +      * Is the GPU currently considered idle, or busy executing userspace
>> +      * requests?  Whilst idle, we attempt to power down the hardware and
>> +      * display clocks. In order to reduce the effect on performance, there
>> +      * is a slight delay before we do so.
>> +      */
>> +     bool busy;
>> +
>>       /** Bit 6 swizzling required for X tiling */
>>       uint32_t bit_6_swizzle_x;
>>       /** Bit 6 swizzling required for Y tiling */
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 9a40ef5..4525dd7 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2315,7 +2315,6 @@ int __i915_add_request(struct intel_ring_buffer *ring,
>>               i915_gem_context_reference(request->ctx);
>>
>>       request->emitted_jiffies = jiffies;
>> -     was_empty = list_empty(&ring->request_list);
>>       list_add_tail(&request->list, &ring->request_list);
>>       request->file_priv = NULL;
>>
>> @@ -2336,12 +2335,11 @@ int __i915_add_request(struct intel_ring_buffer *ring,
>>       if (!dev_priv->ums.mm_suspended) {
>>               i915_queue_hangcheck(ring->dev);
>>
>> -             if (was_empty) {
>> +             if (intel_mark_busy(dev_priv->dev)) {

I'm new to this code, so forgive me if I'm way off. Now that we
changed the relative order, isn't it possible that we run the code
line above, marking the device as busy, and then just before the next
line runs, the still-not-canceled idle_work function runs and marks
the device as idle? That could be bad, right?

Also, why do we need the change on this function?


>>                       cancel_delayed_work_sync(&dev_priv->mm.idle_work);
>>                       queue_delayed_work(dev_priv->wq,
>>                                          &dev_priv->mm.retire_work,
>>                                          round_jiffies_up_relative(HZ));
>> -                     intel_mark_busy(dev_priv->dev);
>>               }
>>       }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e127b23..bfd6396 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -8220,8 +8220,14 @@ void intel_mark_busy(
>
> bool intel_mark_busy(struct drm_device *dev)
>
> -Mika

Exactly.

Thanks,
Paulo

>
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> +     if (dev_priv->mm.busy)
>> +             return false;
>> +
>>       hsw_package_c8_gpu_busy(dev_priv);
>>       i915_update_gfx_val(dev_priv);
>> +     dev_priv->mm.busy = true;
>> +
>> +     return true;
>>  }
>>
>>  void intel_mark_idle(struct drm_device *dev)
>> @@ -8229,6 +8235,11 @@ void intel_mark_idle(struct drm_device *dev)
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       struct drm_crtc *crtc;
>>
>> +     if (!dev_priv->mm.busy)
>> +             return;
>> +
>> +     dev_priv->mm.busy = false;
>> +
>>       hsw_package_c8_gpu_idle(dev_priv);
>>
>>       if (!i915.powersave)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index e5e1a5c..4c329e0 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -656,7 +656,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>>  const char *intel_output_name(int output);
>>  bool intel_has_pending_fb_unpin(struct drm_device *dev);
>>  int intel_pch_rawclk(struct drm_device *dev);
>> -void intel_mark_busy(struct drm_device *dev);
>> +bool intel_mark_busy(struct drm_device *dev);
>>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>>                       struct intel_ring_buffer *ring);
>>  void intel_mark_idle(struct drm_device *dev);
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni