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(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 7816cd53100a..6fd801ef7cbb 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2690,6 +2690,39 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> > return 0;
> > }
> >
> > +static int
> > +i915_edp_psr_debug_set(void *data, u64 val)
> > +{
> > + struct drm_i915_private *dev_priv = data;
> > +
> > + if (!CAN_PSR(dev_priv))
> > + return -ENODEV;
> > +
> > + if (val < 0 || val > 1)
> > + return -EINVAL;
>
> Can't be < 0.
>
> > +
> > + DRM_DEBUG_KMS("%s PSR debug\n", val == 1 ? "Enabling" : "Disabling");
>
> ==1 seems pointless
> enableddisabled() could be used perhaps.
>
Will do.
>
> > + intel_psr_debug_control(dev_priv, val);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +i915_edp_psr_debug_get(void *data, u64 *val)
> > +{
> > + struct drm_i915_private *dev_priv = data;
> > +
> > + if (!CAN_PSR(dev_priv))
> > + return -ENODEV;
> > +
> > + *val = READ_ONCE(dev_priv->psr.debug);
> > + return 0;
> > +}
> > +
> > +DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops,
> > + i915_edp_psr_debug_get, i915_edp_psr_debug_set,
> > + "%llu\n");
> > +
> > static int i915_sink_crc(struct seq_file *m, void *data)
> > {
> > struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > @@ -4811,7 +4844,8 @@ static const struct i915_debugfs_files {
> > {"i915_guc_log_relay", &i915_guc_log_relay_fops},
> > {"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
> > {"i915_ipc_status", &i915_ipc_status_fops},
> > - {"i915_drrs_ctl", &i915_drrs_ctl_fops}
> > + {"i915_drrs_ctl", &i915_drrs_ctl_fops},
> > + {"i915_edp_psr_debug", &i915_edp_psr_debug_fops}
> > };
> >
> > int i915_debugfs_register(struct drm_i915_private *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c9c3b2ba6a86..c0224a86344e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -608,6 +608,7 @@ struct i915_psr {
> > bool colorimetry_support;
> > bool alpm;
> > bool has_hw_tracking;
> > + bool debug;
> >
> > void (*enable_source)(struct intel_dp *,
> > const struct intel_crtc_state *);
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 8a894adf2ca1..e5aaf805c6a8 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2391,40 +2391,6 @@ 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);
> > - u32 edp_psr_imr = I915_READ(EDP_PSR_IMR);
> > - u32 mask = BIT(TRANSCODER_EDP);
> > - enum transcoder cpu_transcoder;
> > -
> > - if (INTEL_GEN(dev_priv) >= 8)
> > - mask |= BIT(TRANSCODER_A) |
> > - BIT(TRANSCODER_B) |
> > - BIT(TRANSCODER_C);
> > -
> > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, mask) {
> > - if (edp_psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > - DRM_DEBUG_KMS("Transcoder %s PSR error\n",
> > - transcoder_name(cpu_transcoder));
> > -
> > - if (edp_psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > - DRM_DEBUG_KMS("Transcoder %s PSR prepare entry in 2 vblanks\n",
> > - transcoder_name(cpu_transcoder));
> > - edp_psr_imr |= EDP_PSR_PRE_ENTRY(cpu_transcoder);
> > - }
> > -
> > - if (edp_psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> > - DRM_DEBUG_KMS("Transcoder %s PSR exit completed\n",
> > - transcoder_name(cpu_transcoder));
> > - edp_psr_imr &= ~EDP_PSR_PRE_ENTRY(cpu_transcoder);
> > - }
> > - }
> > -
> > - I915_WRITE(EDP_PSR_IMR, edp_psr_imr);
> > - I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> > -}
> > -
> > static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > u32 de_iir)
> > {
> > @@ -2437,8 +2403,12 @@ 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_EDP_PSR_INT_HSW) {
> > + u32 psr_iir = I915_READ(EDP_PSR_IIR);
> > +
> > + intel_psr_irq_handler(dev_priv, psr_iir);
> > + I915_WRITE(EDP_PSR_IIR, psr_iir);
> > + }
> >
> > if (de_iir & DE_AUX_CHANNEL_A_IVB)
> > dp_aux_irq_handler(dev_priv);
> > @@ -2580,7 +2550,10 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> > }
> >
> > if (iir & GEN8_DE_EDP_PSR) {
> > - hsw_edp_psr_irq_handler(dev_priv);
> > + u32 psr_iir = I915_READ(EDP_PSR_IIR);
> > +
> > + intel_psr_irq_handler(dev_priv, psr_iir);
> > + I915_WRITE(EDP_PSR_IIR, psr_iir);
> > found = true;
> > }
> >
> > @@ -3729,7 +3702,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> >
> > if (IS_HASWELL(dev_priv)) {
> > gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > - I915_WRITE(EDP_PSR_IMR, 0);
> > + I915_WRITE(EDP_PSR_IMR, ~EDP_PSR_ERROR(TRANSCODER_EDP));
> > display_mask |= DE_EDP_PSR_INT_HSW;
> > }
> >
> > @@ -3845,6 +3818,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> > u32 de_port_enables;
> > u32 de_misc_masked = GEN8_DE_MISC_GSE | GEN8_DE_EDP_PSR;
> > enum pipe pipe;
> > + u32 psr_masked;
> >
> > if (INTEL_GEN(dev_priv) >= 9) {
> > de_pipe_masked |= GEN9_DE_PIPE_IRQ_FAULT_ERRORS;
> > @@ -3869,7 +3843,10 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> > de_port_enables |= GEN8_PORT_DP_A_HOTPLUG;
> >
> > gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > - I915_WRITE(EDP_PSR_IMR, 0);
> > + psr_masked = EDP_PSR_ERROR(TRANSCODER_EDP) |
> > + EDP_PSR_ERROR(TRANSCODER_A) | EDP_PSR_ERROR(TRANSCODER_B) |
> > + EDP_PSR_ERROR(TRANSCODER_C);
> > + I915_WRITE(EDP_PSR_IMR, ~psr_masked);
> >
> > for_each_pipe(dev_priv, pipe) {
> > dev_priv->de_irq_mask[pipe] = ~de_pipe_masked;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a215aa78b0be..8be75cc5bf24 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1887,6 +1887,8 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
> > unsigned frontbuffer_bits);
> > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > struct intel_crtc_state *crtc_state);
> > +void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool enable);
> > +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
> >
> > /* intel_runtime_pm.c */
> > int intel_power_domains_init(struct drm_i915_private *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index b8e083e10029..fdc5e1bf198a 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -93,6 +93,60 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> > intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > }
> >
> > +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
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
> > + } 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.
> > + 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
