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.v...@intel.com>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
