Re: [PATCH 3/4] drm/i915/pps: add intel_pps_dp_power_down()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 04, 2024 at 05:02:33PM +0300, Jani Nikula wrote:
> Add intel_pps_dp_power_down() and move the VLV/CHV active pipe clear
> there from intel_dp_link_down(), hiding the PPS pipe details inside PPS
> code.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/g4x_dp.c    |  9 +--------
>  drivers/gpu/drm/i915/display/intel_pps.c | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_pps.h |  1 +
>  3 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
> index 1f9812223263..98405fbd0e04 100644
> --- a/drivers/gpu/drm/i915/display/g4x_dp.c
> +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
> @@ -475,14 +475,7 @@ intel_dp_link_down(struct intel_encoder *encoder,
>  		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
>  	}
>  
> -	msleep(intel_dp->pps.panel_power_down_delay);
> -
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -		intel_wakeref_t wakeref;
> -
> -		with_intel_pps_lock(intel_dp, wakeref)
> -			intel_dp->pps.active_pipe = INVALID_PIPE;
> -	}
> +	intel_pps_dp_power_down(intel_dp);
>  }
>  
>  static void g4x_dp_audio_enable(struct intel_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index 9e54acfea992..a7f7e5e1f3aa 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -1599,6 +1599,22 @@ static void pps_init_registers(struct intel_dp *intel_dp, bool force_disable_vdd
>  		    (intel_de_read(display, regs.pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK));
>  }
>  
> +/* Call on all DP, not just eDP */
> +void intel_pps_dp_power_down(struct intel_dp *intel_dp)

The name seems to imply this powers down something, which it doesn't.

> +{
> +	struct intel_display *display = to_intel_display(intel_dp);
> +	struct drm_i915_private *i915 = to_i915(display->drm);
> +
> +	msleep(intel_dp->pps.panel_power_down_delay);

I can't actually figure out why this msleep() even exists. We already
waited for the power down delay (by waiting for the pps state machine)
when we turned off the panel power, so this seems completely redundant.

The whole pre-ddi modeset sequence is a bit weird because the port
enable/disable essentially happens on the wrong side of panel power
enable/disable. But I guess that's just how the hw works and the PPS
somehow makes sure things happen in the right order. And I suppose
the magic PPS register write protect thing even prevents doing it
in the opposite order (although IIRC we always disable the write
protect mechanism).

> +
> +	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
> +		intel_wakeref_t wakeref;
> +
> +		with_intel_pps_lock(intel_dp, wakeref)
> +			intel_dp->pps.active_pipe = INVALID_PIPE;
> +	}

This part is basically the counterpart to vlv_pps_init(),
so the function naming should probably reflect that somehow.
vlv_pps_port_{enable,disable}() or something like that?

> +}
> +
>  /* Call on all DP, not just eDP */
>  void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h
> index 8dbea05f498d..42f0377a93a8 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.h
> +++ b/drivers/gpu/drm/i915/display/intel_pps.h
> @@ -45,6 +45,7 @@ void intel_pps_init_late(struct intel_dp *intel_dp);
>  
>  void intel_pps_dp_init(struct intel_dp *intel_dp);
>  void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp);
> +void intel_pps_dp_power_down(struct intel_dp *intel_dp);
>  void intel_pps_reset_all(struct intel_display *display);
>  
>  void vlv_pps_init(struct intel_encoder *encoder,
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel