Re: [PATCH 1/7] drm/i915: Enable edp psr error interrupts on hsw

["Jindal, Sonika" <sonika.jindal@intel.com>]

On 5/18/2016 11:56 PM, Ville Syrjälä wrote:
> On Wed, May 18, 2016 at 06:47:10PM +0200, Daniel Vetter wrote:
>> The definitions for the error register should be valid on bdw/skl too,
>> but there we haven't even enabled DE_MISC handling yet.
>>
>> Somewhat confusing the the moved register offset on bdw is only for
>> the _CTL/_AUX register, and that _IIR/IMR stayed where they have been
>> on bdw.
>>
>> v2: Fixes from Ville.
>>
>> v3: Drop the REG_WRITE masking since edp interrupt still work and we
>> still enter PSR.
>>
>> Cc: "Runyan, Arthur J" <arthur.j.runyan@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Sonika Jindal <sonika.jindal@intel.com>
>> Cc: Durgadoss R <durgadoss.r@intel.com>
>> Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> I guess we might enable the error interrupt unconditionally, but the
> entry/exit interrupts are going to cause constant dmesg spam, so those I
> don't think we want enabled normally. Might be interesting to have some
> kind of debug knob for those though.
>
> Hmm. Actually is the PSR error interrupt tied to the "error interrupt"
> bit in the PSR AUX register? We don't enable that bit, so maybe we can't
> actually get the error interrupt?
Only bit 2 (SRD Aux Error) must be tied to the error interrupt bit of 
SRD_AUX register.
The entry and exit interrupts should be independent as per my understanding.
I agree the entry/exit messages will be too much in dmesg.
>> ---
>>   drivers/gpu/drm/i915/i915_irq.c | 35 +++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_reg.h | 10 ++++++++++
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index f0d941455bed..579f582ef987 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -2185,6 +2185,26 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
>>   		ironlake_rps_change_irq_handler(dev_priv);
>>   }
>>   
>> +static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
>> +{
>> +	u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
>> +
>> +	if (edp_psr_iir & EDP_PSR_ERROR)
>> +		DRM_DEBUG_KMS("PSR error\n");
>> +
>> +	if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
>> +		DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
>> +		I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
>> +	}
>> +
>> +	if (edp_psr_iir & EDP_PSR_POST_EXIT) {
>> +		DRM_DEBUG_KMS("PSR exit completed\n");
>> +		I915_WRITE(EDP_PSR_IMR, 0);
>> +	}
>> +
>> +	I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
>> +}
>> +
>>   static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
>>   				    u32 de_iir)
>>   {
>> @@ -2197,6 +2217,9 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
>>   	if (de_iir & DE_ERR_INT_IVB)
>>   		ivb_err_int_handler(dev_priv);
>>   
>> +	if (de_iir & DE_EDP_PSR_INT_HSW)
>> +		hsw_edp_psr_irq_handler(dev_priv);
>> +
>>   	if (de_iir & DE_AUX_CHANNEL_A_IVB)
>>   		dp_aux_irq_handler(dev_priv);
>>   
>> @@ -3381,6 +3404,11 @@ static void ironlake_irq_reset(struct drm_device *dev)
>>   	if (IS_GEN7(dev))
>>   		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
>>   
>> +	if (IS_HASWELL(dev)) {
>> +		I915_WRITE(EDP_PSR_IMR, 0xffffffff);
>> +		I915_WRITE(EDP_PSR_IIR, 0xffffffff);
>> +	}
>> +
>>   	gen5_gt_irq_reset(dev);
>>   
>>   	ibx_irq_reset(dev);
>> @@ -3676,6 +3704,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>>   				DE_PCH_EVENT_IVB | DE_PLANEC_FLIP_DONE_IVB |
>>   				DE_PLANEB_FLIP_DONE_IVB |
>>   				DE_PLANEA_FLIP_DONE_IVB | DE_AUX_CHANNEL_A_IVB);
>> +
extra line.
>>   		extra_mask = (DE_PIPEC_VBLANK_IVB | DE_PIPEB_VBLANK_IVB |
>>   			      DE_PIPEA_VBLANK_IVB | DE_ERR_INT_IVB |
>>   			      DE_DP_A_HOTPLUG_IVB);
>> @@ -3690,6 +3719,12 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>>   			      DE_DP_A_HOTPLUG);
>>   	}
>>   
>> +	if (IS_HASWELL(dev)) {
>> +		gen5_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
>> +		I915_WRITE(EDP_PSR_IMR, 0);
>> +		display_mask |= DE_EDP_PSR_INT_HSW;
>> +	}
>> +
>>   	dev_priv->irq_mask = ~display_mask;
>>   
>>   	I915_WRITE(HWSTAM, 0xeffe);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 86fbf723eca7..0f99e67f2114 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3225,6 +3225,13 @@ enum skl_disp_power_wells {
>>   #define   EDP_PSR_TP1_TIME_0us			(3<<4)
>>   #define   EDP_PSR_IDLE_FRAME_SHIFT		0
>>   
>> +/* Bspec claims those aren't shifted but stay at 0x64800 */
>> +#define EDP_PSR_IMR				_MMIO(0x64834)
>> +#define EDP_PSR_IIR				_MMIO(0x64838)
>> +#define   EDP_PSR_ERROR				(1<<2)
>> +#define   EDP_PSR_POST_EXIT			(1<<1)
>> +#define   EDP_PSR_PRE_ENTRY			(1<<0)
>> +
>>   #define EDP_PSR_AUX_CTL				_MMIO(dev_priv->psr_mmio_base + 0x10)
>>   #define EDP_PSR_AUX_DATA(i)			_MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
>>   
>> @@ -3259,6 +3266,7 @@ enum skl_disp_power_wells {
>>   #define   EDP_PSR_DEBUG_MASK_LPSP	(1<<27)
>>   #define   EDP_PSR_DEBUG_MASK_MEMUP	(1<<26)
>>   #define   EDP_PSR_DEBUG_MASK_HPD	(1<<25)
>> +#define   EDP_PSR_DEBUG_MASK_REG_WRITE	(1<<16)
Is this remaining from some previous version? or left intentionally?
>>   
>>   #define EDP_PSR2_CTL			_MMIO(0x6f900)
>>   #define   EDP_PSR2_ENABLE		(1<<31)
>> @@ -5886,6 +5894,7 @@ enum skl_disp_power_wells {
>>   #define DE_PCH_EVENT_IVB		(1<<28)
>>   #define DE_DP_A_HOTPLUG_IVB		(1<<27)
>>   #define DE_AUX_CHANNEL_A_IVB		(1<<26)
>> +#define DE_EDP_PSR_INT_HSW		(1<<19)
>>   #define DE_SPRITEC_FLIP_DONE_IVB	(1<<14)
>>   #define DE_PLANEC_FLIP_DONE_IVB		(1<<13)
>>   #define DE_PIPEC_VBLANK_IVB		(1<<10)
>> @@ -5998,6 +6007,7 @@ enum skl_disp_power_wells {
>>   #define GEN8_DE_MISC_IIR _MMIO(0x44468)
>>   #define GEN8_DE_MISC_IER _MMIO(0x4446c)
>>   #define  GEN8_DE_MISC_GSE		(1 << 27)
>> +#define  GEN8_DE_EDP_PSR		(1 << 19)
We are not enabling it for bdw now right?
>>   
>>   #define GEN8_PCU_ISR _MMIO(0x444e0)
>>   #define GEN8_PCU_IMR _MMIO(0x444e4)
>> -- 
>> 2.8.1

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