diff mbox

drm/i915: Fix to Enable GT/PM Interrupts for cherryview.

Message ID 1408608429-27991-1-git-send-email-deepak.s@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

deepak.s@linux.intel.com Aug. 21, 2014, 8:07 a.m. UTC
From: Deepak S <deepak.s@linux.intel.com>

Programing GT IER interrupts was fumbled while enabling Interrupts for
gen8

This is a regression from
    commit abd58f0175915bed644aa67c8f69dc571b8280e0
    Author: Ben Widawsky <benjamin.widawsky@intel.com>
    Date:   Sat Nov 2 21:07:09 2013 -0700

	drm/i915/bdw: Implement interrupt changes

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ville Syrjälä Aug. 20, 2014, 10:56 a.m. UTC | #1
On Thu, Aug 21, 2014 at 01:37:09PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> Programing GT IER interrupts was fumbled while enabling Interrupts for
> gen8

True, but...

> 
> This is a regression from
>     commit abd58f0175915bed644aa67c8f69dc571b8280e0
>     Author: Ben Widawsky <benjamin.widawsky@intel.com>
>     Date:   Sat Nov 2 21:07:09 2013 -0700
> 
> 	drm/i915/bdw: Implement interrupt changes
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d5445e7..48c02bc 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3812,7 +3812,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>  			GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS1_IRQ_SHIFT |
>  			GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT |
>  			GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS2_IRQ_SHIFT,
> -		0,
> +		dev_priv->pm_rps_events,

.. this would now unmask the rps interrupts already in postinstall which
isn't quite what we want. I think the best solution might be to just
kill the loop below and just init each GT interrupt register set indivually
so that you can pass the correct mask to GT_IMR(2). So I'm thinking simply
something like this:

dev_priv->pm_irq_mask = 0xffffffff;
GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]);
GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_irq_mask, dev_priv->pm_rps_events);
GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);

>  		GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT |
>  			GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT
>  		};
> -- 
> 1.9.1
deepak.s@linux.intel.com Aug. 22, 2014, 2:13 a.m. UTC | #2
On Wednesday 20 August 2014 04:26 PM, Ville Syrjälä wrote:
> On Thu, Aug 21, 2014 at 01:37:09PM +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@linux.intel.com>
>>
>> Programing GT IER interrupts was fumbled while enabling Interrupts for
>> gen8
> True, but...
>
>> This is a regression from
>>      commit abd58f0175915bed644aa67c8f69dc571b8280e0
>>      Author: Ben Widawsky <benjamin.widawsky@intel.com>
>>      Date:   Sat Nov 2 21:07:09 2013 -0700
>>
>> 	drm/i915/bdw: Implement interrupt changes
>>
>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_irq.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index d5445e7..48c02bc 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3812,7 +3812,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>>   			GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS1_IRQ_SHIFT |
>>   			GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT |
>>   			GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS2_IRQ_SHIFT,
>> -		0,
>> +		dev_priv->pm_rps_events,
> .. this would now unmask the rps interrupts already in postinstall which
> isn't quite what we want. I think the best solution might be to just
> kill the loop below and just init each GT interrupt register set indivually
> so that you can pass the correct mask to GT_IMR(2). So I'm thinking simply
> something like this:
>
> dev_priv->pm_irq_mask = 0xffffffff;
> GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]);
> GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
> GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_irq_mask, dev_priv->pm_rps_events);
> GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);

Thanks for the review. I will address this.

>>   		GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT |
>>   			GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT
>>   		};
>> -- 
>> 1.9.1
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d5445e7..48c02bc 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3812,7 +3812,7 @@  static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 			GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS1_IRQ_SHIFT |
 			GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT |
 			GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS2_IRQ_SHIFT,
-		0,
+		dev_priv->pm_rps_events,
 		GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT |
 			GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT
 		};