Re: [PATCH v3 1/1] drm/i915: Update GEN6_PMINTRMSK setup with GuC enabled

["Kamble, Sagar A" <sagar.a.kamble@intel.com>]

On 5/31/2016 1:48 AM, Chris Wilson wrote:
> On Tue, May 31, 2016 at 12:16:11AM +0530, Sagar Arun Kamble wrote:
>>   void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>> @@ -4580,6 +4568,28 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>>   	else
>>   		dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
>>   
>> +	dev_priv->rps.pm_intr_keep = 0;
>> +
>> +	/*
>> +	 * SNB,IVB can while VLV,CHV may hard hang on looping batchbuffer
>> +	 * if GEN6_PM_UP_EI_EXPIRED is masked.
>> +	 *
>> +	 * TODO: verify if this can be reproduced on VLV,CHV.
>> +	 */
>> +	if (INTEL_INFO(dev_priv)->gen <= 7 && !IS_HASWELL(dev_priv))
>> +		dev_priv->rps.pm_intr_keep |= GEN6_PM_RP_UP_EI_EXPIRED;
>> +
>> +	/*
>> +	 * If GuC submission is enabled keep PM interrupts routed to GuC
>> +	 * and unmask ARAT Expired interrupt as it is needed by GuC.
>> +	*/
>> +	if (INTEL_INFO(dev_priv)->gen >= 8) {
>> +		if (i915.enable_guc_submission)
>> +			dev_priv->rps.pm_intr_keep |= GEN8_ARAT_EXPIRED_INT_MASK;
>> +		else
>> +			dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_NON_DISP;
> The logic doesn't match the comment. Don't you want
>
> if (i915.enable_guc_submission) {
> 	dev_priv->rps.pm_intr_keep |= GEN8_ARAT_EXPIRED_INT_MASK;
> 	dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_NON_DISP;
> }
We don't want to keep GEN8_PMINTR_REDIRECT_TO_NON_DISP if GuC submission 
is used.
I added WARN_ON for bits of GEN6_PMINTRMASK if GuC has set expected.
With GuC loaded bit 31 will be set and bit 9 is reset. GuC might reset 
other bits if it needs
those interrupts. I am extending the logic to other interrupts to make 
it extensible.
Certainly this patch is looking more neat now :) ... Thank you.
>
> as implied by the comment? Although I'm guessing the
> i915.enable_guc_submission will be refined in future, it should do for
> now.
>
>> +		WARN_ON(!(I915_READ(GEN6_PMINTRMSK) |
>> +				GEN8_PMINTR_REDIRECT_TO_NON_DISP));
> Always false;
>
>> +		WARN_ON(I915_READ(GEN6_PMINTRMSK) |
>> +				GEN8_ARAT_EXPIRED_INT_MASK);
> Always true.
>
> I don't this was quite what you meant :)
>
> But the patch is a lot neater now!
> -Chris
>

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