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 <rodrigo.vivi@xxxxxxxxx>
> > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > > > ---
> > > > 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?
> >
> >
> >
> > > > + } 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
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
