[Intel-gfx] [PATCH v3 1/4] drm/i915: Enable edp psr error interrupts on hsw

[Souza, Jose]

On Tue, 2018-04-03 at 14:24 -0700, Dhinakaran Pandiyan wrote:
> From: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> 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: From DK
>  * Rebased on drm-tip
>  * Removed BDW IIR bit definition, looks like an unintentional change
> that
> should be in the following patch.
> 
> v4: From DK
>  * Don't mask REG_WRITE.
> 
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Daniel Vetter <daniel.vetter at intel.com>

With bspec id and comment about why are you masking interruptions in
hsw_edp_psr_irq_handler() feel free to add:

Reviewed-by: Jose Roberto de Souza <jose.souza at intel.com>

> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 34
> ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h |  8 ++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index 27aee25429b7..c2d3f30778ee 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2391,6 +2391,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);

Just to know... you need to mask this one otherwise it will keep
triggering EDP_PSR_PRE_ENTRY interruptions? Would be nice to have a
comment explaning why you are masking it.

> +	}
> +
> +	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)
>  {
> @@ -2403,6 +2423,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);
>  
> @@ -3260,6 +3283,11 @@ static void ironlake_irq_reset(struct
> drm_device *dev)
>  	if (IS_GEN7(dev_priv))
>  		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
>  
> +	if (IS_HASWELL(dev_priv)) {
> +		I915_WRITE(EDP_PSR_IMR, 0xffffffff);
> +		I915_WRITE(EDP_PSR_IIR, 0xffffffff);

No need to do a POSTING_READ(EDP_PSR_IMR);? Like is done in:
GEN3_IRQ_RESET()

> +	}
> +
>  	gen5_gt_irq_reset(dev_priv);
>  
>  	ibx_irq_reset(dev_priv);
> @@ -3671,6 +3699,12 @@ static int ironlake_irq_postinstall(struct
> drm_device *dev)
>  			      DE_DP_A_HOTPLUG);
>  	}
>  
> +	if (IS_HASWELL(dev_priv)) {
> +		gen3_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;
>  
>  	ibx_irq_pre_postinstall(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 176dca6554f4..f5783d6db614 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4011,6 +4011,13 @@ enum {
>  #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)

Could you add the bspec id of where did you got this?
The hsw spec that I'm reading don't have the bits, skl have but don't
have the bits of the other transcoders.

> +
>  #define EDP_PSR_AUX_CTL				_MMIO(dev_pri
> v->psr_mmio_base + 0x10)
>  #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
>  #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
> @@ -6820,6 +6827,7 @@ enum {
>  #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)