Re: [PATCH] drm/i915/execlists: Document runtime pm for intel_lrc_irq_handler()

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 12/04/2017 09:31, Chris Wilson wrote:
> On Wed, Apr 12, 2017 at 08:12:17AM +0100, Tvrtko Ursulin wrote:
>>
>> On 11/04/2017 18:58, Chris Wilson wrote:
>>> We indirectly hold the runtime-pm for the intel_lrc_irq_handler() by
>>> virtue of dev_priv->gt.awake keeping a wakeref whilst the requests are
>>> busy. As this is not obvious from the code, add a comment.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_lrc.c | 9 +++++++++
>>> 1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 0dc1cc4ad6e7..e16cc28dc783 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -515,6 +515,15 @@ static void intel_lrc_irq_handler(unsigned long data)
>>> 	struct execlist_port *port = engine->execlist_port;
>>> 	struct drm_i915_private *dev_priv = engine->i915;
>>>
>>> +	/* We can skip acquiring intel_runtime_pm_get() here as it was taken
>>> +	 * on our behalf by the request (see i915_gem_mark_busy ()) and it will
>>> +	 * not be relinquished until the device is idle (see
>>> +	 * i915_gem_idle_work_handler()). As a precaution, we make sure
>>> +	 * that all ELSP are drained i.e. we have processed the CSB,
>>> +	 * before allowing ourselves to idle and calling intel_runtime_pm_put().
>>> +	 */
>>> +	GEM_BUG_ON(!dev_priv->gt.awake);
>>> +
>>> 	intel_uncore_forcewake_get(dev_priv, engine->fw_domains);
>>>
>>> 	/* Prefer doing test_and_clear_bit() as a two stage operation to avoid
>>>
>>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> It feels a big to c&p, but not sure if it worth putting a comment
>> "see comment in intel_lrc_irq_handler on why we don't have to " to
>> i915_guc_irq_handler or maybe dequeue?
>
> For guc, the requirement for rpm is much less strict - the interface
> with the guc is through ordinary memory not device memory, so we don't
> need rpm_get (aiui). There is the mmio readback for !llc to ensure that
> all GTT writes are flushed to system memory prior to dispatching the
> workqueue -- that technically doesn't require rpm either.
>
> I'm a bit more hesistant about adding such a comment to the guc atm, as
> I think I will be more misleading then helpful.

I am not sure but was thinking that presumably device needs to be awake 
as we are adding stuff into the GuC's workqueue.

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx