| Message ID | 20180403212420.25007-3-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-04-03 at 14:24 -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 > > There are no locks to serialize PSR debug enabling from > irq_postinstall() and debugfs for simplicity. As irq_postinstall() is > called only during module initialization/resume and IGT subtests > aren't expected to modify PSR debug at those times, we should be > safe. > > v2: Unroll loops (Ville) > Avoid resetting error mask bits. > > v3: Unmask interrupts in postinstall() if debug was still enabled. > Avoid RMW (Ville) > > 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 | 57 +++++++++++-------------- > ---------- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_psr.c | 60 > +++++++++++++++++++++++++++++++++++++ > 5 files changed, 116 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 1dba2c451255..28f91df5b401 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; > + > + DRM_DEBUG_KMS("PSR debug %s\n", enableddisabled(val)); > + > + intel_runtime_pm_get(dev_priv); > + intel_psr_debug_control(dev_priv, !!val); > + intel_runtime_pm_put(dev_priv); > + > + 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); > @@ -4812,7 +4845,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} same as bellow, why not i915_edp_psr_int_debug? > }; > > 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 5373b171bb96..b97ed0cd4ca9 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -609,6 +609,7 @@ struct i915_psr { > bool has_hw_tracking; > bool psr2_enabled; > u8 sink_sync_latency; > + bool debug; maybe change to a name that gives more information about the use of this flag? like int_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..714570955196 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,8 @@ 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)); > + intel_psr_debug_control(dev_priv, dev_priv- > >psr.debug); > display_mask |= DE_EDP_PSR_INT_HSW; > } > > @@ -3845,6 +3819,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 +3844,11 @@ 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); intel_psr_debug_control() will already write to EDP_PSR_IMR with the right value, why write twice? > + intel_psr_debug_control(dev_priv, dev_priv->psr.debug); > > 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 d1452fd2a58d..53cb76b920e6 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1891,6 +1891,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 2d53f7398a6d..56ff2d7691a1 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -93,6 +93,66 @@ 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 debug_mask, error_mask; > + > + /* No PSR interrupts on VLV/CHV */ > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > + return; > + > + 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, ~debug_mask); > + } else { > + I915_WRITE(EDP_PSR_IMR, ~error_mask); > + WRITE_ONCE(dev_priv->psr.debug, false); > + } nit pick but why not something like? u32 debug_mask, mask; mask = EDP_PSR_ERROR(TRANSCODER_EDP); debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) ... if (enable) mask |= debug_mask; I915_WRITE(EDP_PSR_IMR, ~mask) WRITE_ONCE(dev_priv->psr.debug, enable); > +} > + > +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 > psr_iir) two minnor things here: - why you moved the function here? it adds more noise to the diff. - the rename from hsw_edp_psr_irq_handler to intel_psr_irq_handler should have being done in 'drm/i915: Enable edp psr error interrupts on bdw+' > +{ > + 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 and link train 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_coord_required(struct intel_dp *intel_dp) > { > uint8_t psr_caps = 0;
On Wed, 2018-04-04 at 00:54 +0000, Souza, Jose wrote: > On Tue, 2018-04-03 at 14:24 -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 > > > > There are no locks to serialize PSR debug enabling from > > irq_postinstall() and debugfs for simplicity. As irq_postinstall() is > > called only during module initialization/resume and IGT subtests > > aren't expected to modify PSR debug at those times, we should be > > safe. > > > > v2: Unroll loops (Ville) > > Avoid resetting error mask bits. > > > > v3: Unmask interrupts in postinstall() if debug was still enabled. > > Avoid RMW (Ville) > > > > 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 | 57 +++++++++++-------------- > > ---------- > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_psr.c | 60 > > +++++++++++++++++++++++++++++++++++++ > > 5 files changed, 116 insertions(+), 40 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index 1dba2c451255..28f91df5b401 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; > > + > > + DRM_DEBUG_KMS("PSR debug %s\n", enableddisabled(val)); > > + > > + intel_runtime_pm_get(dev_priv); > > + intel_psr_debug_control(dev_priv, !!val); > > + intel_runtime_pm_put(dev_priv); > > + > > + 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); > > @@ -4812,7 +4845,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} > > > same as bellow, why not i915_edp_psr_int_debug? > > > }; > > > > 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 5373b171bb96..b97ed0cd4ca9 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -609,6 +609,7 @@ struct i915_psr { > > bool has_hw_tracking; > > bool psr2_enabled; > > u8 sink_sync_latency; > > + bool debug; > > maybe change to a name that gives more information about the use of > this flag? like int_debug? > Enabling interrupts is one part of enabling PSR debug, that is how I think of it. We'll probably use the same control to enable other debug mechanisms as well. At the least to update timestamps from psr_activate and psr_exit for VLV/CHV. > > > > 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..714570955196 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,8 @@ 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)); > > + intel_psr_debug_control(dev_priv, dev_priv- > > >psr.debug); > > display_mask |= DE_EDP_PSR_INT_HSW; > > } > > > > @@ -3845,6 +3819,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 +3844,11 @@ 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); > > intel_psr_debug_control() will already write to EDP_PSR_IMR with the > right value, why write twice? > Good point. The benefit is it keeps whatever happens in psr_debug_control() separate from what irq_postinstall should always do. Doesn't outweigh the benefit of avoiding an additional MMIO write, I think. I will remove the IMR write from here. > > + intel_psr_debug_control(dev_priv, dev_priv->psr.debug); > > > > 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 d1452fd2a58d..53cb76b920e6 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1891,6 +1891,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 2d53f7398a6d..56ff2d7691a1 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -93,6 +93,66 @@ 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 debug_mask, error_mask; > > + > > + /* No PSR interrupts on VLV/CHV */ > > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > + return; > > + > > + 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, ~debug_mask); > > + } else { > > + I915_WRITE(EDP_PSR_IMR, ~error_mask); > > + WRITE_ONCE(dev_priv->psr.debug, false); > > + } > > > nit pick but why not something like? > > u32 debug_mask, mask; > > mask = EDP_PSR_ERROR(TRANSCODER_EDP); > debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) > ... > > if (enable) > mask |= debug_mask; > > I915_WRITE(EDP_PSR_IMR, ~mask) > WRITE_ONCE(dev_priv->psr.debug, enable); > > Don't see much advantage in writing it one way or the other. Having said that I have no issues in making the change. > > +} > > + > > +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 > > psr_iir) > > two minnor things here: > - why you moved the function here? it adds more noise to the diff. Wanted the PSR irq_handler to be close to the rest of PSR code. We'll eventually want to call other functions in this file to handle errors. > - the rename from hsw_edp_psr_irq_handler to intel_psr_irq_handler > should have being done in 'drm/i915: Enable edp psr error interrupts on > bdw+' > I wanted to make only minimal rebase changes for the previous two patches since I did not write them originally. > > +{ > > + 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 and link train 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_coord_required(struct intel_dp *intel_dp) > > { > > uint8_t psr_caps = 0;
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 1dba2c451255..28f91df5b401 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; + + DRM_DEBUG_KMS("PSR debug %s\n", enableddisabled(val)); + + intel_runtime_pm_get(dev_priv); + intel_psr_debug_control(dev_priv, !!val); + intel_runtime_pm_put(dev_priv); + + 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); @@ -4812,7 +4845,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 5373b171bb96..b97ed0cd4ca9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -609,6 +609,7 @@ struct i915_psr { bool has_hw_tracking; bool psr2_enabled; u8 sink_sync_latency; + 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..714570955196 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,8 @@ 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)); + intel_psr_debug_control(dev_priv, dev_priv->psr.debug); display_mask |= DE_EDP_PSR_INT_HSW; } @@ -3845,6 +3819,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 +3844,11 @@ 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); + intel_psr_debug_control(dev_priv, dev_priv->psr.debug); 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 d1452fd2a58d..53cb76b920e6 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1891,6 +1891,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 2d53f7398a6d..56ff2d7691a1 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -93,6 +93,66 @@ 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 debug_mask, error_mask; + + /* No PSR interrupts on VLV/CHV */ + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) + return; + + 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, ~debug_mask); + } else { + I915_WRITE(EDP_PSR_IMR, ~error_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 and link train 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_coord_required(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 There are no locks to serialize PSR debug enabling from irq_postinstall() and debugfs for simplicity. As irq_postinstall() is called only during module initialization/resume and IGT subtests aren't expected to modify PSR debug at those times, we should be safe. v2: Unroll loops (Ville) Avoid resetting error mask bits. v3: Unmask interrupts in postinstall() if debug was still enabled. Avoid RMW (Ville) 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 | 57 +++++++++++------------------------ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_psr.c | 60 +++++++++++++++++++++++++++++++++++++ 5 files changed, 116 insertions(+), 40 deletions(-)