[PATCH 3/4] drm/i915/guc: Update rps.intr_keep_unmasked in guc_interrupts_capture/release

[Kamble, Sagar A]

Thanks for the inputs Chris.

On 3/10/2017 11:00 PM, Chris Wilson wrote:
> On Fri, Mar 10, 2017 at 10:22:27PM +0530, Sagar Arun Kamble wrote:
>> Different state is to be maintained for rps.intr_keep_unmasked for GuC and
>> Execlists. Updating it inside guc_interrupts_* routines as in those
>> routines GuC load/submission params are sanitized and it should not be set
>> based on HAS_GUC_SCHED during intel_irq_init.
>>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg at intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 29 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_irq.c            | 24 ------------------------
>>   2 files changed, 29 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 2282d83..314880f 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -954,6 +954,30 @@ static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>>   	I915_WRITE(GUC_BCS_RCS_IER, ~irqs);
>>   	I915_WRITE(GUC_VCS2_VCS1_IER, ~irqs);
>>   	I915_WRITE(GUC_WD_VECS_IER, ~irqs);
>> +
>> +	/*
>> +	 * The REDIRECT_TO_GUC bit of the PMINTRMSK register directs all
>> +	 * (unmasked) PM interrupts to the GuC. All other bits of this
>> +	 * register *disable* generation of a specific interrupt.
>> +	 *
>> +	 * 'intr_keep_unmasked' indicates bits that are NOT to be set when
>> +	 * writing to the PM interrupt mask register, i.e. interrupts
>> +	 * that must not be disabled.
>> +	 *
>> +	 * If the GuC is handling these interrupts, then we must not let
>> +	 * the PM code disable ANY interrupt that the GuC is expecting.
>> +	 * So for each ENABLED (0) bit in this register, we must SET the
>> +	 * bit in intr_keep_unmasked so that it's left enabled for the GuC.
>> +	 * GuC needs ARAT expired interrupt unmasked hence it is set in
>> +	 * intr_keep_unmasked.
>> +	 *
>> +	 * Here we CLEAR REDIRECT_TO_GUC bit in intr_keep_unmasked, which will
>> +	 * result in the register bit being left SET!
>> +	 */
>> +	if (HAS_GUC_SCHED(dev_priv)) {
> Cen we get here without HAS_GUC_SCHED() ?
Checked now. can't. will fix this.
>
>> +		dev_priv->rps.intr_keep_unmasked |= ARAT_EXPIRED_INTRMSK;
>> +		dev_priv->rps.intr_keep_unmasked &= ~GEN8_PMINTR_REDIRECT_TO_GUC;
>> +	}
>>   }
>>   
>>   int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>> @@ -1011,6 +1035,11 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
>>   	I915_WRITE(GUC_BCS_RCS_IER, 0);
>>   	I915_WRITE(GUC_VCS2_VCS1_IER, 0);
>>   	I915_WRITE(GUC_WD_VECS_IER, 0);
>> +
>> +	dev_priv->rps.intr_keep_unmasked = 0;
>> +
>> +	if (INTEL_INFO(dev_priv)->gen >= 8)
>> +		dev_priv->rps.intr_keep_unmasked |= GEN8_PMINTR_REDIRECT_TO_GUC;
> Here we can safely say that should be unconditional.
>
> I think symmetry would be better, i.e.
> dev_priv->rps.intr_keep_unmasked |= GEN8_PMINTR_REDIRECT_TO_GUC;
> dev_priv->rps.intr_keep_unmasked &= ~ARAT_EXPIRED_INTRMSK;
>
> Looks ok to me here. We did at one point talk about trying to keep the
> rps together, but adding entry points just to set/clear these bits seems
> overkill right now.
> -Chris
Yes. symmetry makes sense. Will update. Thank you.
>