| Message ID | 20180327011623.3341-3-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > 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. > + 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? > + } 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"? > + 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
Quoting Dhinakaran Pandiyan (2018-03-27 02:16:22) > 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@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > 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; > + > + DRM_DEBUG_KMS("%s PSR debug\n", val == 1 ? "Enabling" : "Disabling"); > + intel_psr_debug_control(dev_priv, val); > + > + return 0; > +} A really useful trick for features like this (that we think should be used wherever it can) is just to enable debug while the debugfs file is open. igt need only do something like openat(debugfs_dir, "i915_edp_psr_debug", O_RDONLY); and then it will be automatically released and the system returned to default when terminated. You can then enforce exclusive access or keep an open count. -Chris
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@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > 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 >
On Tue, 2018-03-27 at 11:26 +0100, Chris Wilson wrote: > Quoting Dhinakaran Pandiyan (2018-03-27 02:16:22) > > 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@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > 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; > > + > > + DRM_DEBUG_KMS("%s PSR debug\n", val == 1 ? "Enabling" : "Disabling"); > > + intel_psr_debug_control(dev_priv, val); > > + > > + return 0; > > +} > > A really useful trick for features like this (that we think should be > used wherever it can) is just to enable debug while the debugfs file is > open. igt need only do something like > openat(debugfs_dir, "i915_edp_psr_debug", O_RDONLY); That is neat. We also want to support user enabling/disabling PSR debug via shell. The semantics get slightly confusing taking that into consideration. For example, 1) should cat $debugfs/i915_edp_psr_debug also enable debug or just print the current status of debug 2) if the file is already open (debug enabled), should echo 0 > $debugfs/i915_edp_psr_debug disable debug or check for the refcount to drop to zero. Choosing the correct solution and implementing it correctly feels like we'd be over-engineering. > and then it will be automatically released and the system returned to > default when terminated. You can then enforce exclusive access or keep an > open count. > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > 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 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. > > > > > > + } 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. If there is no way, maybe we could provide garbage settings for the AUX stuff and that would cause the failure? > > > > > > + 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 > >
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@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > --- > > > > 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 > > > >
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 <rodrigo.vivi@intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > --- > > > > > 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 > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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; + + DRM_DEBUG_KMS("%s PSR debug\n", val == 1 ? "Enabling" : "Disabling"); + 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); + } 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. */ + 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;
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@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- 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(-)