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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Qui, 2018-04-05 às 15:00 -0700, Dhinakaran Pandiyan escreveu:
> From: Daniel Vetter <daniel.vetter@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.
> 
> References: bspec/11974 [SRD Interrupt Bit Definition DevHSW]
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Jose Roberto de Souza <jose.souza@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);

Why are we masking it here? During these 2 vblanks it's possible that
something will happen (e.g., frontbuffer writing, cursor moving, page
flipping). Are we guaranteed to get a POST_EXIT interrupt even if we
give up entering PSR before it is actually entered?


> +	}
> +
> +	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);
> +	}

We need another IIR write we do for the other platforms. This is not
cargo cult (as mentioned in previous review emails), this is required
since our hardware is able to store more than one IIR interrupt. Please
do it like we do for the other interrupts.

Do git-blame in the relevant lines and read the commit messages for
more information on why stuff is the way it is. If you still think this
is unnecessary, feel free to write a patch removing the unnecessary
stuff for every interrupt instead of of making just one register be
not-cargo-culted.

> +
>  	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)
> +

I know you'll remove these definitions in the next patch, but please
make sure you add the definitions following the new standard described
at the top of the file: (1 << 1) instead of (1<<1).


>  #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)

For this one you're allowed to not put the spaces since the other bits
are like this. But you also have the option of drive-by fixing the bits
of this register if you want since you're touching it.


>  #define DE_SPRITEC_FLIP_DONE_IVB	(1<<14)
>  #define DE_PLANEC_FLIP_DONE_IVB		(1<<13)
>  #define DE_PIPEC_VBLANK_IVB		(1<<10)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx