Re: [PATCH 09/14] drm/i915/dpio: Derive the phy from the port rather than pipe in encoder hooks

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 22 Apr 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> In the encoder hooks we are dealing primarily with the encoder,
> so derive the DPIO PHY from the encoder rather than the pipe.
> Technically this doesn't matter as we can't cross connect
> pipes<->port across PHY boundaries, but it does conveny the
> intention more accurately.

I'll note that for most places converting vlv_dig_port_to_* to
vlv_encoder_to_* would be more convenient in the caller side. We have
the encoder available where they're needed, and in many places we use
enc_to_dig_port(encoder) just to be able to use vlv_dig_port_to_*.

I'd just convert them to vlv_encoder_to_*.

Regardless, this does what it says on the tin,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Oh, one comment inline near the end.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dpio_phy.c | 27 ++++++++-----------
>  drivers/gpu/drm/i915/vlv_sideband.c           |  1 -
>  2 files changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpio_phy.c b/drivers/gpu/drm/i915/display/intel_dpio_phy.c
> index e4a04c9b5b19..4fafac534967 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpio_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpio_phy.c
> @@ -719,9 +719,8 @@ void chv_set_phy_signal_level(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	enum dpio_channel ch = vlv_dig_port_to_channel(dig_port);
> -	enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe);
> +	enum dpio_phy phy = vlv_dig_port_to_phy(dig_port);
>  	u32 val;
>  	int i;
>  
> @@ -814,9 +813,9 @@ void chv_data_lane_soft_reset(struct intel_encoder *encoder,
>  			      bool reset)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -	enum dpio_channel ch = vlv_dig_port_to_channel(enc_to_dig_port(encoder));
> -	enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +	enum dpio_channel ch = vlv_dig_port_to_channel(dig_port);
> +	enum dpio_phy phy = vlv_dig_port_to_phy(dig_port);
>  	u32 val;
>  
>  	val = vlv_dpio_read(dev_priv, phy, VLV_PCS01_DW0(ch));
> @@ -861,7 +860,7 @@ void chv_phy_pre_pll_enable(struct intel_encoder *encoder,
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	enum dpio_channel ch = vlv_dig_port_to_channel(dig_port);
> -	enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe);
> +	enum dpio_phy phy = vlv_dig_port_to_phy(dig_port);
>  	enum pipe pipe = crtc->pipe;
>  	unsigned int lane_mask =
>  		intel_dp_unused_lane_mask(crtc_state->lane_count);
> @@ -941,9 +940,8 @@ void chv_phy_pre_encoder_enable(struct intel_encoder *encoder,
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	enum dpio_channel ch = vlv_dig_port_to_channel(dig_port);
> -	enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe);
> +	enum dpio_phy phy = vlv_dig_port_to_phy(dig_port);
>  	int data, i, stagger;
>  	u32 val;
>  
> @@ -1030,8 +1028,8 @@ void chv_phy_post_pll_disable(struct intel_encoder *encoder,
>  			      const struct intel_crtc_state *old_crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	enum dpio_phy phy = vlv_dig_port_to_phy(enc_to_dig_port(encoder));
>  	enum pipe pipe = to_intel_crtc(old_crtc_state->uapi.crtc)->pipe;
> -	enum dpio_phy phy = vlv_pipe_to_phy(pipe);
>  	u32 val;
>  
>  	vlv_dpio_get(dev_priv);
> @@ -1068,9 +1066,8 @@ void vlv_set_phy_signal_level(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	enum dpio_channel ch = vlv_dig_port_to_channel(dig_port);
> -	enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe);
> +	enum dpio_phy phy = vlv_dig_port_to_phy(dig_port);
>  
>  	vlv_dpio_get(dev_priv);
>  
> @@ -1095,9 +1092,8 @@ void vlv_phy_pre_pll_enable(struct intel_encoder *encoder,
>  {
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	enum dpio_channel ch = vlv_dig_port_to_channel(dig_port);
> -	enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe);
> +	enum dpio_phy phy = vlv_dig_port_to_phy(dig_port);
>  
>  	/* Program Tx lane resets to default */
>  	vlv_dpio_get(dev_priv);
> @@ -1127,8 +1123,8 @@ void vlv_phy_pre_encoder_enable(struct intel_encoder *encoder,
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	enum dpio_channel ch = vlv_dig_port_to_channel(dig_port);
> +	enum dpio_phy phy = vlv_dig_port_to_phy(dig_port);
>  	enum pipe pipe = crtc->pipe;
> -	enum dpio_phy phy = vlv_pipe_to_phy(pipe);
>  	u32 val;
>  
>  	vlv_dpio_get(dev_priv);
> @@ -1154,9 +1150,8 @@ void vlv_phy_reset_lanes(struct intel_encoder *encoder,
>  {
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>  	enum dpio_channel ch = vlv_dig_port_to_channel(dig_port);
> -	enum dpio_phy phy = vlv_pipe_to_phy(crtc->pipe);
> +	enum dpio_phy phy = vlv_dig_port_to_phy(dig_port);
>  
>  	vlv_dpio_get(dev_priv);
>  	vlv_dpio_write(dev_priv, phy, VLV_PCS_DW0(ch), 0x00000000);
> diff --git a/drivers/gpu/drm/i915/vlv_sideband.c b/drivers/gpu/drm/i915/vlv_sideband.c
> index ffa195560d0d..68291412f4cb 100644
> --- a/drivers/gpu/drm/i915/vlv_sideband.c
> +++ b/drivers/gpu/drm/i915/vlv_sideband.c
> @@ -9,7 +9,6 @@
>  #include "vlv_sideband.h"
>  
>  #include "display/intel_dpio_phy.h"
> -#include "display/intel_display_types.h"

I guess this should be done in some other patch?

>  
>  /*
>   * IOSF sideband, see VLV2_SidebandMsg_HAS.docx and

-- 
Jani Nikula, Intel