Re: [PATCH 01/65] drm/i915: pass dev_priv explicitly to DPLL

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Jun 04, 2024 at 06:25:19PM +0300, Jani Nikula wrote:
> Avoid the implicit dev_priv local variable use, and pass dev_priv
> explicitly to the DPLL register macro.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

(
even though it is a big series and overall it looks all right,
I will put one by one for 2 reasons:
- so I ensure I don't miss details and I know where I stopped
- so you can use dim b4 directly without extra rebase and adding my rv-b one by one...
)

> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 21 ++++-----
>  .../drm/i915/display/intel_display_power.c    |  2 +-
>  .../i915/display/intel_display_power_well.c   |  6 +--
>  drivers/gpu/drm/i915/display/intel_dpll.c     | 45 ++++++++++---------
>  drivers/gpu/drm/i915/display/intel_dvo.c      |  5 ++-
>  drivers/gpu/drm/i915/display/intel_pps.c      |  2 +-
>  drivers/gpu/drm/i915/i915_reg.h               |  2 +-
>  7 files changed, 43 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 7370acdd6b8b..42e2d884c98e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -382,11 +382,11 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>  		fallthrough;
>  	case PORT_B:
>  		port_mask = DPLL_PORTB_READY_MASK;
> -		dpll_reg = DPLL(0);
> +		dpll_reg = DPLL(dev_priv, 0);
>  		break;
>  	case PORT_C:
>  		port_mask = DPLL_PORTC_READY_MASK;
> -		dpll_reg = DPLL(0);
> +		dpll_reg = DPLL(dev_priv, 0);
>  		expected_mask <<= 4;
>  		break;
>  	case PORT_D:
> @@ -8212,11 +8212,12 @@ void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	 * the P1/P2 dividers. Otherwise the DPLL will keep using the old
>  	 * dividers, even though the register value does change.
>  	 */
> -	intel_de_write(dev_priv, DPLL(pipe), dpll & ~DPLL_VGA_MODE_DIS);
> -	intel_de_write(dev_priv, DPLL(pipe), dpll);
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe),
> +		       dpll & ~DPLL_VGA_MODE_DIS);
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe), dpll);
>  
>  	/* Wait for the clocks to stabilize. */
> -	intel_de_posting_read(dev_priv, DPLL(pipe));
> +	intel_de_posting_read(dev_priv, DPLL(dev_priv, pipe));
>  	udelay(150);
>  
>  	/* The pixel multiplier can only be updated once the
> @@ -8224,12 +8225,12 @@ void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	 *
>  	 * So write it again.
>  	 */
> -	intel_de_write(dev_priv, DPLL(pipe), dpll);
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe), dpll);
>  
>  	/* We do this three times for luck */
>  	for (i = 0; i < 3 ; i++) {
> -		intel_de_write(dev_priv, DPLL(pipe), dpll);
> -		intel_de_posting_read(dev_priv, DPLL(pipe));
> +		intel_de_write(dev_priv, DPLL(dev_priv, pipe), dpll);
> +		intel_de_posting_read(dev_priv, DPLL(dev_priv, pipe));
>  		udelay(150); /* wait for warmup */
>  	}
>  
> @@ -8262,8 +8263,8 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
>  
>  	intel_wait_for_pipe_scanline_stopped(crtc);
>  
> -	intel_de_write(dev_priv, DPLL(pipe), DPLL_VGA_MODE_DIS);
> -	intel_de_posting_read(dev_priv, DPLL(pipe));
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe), DPLL_VGA_MODE_DIS);
> +	intel_de_posting_read(dev_priv, DPLL(dev_priv, pipe));
>  }
>  
>  void intel_hpd_poll_fini(struct drm_i915_private *i915)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 34b6d843bc9e..3c5cb587f9bd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -1772,7 +1772,7 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>  	 * current lane status.
>  	 */
>  	if (intel_power_well_is_enabled(dev_priv, cmn_bc)) {
> -		u32 status = intel_de_read(dev_priv, DPLL(PIPE_A));
> +		u32 status = intel_de_read(dev_priv, DPLL(dev_priv, PIPE_A));
>  		unsigned int mask;
>  
>  		mask = status & DPLL_PORTB_READY_MASK;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> index 83f616097a29..3b6cb237d80a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> @@ -1196,13 +1196,13 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)
>  	 * CHV DPLL B/C have some issues if VGA mode is enabled.
>  	 */
>  	for_each_pipe(dev_priv, pipe) {
> -		u32 val = intel_de_read(dev_priv, DPLL(pipe));
> +		u32 val = intel_de_read(dev_priv, DPLL(dev_priv, pipe));
>  
>  		val |= DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
>  		if (pipe != PIPE_A)
>  			val |= DPLL_INTEGRATED_CRI_CLK_VLV;
>  
> -		intel_de_write(dev_priv, DPLL(pipe), val);
> +		intel_de_write(dev_priv, DPLL(dev_priv, pipe), val);
>  	}
>  
>  	vlv_init_display_clock_gating(dev_priv);
> @@ -1355,7 +1355,7 @@ static void assert_chv_phy_status(struct drm_i915_private *dev_priv)
>  		 */
>  		if (BITS_SET(phy_control,
>  			     PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1)) &&
> -		    (intel_de_read(dev_priv, DPLL(PIPE_B)) & DPLL_VCO_ENABLE) == 0)
> +		    (intel_de_read(dev_priv, DPLL(dev_priv, PIPE_B)) & DPLL_VCO_ENABLE) == 0)
>  			phy_status |= PHY_STATUS_CMN_LDO(DPIO_PHY0, DPIO_CH1);
>  
>  		if (BITS_SET(phy_control,
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll.c b/drivers/gpu/drm/i915/display/intel_dpll.c
> index a981f45facb3..a007ca5208b8 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll.c
> @@ -403,7 +403,7 @@ void i9xx_dpll_get_hw_state(struct intel_crtc *crtc,
>  		hw_state->dpll_md = tmp;
>  	}
>  
> -	hw_state->dpll = intel_de_read(dev_priv, DPLL(crtc->pipe));
> +	hw_state->dpll = intel_de_read(dev_priv, DPLL(dev_priv, crtc->pipe));
>  
>  	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
>  		hw_state->fp0 = intel_de_read(dev_priv, FP0(crtc->pipe));
> @@ -1842,11 +1842,12 @@ void i9xx_enable_pll(const struct intel_crtc_state *crtc_state)
>  	 * the P1/P2 dividers. Otherwise the DPLL will keep using the old
>  	 * dividers, even though the register value does change.
>  	 */
> -	intel_de_write(dev_priv, DPLL(pipe), hw_state->dpll & ~DPLL_VGA_MODE_DIS);
> -	intel_de_write(dev_priv, DPLL(pipe), hw_state->dpll);
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe),
> +		       hw_state->dpll & ~DPLL_VGA_MODE_DIS);
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe), hw_state->dpll);
>  
>  	/* Wait for the clocks to stabilize. */
> -	intel_de_posting_read(dev_priv, DPLL(pipe));
> +	intel_de_posting_read(dev_priv, DPLL(dev_priv, pipe));
>  	udelay(150);
>  
>  	if (DISPLAY_VER(dev_priv) >= 4) {
> @@ -1857,13 +1858,13 @@ void i9xx_enable_pll(const struct intel_crtc_state *crtc_state)
>  		 *
>  		 * So write it again.
>  		 */
> -		intel_de_write(dev_priv, DPLL(pipe), hw_state->dpll);
> +		intel_de_write(dev_priv, DPLL(dev_priv, pipe), hw_state->dpll);
>  	}
>  
>  	/* We do this three times for luck */
>  	for (i = 0; i < 3; i++) {
> -		intel_de_write(dev_priv, DPLL(pipe), hw_state->dpll);
> -		intel_de_posting_read(dev_priv, DPLL(pipe));
> +		intel_de_write(dev_priv, DPLL(dev_priv, pipe), hw_state->dpll);
> +		intel_de_posting_read(dev_priv, DPLL(dev_priv, pipe));
>  		udelay(150); /* wait for warmup */
>  	}
>  }
> @@ -1991,11 +1992,11 @@ static void _vlv_enable_pll(const struct intel_crtc_state *crtc_state)
>  	const struct i9xx_dpll_hw_state *hw_state = &crtc_state->dpll_hw_state.i9xx;
>  	enum pipe pipe = crtc->pipe;
>  
> -	intel_de_write(dev_priv, DPLL(pipe), hw_state->dpll);
> -	intel_de_posting_read(dev_priv, DPLL(pipe));
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe), hw_state->dpll);
> +	intel_de_posting_read(dev_priv, DPLL(dev_priv, pipe));
>  	udelay(150);
>  
> -	if (intel_de_wait_for_set(dev_priv, DPLL(pipe), DPLL_LOCK_VLV, 1))
> +	if (intel_de_wait_for_set(dev_priv, DPLL(dev_priv, pipe), DPLL_LOCK_VLV, 1))
>  		drm_err(&dev_priv->drm, "DPLL %d failed to lock\n", pipe);
>  }
>  
> @@ -2012,7 +2013,7 @@ void vlv_enable_pll(const struct intel_crtc_state *crtc_state)
>  	assert_pps_unlocked(dev_priv, pipe);
>  
>  	/* Enable Refclk */
> -	intel_de_write(dev_priv, DPLL(pipe),
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe),
>  		       hw_state->dpll & ~(DPLL_VCO_ENABLE | DPLL_EXT_BUFFER_ENABLE_VLV));
>  
>  	if (hw_state->dpll & DPLL_VCO_ENABLE) {
> @@ -2138,10 +2139,10 @@ static void _chv_enable_pll(const struct intel_crtc_state *crtc_state)
>  	udelay(1);
>  
>  	/* Enable PLL */
> -	intel_de_write(dev_priv, DPLL(pipe), hw_state->dpll);
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe), hw_state->dpll);
>  
>  	/* Check PLL is locked */
> -	if (intel_de_wait_for_set(dev_priv, DPLL(pipe), DPLL_LOCK_VLV, 1))
> +	if (intel_de_wait_for_set(dev_priv, DPLL(dev_priv, pipe), DPLL_LOCK_VLV, 1))
>  		drm_err(&dev_priv->drm, "PLL %d failed to lock\n", pipe);
>  }
>  
> @@ -2158,7 +2159,7 @@ void chv_enable_pll(const struct intel_crtc_state *crtc_state)
>  	assert_pps_unlocked(dev_priv, pipe);
>  
>  	/* Enable Refclk and SSC */
> -	intel_de_write(dev_priv, DPLL(pipe),
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe),
>  		       hw_state->dpll & ~DPLL_VCO_ENABLE);
>  
>  	if (hw_state->dpll & DPLL_VCO_ENABLE) {
> @@ -2183,7 +2184,7 @@ void chv_enable_pll(const struct intel_crtc_state *crtc_state)
>  		 * We should always have it disabled.
>  		 */
>  		drm_WARN_ON(&dev_priv->drm,
> -			    (intel_de_read(dev_priv, DPLL(PIPE_B)) &
> +			    (intel_de_read(dev_priv, DPLL(dev_priv, PIPE_B)) &
>  			     DPLL_VGA_MODE_DIS) == 0);
>  	} else {
>  		intel_de_write(dev_priv, DPLL_MD(pipe), hw_state->dpll_md);
> @@ -2241,8 +2242,8 @@ void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	if (pipe != PIPE_A)
>  		val |= DPLL_INTEGRATED_CRI_CLK_VLV;
>  
> -	intel_de_write(dev_priv, DPLL(pipe), val);
> -	intel_de_posting_read(dev_priv, DPLL(pipe));
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe), val);
> +	intel_de_posting_read(dev_priv, DPLL(dev_priv, pipe));
>  }
>  
>  void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> @@ -2259,8 +2260,8 @@ void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	if (pipe != PIPE_A)
>  		val |= DPLL_INTEGRATED_CRI_CLK_VLV;
>  
> -	intel_de_write(dev_priv, DPLL(pipe), val);
> -	intel_de_posting_read(dev_priv, DPLL(pipe));
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe), val);
> +	intel_de_posting_read(dev_priv, DPLL(dev_priv, pipe));
>  
>  	vlv_dpio_get(dev_priv);
>  
> @@ -2285,8 +2286,8 @@ void i9xx_disable_pll(const struct intel_crtc_state *crtc_state)
>  	/* Make sure the pipe isn't still relying on us */
>  	assert_transcoder_disabled(dev_priv, crtc_state->cpu_transcoder);
>  
> -	intel_de_write(dev_priv, DPLL(pipe), DPLL_VGA_MODE_DIS);
> -	intel_de_posting_read(dev_priv, DPLL(pipe));
> +	intel_de_write(dev_priv, DPLL(dev_priv, pipe), DPLL_VGA_MODE_DIS);
> +	intel_de_posting_read(dev_priv, DPLL(dev_priv, pipe));
>  }
>  
>  
> @@ -2312,7 +2313,7 @@ static void assert_pll(struct drm_i915_private *dev_priv,
>  {
>  	bool cur_state;
>  
> -	cur_state = intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
> +	cur_state = intel_de_read(dev_priv, DPLL(dev_priv, pipe)) & DPLL_VCO_ENABLE;
>  	I915_STATE_WARN(dev_priv, cur_state != state,
>  			"PLL state assertion failure (expected %s, current %s)\n",
>  			str_on_off(state), str_on_off(cur_state));
> diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> index 1840f5b59229..091824334f26 100644
> --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> @@ -456,13 +456,14 @@ static bool intel_dvo_init_dev(struct drm_i915_private *dev_priv,
>  	 * the device.
>  	 */
>  	for_each_pipe(dev_priv, pipe)
> -		dpll[pipe] = intel_de_rmw(dev_priv, DPLL(pipe), 0, DPLL_DVO_2X_MODE);
> +		dpll[pipe] = intel_de_rmw(dev_priv, DPLL(dev_priv, pipe), 0,
> +					  DPLL_DVO_2X_MODE);
>  
>  	ret = dvo->dev_ops->init(&intel_dvo->dev, i2c);
>  
>  	/* restore the DVO 2x clock state to original */
>  	for_each_pipe(dev_priv, pipe) {
> -		intel_de_write(dev_priv, DPLL(pipe), dpll[pipe]);
> +		intel_de_write(dev_priv, DPLL(dev_priv, pipe), dpll[pipe]);
>  	}
>  
>  	intel_gmbus_force_bit(i2c, false);
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index 73046ef58d8e..42306bc4ba86 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -119,7 +119,7 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
>  	else
>  		DP |= DP_PIPE_SEL(pipe);
>  
> -	pll_enabled = intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
> +	pll_enabled = intel_de_read(dev_priv, DPLL(dev_priv, pipe)) & DPLL_VCO_ENABLE;
>  
>  	/*
>  	 * The DPLL for the pipe must be enabled for this to work.
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6877e2f0fbc3..8ff04bb19cbe 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -668,7 +668,7 @@
>  #define _DPLL_A			0x6014
>  #define _DPLL_B			0x6018
>  #define _CHV_DPLL_C		0x6030
> -#define DPLL(pipe)		_MMIO_BASE_PIPE3(DISPLAY_MMIO_BASE(dev_priv), \
> +#define DPLL(dev_priv, pipe)		_MMIO_BASE_PIPE3(DISPLAY_MMIO_BASE(dev_priv), \
>  						 (pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C)
>  
>  #define VGA0	_MMIO(0x6000)
> -- 
> 2.39.2
>