Re: [PATCH v3 5/6] drm/i915/lobf: Check for sink error and disable LOBF

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Mon, 2025-01-06 at 09:45 +0530, Animesh Manna wrote:
> Disable LOBF/ALPM for any erroneous condition from sink side.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_alpm.c     | 38
> +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_alpm.h     |  1 +
>  .../drm/i915/display/intel_display_types.h    |  1 +
>  drivers/gpu/drm/i915/display/intel_dp.c       |  2 +
>  4 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 1cc0e5ed3f74..ab98b48ec47e 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -388,6 +388,7 @@ void intel_alpm_configure(struct intel_dp
> *intel_dp,
>  			  const struct intel_crtc_state *crtc_state)
>  {
>  	lnl_alpm_configure(intel_dp, crtc_state);
> +	intel_dp->alpm_parameters.transcoder = crtc_state-
> >cpu_transcoder;
>  }
>  
>  void intel_alpm_post_plane_update(struct intel_atomic_state *state,
> @@ -511,3 +512,40 @@ void intel_alpm_lobf_debugfs_add(struct
> intel_connector *connector)
>  	debugfs_create_file("i915_edp_lobf_info", 0444, root,
>  			    connector, &i915_edp_lobf_info_fops);
>  }
> +
> +void intel_alpm_short_pulse(struct intel_dp *intel_dp)
> +{
> +	struct intel_display *display = to_intel_display(intel_dp);
> +	struct drm_dp_aux *aux = &intel_dp->aux;
> +	enum transcoder cpu_transcoder = intel_dp-
> >alpm_parameters.transcoder;
> +	u8 val;
> +	int r;
> +
> +	if (DISPLAY_VER(display) < 20)
> +		return;
> +
> +	if (!(intel_de_read(display, ALPM_CTL(display,
> cpu_transcoder)) & ALPM_CTL_LOBF_ENABLE))
> +		return;
> +
> +	r = drm_dp_dpcd_readb(aux, DP_RECEIVER_ALPM_STATUS, &val);
> +	if (r != 1) {
> +		drm_err(display->drm, "Error reading ALPM
> status\n");
> +		return;
> +	}
> +
> +	if (val & DP_ALPM_LOCK_TIMEOUT_ERROR) {
> +		intel_de_rmw(display, ALPM_CTL(display,
> cpu_transcoder),
> +			     ALPM_CTL_ALPM_ENABLE |
> ALPM_CTL_LOBF_ENABLE |
> +			     ALPM_CTL_ALPM_AUX_LESS_ENABLE, 0);
> +
> +		intel_de_rmw(display,
> +			     PORT_ALPM_CTL(cpu_transcoder),
> +			     PORT_ALPM_CTL_ALPM_AUX_LESS_ENABLE, 0);
> +
> +		drm_dbg_kms(display->drm,
> +			    "ALPM lock timeout error, disabling
> LOBF\n");
> +
> +		/* Clearing error */
> +		drm_dp_dpcd_writeb(aux, DP_RECEIVER_ALPM_STATUS,
> val);
> +	}
> +}

I think we should centralize this. Either into intel_psr.c or here. I
think you should have locking for accessing ALPM_CTL register. Should
we have also own flag telling alpm_compute_config that we don't want to
use LOBF?

BR,

Jouni Högander

> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
> b/drivers/gpu/drm/i915/display/intel_alpm.h
> index 485e511629fb..96d4c903f3ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> @@ -29,4 +29,5 @@ void intel_alpm_post_plane_update(struct
> intel_atomic_state *state,
>  void intel_alpm_lobf_debugfs_add(struct intel_connector *connector);
>  bool intel_alpm_aux_wake_supported(struct intel_dp *intel_dp);
>  bool intel_alpm_aux_less_wake_supported(struct intel_dp *intel_dp);
> +void intel_alpm_short_pulse(struct intel_dp *intel_dp);
>  #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 37f061acb204..636af4c4e94d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1806,6 +1806,7 @@ struct intel_dp {
>  #define I915_LOBF_DEBUG_DISABLE			0x01
>  #define I915_LOBF_DEBUG_FORCE_EN		0x02
>  		bool lobf_debug;
> +		enum transcoder transcoder;
>  	} alpm_parameters;
>  
>  	u8 alpm_dpcd;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0d74adae2ec9..40f2760d8aaa 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5363,6 +5363,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  
>  	intel_psr_short_pulse(intel_dp);
>  
> +	intel_alpm_short_pulse(intel_dp);
> +
>  	if (intel_dp_test_short_pulse(intel_dp))
>  		reprobe_needed = true;
>