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

[Paulo Zanoni]

2014-02-21 14:27 GMT-03:00 Chris Wilson <chris at chris-wilson.co.uk>:
> On Fri, Feb 21, 2014 at 02:04:32PM -0300, Paulo Zanoni wrote:
>> 2014-02-21 13:55 GMT-03:00 Chris Wilson <chris at chris-wilson.co.uk>:
>> > On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote:
>> >> From: Chris Wilson <chris at chris-wilson.co.uk>
>> >>
>> >> 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.
>> >>
>> >> v2: From Paulo
>> >>   - Make it compile
>> >>   - Drop the __i915_add_request chunk
>> >>
>> >> Reported-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >> Tested-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>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/i915_drv.h      | 8 ++++++++
>> >>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>> >>  2 files changed, 17 insertions(+)
>> >>
>> >>
>> >> Chris did not reply to my review comments yet, so I just went and implemented
>> >> them. We need at least an ACK form him here before merging.
>> >
>> > Didn't see them... Why have you altered the logic?
>>
>> See the comment at the __i915_add_request chunk:
>>
>> http://lists.freedesktop.org/archives/intel-gfx/2014-February/040334.html
>
> Oh, I didn't look for comments inline.
>>
>> Maybe I just broke your patch :)
>> If my review doesn't make sense, we can stick to your version, it
>> should do the job, and I can retest everything easily.
>
> If there was a pending work item, the call to intel_mark_busy() would
> return false. So we can revamp the logic around there a little bit. The
> reason for the change should be self-evident - the previous code lost its
> way in the transition to multiple rings arguing over a global property

Just to avoid any possible confusions when/if we merge this series:
Chris sent a new version of this patch on the original mail thread.

Thanks,
Paulo
.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni