On Tue, Apr 17, 2018 at 01:01:39PM -0700, Dhinakaran Pandiyan wrote: > > > > On Tue, 2018-04-17 at 20:41 +0300, Ville Syrjälä wrote: > > On Mon, Apr 16, 2018 at 05:43:54PM -0700, Paulo Zanoni wrote: > > > Em Qui, 2018-04-05 às 15:00 -0700, Dhinakaran Pandiyan escreveu: > > > > From: Daniel Vetter <daniel.vet...@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.syrj...@linux.intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.v...@intel.com> > > > > Cc: Daniel Vetter <daniel.vet...@intel.com> > > > > Signed-off-by: Daniel Vetter <daniel.vet...@intel.com> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com> > > > > Reviewed-by: Jose Roberto de Souza <jose.so...@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). > > > > The masking was here to avoid seeing a big flood of entry interrupts. > > > > > Are we guaranteed to get a POST_EXIT interrupt even if we > > > give up entering PSR before it is actually entered? > > > > No. The exit interrupt only happens if you actually reached PSR. > > > > > > > > > > > > + } > > > > + > > > > + 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. > > > > My reply and the review were specifically about the POSTING_READ(). I > agree on the second IIR write, but that was not what the original review > comment was about. > > I'm still skeptical about inserting the POSTING_READ() between write's > There are at commit messages and emails that indicate the > POSTING_READS() are indeed cargo culted. Let me find more data about > this and get back. > > > I don't remember if the PSR IIR is double buffered or not. I would > > assumeme it is. It is super easy to verify though if you want to be > > sure: > > 1. unmask the interrupt > > 2. generate at least two interrupts > > 3. mask the interrupt > > 4. clear IIR > > 5. read IIR > > 6. clear IIR > > 7. read IIR > > > > Step 5 should still show the IIR bit set if the IIR is double buffered. > > And step 7 it should show it to be clear. > > > > I'll give it a try, thanks!
I'm far behind this week and merging the series was on top of my todo list... :( sorry for that. I know that you guys already talked about it and know how to do the follow-up ;) > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx