On Wed, Mar 21, 2018 at 07:19:53PM +0000, Pandiyan, Dhinakaran wrote:
>
>
>
> On Wed, 2018-03-21 at 20:59 +0200, Ville Syrjälä wrote:
> > On Tue, Mar 20, 2018 at 03:41:47PM -0700, Dhinakaran Pandiyan wrote:
> > > From: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > >
> > > 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.
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
> > > drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> > > 3 files changed, 44 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 44eef355e12c..e94a835b7515 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2392,6 +2392,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");
> >
> > I doubt we want to have the entry/exit interrupts generate all this
> > noise in dmesg all the time. We should probably only enable the
> > interrupts for testing. The error interrupt I suppose we want always,
> > might even want DRM_ERROR on it.
>
> I implement debugfs control in Patch 4/5, agreed on DRM_ERROR.
Right. It's a bit hard to read this with stuff getting
added/remove/moved more or less randomly.
>
> >
> > > + 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)
> > > {
> > > @@ -2404,6 +2424,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);
> > >
> > > @@ -3252,6 +3275,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);
> > > + }
> > > +
> > > gen5_gt_irq_reset(dev_priv);
> > >
> > > ibx_irq_reset(dev_priv);
> > > @@ -3663,6 +3691,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 1e000f3004cb..3447f03eac3d 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -3828,6 +3828,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)
> > > +
> > > #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 */
> > >
> > > @@ -6628,6 +6635,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)
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index 317cb4a12693..27dfd507a4f7 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -613,7 +613,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > I915_WRITE(EDP_PSR_DEBUG,
> > > EDP_PSR_DEBUG_MASK_MEMUP |
> > > EDP_PSR_DEBUG_MASK_HPD |
> > > - EDP_PSR_DEBUG_MASK_LPSP);
> > > + EDP_PSR_DEBUG_MASK_LPSP |
> > > + EDP_PSR_DEBUG_MASK_REG_WRITE);
> >
> > I don't think Daniel's original had this actually. I think I added it
> > because I didn't want mmio to mess up my testing.
>
> It gets removed in a separate patch (2/5). So I assumed Daniel added it
> and you removed it.
Hmm. That could indeed be the case.
>
>
> > In general I'd really
> > prefer to mask everything so that we can be sure that the PSR exits
> > happen when we want/need them and not randomly due to other activity.
> >
> > If any random mmio can cause a PSR exit it's going to hard to prove that
> > we're doing the correct thing. Also seems pretty wasteful if eg. GT
> > interrupts can cause PSR exits for no good reason. I never tested to see
> > which registers cause the PSR exit. That would be good to know.
> >
>
> The same bit on SKL+ is called "Mask Display Reg Write", so it either
> means
> 1) HSW and BDW exit only on display register writes and the bspec
> description is ambiguous or
The problem is that eg. some of the interrupt registers live in the
display block. Hence any interrupt could potentially cause the PSR exit.
As could any register read we do for debug purpose etc. So at least I
would feel safer if PSR can be made to work reliably with more or less
everything masked.
> 2) exiting specifically on display MMIO writes was added in SKL.
>
> I'll see if I can get a HSW/BDW with PSR panel to test, but assuming the
> former is safer.
>
>
>
> > > }
> > > }
> > >
> > > --
> > > 2.14.1
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
