On Wed, 2018-03-28 at 17:37 +0000, Pandiyan, Dhinakaran wrote:
>
>
> On Wed, 2018-03-28 at 13:28 +0300, Ville Syrjälä wrote:
> > On Tue, Mar 27, 2018 at 06:33:11PM +0000, Pandiyan, Dhinakaran wrote:
> > > On Tue, 2018-03-27 at 13:24 +0300, Ville Syrjälä wrote:
> > > > On Mon, Mar 26, 2018 at 06:16:22PM -0700, Dhinakaran Pandiyan wrote:
> > > > > Interrupts other than the one for AUX errors are required only for
> > > > > debug,
> > > > > so unmask them via debugfs when the user requests debug.
> > > > >
> > > > > User can make such a request with
> > > > > echo 1 > <DEBUG_FS>/dri/0/i915_edp_psr_debug
> > > > >
> > > > > v2: Unroll loops (Ville)
> > > > > Avoid resetting error mask bits.
> > > > >
> > > > > Cc: Rodrigo Vivi <[email protected]>
> > > > > Cc: Ville Syrjälä <[email protected]>
> > > > > Cc: Chris Wilson <[email protected]>
> > > > > Signed-off-by: Dhinakaran Pandiyan <[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_debugfs.c | 36 +++++++++++++++++++++++-
> > > > > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > > > > drivers/gpu/drm/i915/i915_irq.c | 55
> > > > > +++++++++++--------------------------
> > > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++
> > > > > drivers/gpu/drm/i915/intel_psr.c | 54
> > > > > ++++++++++++++++++++++++++++++++++++
> > > > > 5 files changed, 108 insertions(+), 40 deletions(-)
> > > > >
> <snip>
> > > > >
> > > > > +void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool
> > > > > enable)
> > > > > +{
> > > > > + u32 mask;
> > > > > +
> > > > > + /* No PSR interrupts on VLV/CHV */
> > > > > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > + return;
> > > > > +
> > > > > + mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
> > > > > + EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
> > > > > +
> > > > > + if (INTEL_GEN(dev_priv) >= 8)
> > > > > + mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
> > > > > + EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
> > > > > + EDP_PSR_POST_EXIT(TRANSCODER_B) |
> > > > > + EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
> > > > > + EDP_PSR_POST_EXIT(TRANSCODER_C) |
> > > > > + EDP_PSR_PRE_ENTRY(TRANSCODER_C);
> > > > > +
> > > > > + if (enable) {
> > > > > + WRITE_ONCE(dev_priv->psr.debug, true);
> > > > > + I915_WRITE(EDP_PSR_IMR, I915_READ(EDP_PSR_IMR) & ~mask);
> > > >
> > > > Why RMW?
> > > >
> > > Avoids updating this function when new PSR error bits are added in
> > > i915_irq.c
> >
> > The usual rule is "avoid RMW unles there is a really compelling reason
> > for it". There is none in this case AFAICS.
> >
> > >
> > > Would you prefer
> > >
> > > mask |= EDP_PSR_ERROR(TRANCODER_A) | ... here?
> > >
> > > I think this has started to look ugly already. The loop was concise IMO.
> > >
> > > The other option is to
> > >
> > > #define HSW_PSR_INTR_DBG_MASK = 0x7
> > > #define BDW_PSR_INTR_DBG_MASK = 0x07070707
> >
> > Not sure what these have to do with the RMW.
> >
> They don't, that was an alternative to the series of OR's we have to do
> to derive the mask.
>
> 0x07070707 is the mask of all the relevant bits (error + debug). The
> above definitions could be used as
>
> if (INTEL_GEN(dev_priv) > 8)
> mask = BDW_PSR_INTR_DBG_MASK;
> else
> mask = HSW_PSR_INTR_DBG_MASK
>
>
> if (enable)
> I915_WRITE(EDP_PSR_IMR, ~mask)
>
> Anyway, I am not sure how you want this to be written at this point. Can
> you suggest in code what you like to see here?
>
+ error_mask = EDP_PSR_ERROR(TRANSCODER_EDP);
+ debug_mask = error_mask | EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
+ EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
+
+ if (INTEL_GEN(dev_priv) >= 8) {
+ error_mask |= EDP_PSR_ERROR(TRANSCODER_A) |
+ EDP_PSR_ERROR(TRANSCODER_B) |
+ EDP_PSR_ERROR(TRANSCODER_C);
+
+ debug_mask |= error_mask |
EDP_PSR_POST_EXIT(TRANSCODER_A) |
+ EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
+ EDP_PSR_POST_EXIT(TRANSCODER_B) |
+ EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
+ EDP_PSR_POST_EXIT(TRANSCODER_C) |
+ EDP_PSR_PRE_ENTRY(TRANSCODER_C);
+ }
if (enable) {
WRITE_ONCE(dev_priv->psr.debug, true);
- I915_WRITE(EDP_PSR_IMR, I915_READ(EDP_PSR_IMR) & ~mask);
+ I915_WRITE(EDP_PSR_IMR, ~debug_mask);
} else {
- I915_WRITE(EDP_PSR_IMR, I915_READ(EDP_PSR_IMR) | mask);
+ I915_WRITE(EDP_PSR_IMR, ~error_mask);
WRITE_ONCE(dev_priv->psr.debug, false);
}
Let me know if this is what you had in mind. Also,
dev->driver->irq_uninstall() will disable the debug interrupts. Does it
make sense to re-enable the interrupts in intel_psr_enable() if
psr.debug was still set?
> > >
> > >
> > >
> > > > > + } else {
> > > > > + I915_WRITE(EDP_PSR_IMR, I915_READ(EDP_PSR_IMR) | mask);
> > > > > + WRITE_ONCE(dev_priv->psr.debug, false);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32
> > > > > psr_iir)
> > > > > +{
> > > > > + u32 transcoders = BIT(TRANSCODER_EDP);
> > > > > + enum transcoder cpu_transcoder;
> > > > > +
> > > > > + if (INTEL_GEN(dev_priv) >= 8)
> > > > > + transcoders |= BIT(TRANSCODER_A) |
> > > > > + BIT(TRANSCODER_B) |
> > > > > + BIT(TRANSCODER_C);
> > > > > +
> > > > > + for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > > > > transcoders) {
> > > > > + /* FIXME: Exit PSR when this happens. */
> > > >
> > > > Should this maybe say "retrain the link manually when this happens"?
> > > >
> > >
> > > Yeah, we should do both in fact. Makes sense to exit PSR, link train
> > > manually and keep it disabled.
> >
> > BTW is there a hardware mechnism to inject this failure? Would be nice
> > for testing it.
> >
> Haven't seen anything like that in bspec.
>
> > If there is no way, maybe we could provide garbage settings for the AUX
> > stuff and that would cause the failure?
> >
> Probably, are you aware of a way that aux failures can be triggered in
> general? We could add it as an IGT test to make sure we do get
> interrupts and handle it correctly.
>
> > >
> > >
> > >
> > > > > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > > > > + DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> > > > > + transcoder_name(cpu_transcoder));
> > > > > +
> > > > > + if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder))
> > > > > + DRM_DEBUG_KMS("[transcoder %s] PSR entry
> > > > > attempt in 2 vblanks\n",
> > > > > + transcoder_name(cpu_transcoder));
> > > > > +
> > > > > + if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder))
> > > > > + DRM_DEBUG_KMS("[transcoder %s] PSR exit
> > > > > completed\n",
> > > > > + transcoder_name(cpu_transcoder));
> > > > > + }
> > > > > +}
> > > > > +
> > > > > static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > > > > {
> > > > > uint8_t psr_caps = 0;
> > > > > --
> > > > > 2.14.1
> > > >
> >
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
