| Message ID | 20180320224151.6009-2-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 20, 2018 at 03:41:47PM -0700, Dhinakaran Pandiyan wrote: > 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. > > 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> > --- > 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. > + 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. 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. > } > } > > -- > 2.14.1
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@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. > > > > 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> > > --- > > 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. > > > + 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. > 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 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 >
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@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. > > > > > > 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> > > > --- > > > 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 > >
On Wed, 2018-03-21 at 21:29 +0200, Ville Syrjälä wrote: > 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@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. > > > > > > > > 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> > > > > --- > > > > 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. > The idea is to move towards hardware triggered exits since that's what is necessary for PSR2 selective updates[1]. More importantly, there are places in the driver where we can't manually exit PSR without circumventing locking issues - from vblank enable for instance. Relying on hardware triggered exits in the modeset/commit path keeps it consistent IMO. [1] Rodrigo found a register (PSR2_MAN_TRK_CTL) that allows manual control of selective updates. I don't know how well it works though. > > 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 > > > >
On Thu, Mar 22, 2018 at 01:19:19AM +0000, Pandiyan, Dhinakaran wrote: > > > > On Wed, 2018-03-21 at 21:29 +0200, Ville Syrjälä wrote: > > 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@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. > > > > > > > > > > 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> > > > > > --- > > > > > 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. > > > The idea is to move towards hardware triggered exits since that's what > is necessary for PSR2 selective updates[1]. More importantly, there are > places in the driver where we can't manually exit PSR without > circumventing locking issues - from vblank enable for instance. Relying > on hardware triggered exits in the modeset/commit path keeps it > consistent IMO. What's the difference between the hardware behaviour between "write any random mmio to trigger an exit" vs. "write some specific register to trigger an exit". I presume there is a way to trigger the exit explicitly?
On Thu, Mar 22, 2018 at 01:33:10PM +0200, Ville Syrjälä wrote: > On Thu, Mar 22, 2018 at 01:19:19AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > On Wed, 2018-03-21 at 21:29 +0200, Ville Syrjälä wrote: > > > 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@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. > > > > > > > > > > > > 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> > > > > > > --- > > > > > > 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. > > > > > The idea is to move towards hardware triggered exits since that's what > > is necessary for PSR2 selective updates[1]. More importantly, there are > > places in the driver where we can't manually exit PSR without > > circumventing locking issues - from vblank enable for instance. Relying > > on hardware triggered exits in the modeset/commit path keeps it > > consistent IMO. > > What's the difference between the hardware behaviour between "write any > random mmio to trigger an exit" vs. "write some specific register to > trigger an exit". I presume there is a way to trigger the exit > explicitly? There isn't. :( This is the main reason why psr_exit was still defined for core platforms as psr_disable.x > > -- > Ville Syrjälä > Intel OTC
On Thu, 2018-03-22 at 13:33 +0200, Ville Syrjälä wrote: > On Thu, Mar 22, 2018 at 01:19:19AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > On Wed, 2018-03-21 at 21:29 +0200, Ville Syrjälä wrote: > > > 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@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. > > > > > > > > > > > > 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> > > > > > > --- > > > > > > 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. > > > > > The idea is to move towards hardware triggered exits since that's what > > is necessary for PSR2 selective updates[1]. More importantly, there are > > places in the driver where we can't manually exit PSR without > > circumventing locking issues - from vblank enable for instance. Relying > > on hardware triggered exits in the modeset/commit path keeps it > > consistent IMO. > > What's the difference between the hardware behaviour between "write any > random mmio to trigger an exit" vs. "write some specific register to > trigger an exit". I presume there is a way to trigger the exit > explicitly? > I suppose we could write to the CUR_SURF_LIVE_STATUS (or something like that) and still mask "Display reg write". Can't tell without testing it on all of them - HSW, BDW and SKL. There is no change in the mask due to this series. Change in this patch gets undone in the next. Let's go ahead with this series and I'll come back to the masks after that.
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"); + 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); } }