Re: [Intel-gfx] [PATCH v2 9/9] drm/i915/display/misc: use intel_de_rmw if possible

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

On Thu, Jan 05, 2023 at 02:10:46PM +0100, Andrzej Hajda wrote:
> The helper makes the code more compact and readable.
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
>  drivers/gpu/drm/i915/display/g4x_dp.c     | 12 ++++--------
>  drivers/gpu/drm/i915/display/intel_drrs.c | 12 +++---------
>  drivers/gpu/drm/i915/display/intel_dvo.c  |  7 ++-----
>  drivers/gpu/drm/i915/display/intel_lvds.c | 12 ++++--------
>  drivers/gpu/drm/i915/display/intel_tv.c   | 18 +++++-------------
>  5 files changed, 18 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c
> index 24ef36ec2d3d3c..9629b174ec5d2c 100644
> --- a/drivers/gpu/drm/i915/display/g4x_dp.c
> +++ b/drivers/gpu/drm/i915/display/g4x_dp.c
> @@ -136,16 +136,12 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
>  
>  		intel_dp->DP |= DP_PIPE_SEL_IVB(crtc->pipe);
>  	} else if (HAS_PCH_CPT(dev_priv) && port != PORT_A) {
> -		u32 trans_dp;
> -
>  		intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
>  
> -		trans_dp = intel_de_read(dev_priv, TRANS_DP_CTL(crtc->pipe));
> -		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> -			trans_dp |= TRANS_DP_ENH_FRAMING;
> -		else
> -			trans_dp &= ~TRANS_DP_ENH_FRAMING;
> -		intel_de_write(dev_priv, TRANS_DP_CTL(crtc->pipe), trans_dp);
> +		intel_de_rmw(dev_priv, TRANS_DP_CTL(crtc->pipe),
> +			     TRANS_DP_ENH_FRAMING,
> +			     drm_dp_enhanced_frame_cap(intel_dp->dpcd) ?
> +			     TRANS_DP_ENH_FRAMING : 0);
>  	} else {
>  		if (IS_G4X(dev_priv) && pipe_config->limited_color_range)
>  			intel_dp->DP |= DP_COLOR_RANGE_16_235;
> diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c b/drivers/gpu/drm/i915/display/intel_drrs.c
> index 5b9e44443814e9..a52974f5f66042 100644
> --- a/drivers/gpu/drm/i915/display/intel_drrs.c
> +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
> @@ -68,21 +68,15 @@ intel_drrs_set_refresh_rate_pipeconf(struct intel_crtc *crtc,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum transcoder cpu_transcoder = crtc->drrs.cpu_transcoder;
> -	u32 val, bit;
> +	u32 bit;
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		bit = PIPECONF_REFRESH_RATE_ALT_VLV;
>  	else
>  		bit = PIPECONF_REFRESH_RATE_ALT_ILK;
>  
> -	val = intel_de_read(dev_priv, PIPECONF(cpu_transcoder));
> -
> -	if (refresh_rate == DRRS_REFRESH_RATE_LOW)
> -		val |= bit;
> -	else
> -		val &= ~bit;
> -
> -	intel_de_write(dev_priv, PIPECONF(cpu_transcoder), val);
> +	intel_de_rmw(dev_priv, PIPECONF(cpu_transcoder),
> +		     bit, refresh_rate == DRRS_REFRESH_RATE_LOW ? bit : 0);
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c b/drivers/gpu/drm/i915/display/intel_dvo.c
> index 4aeae0f3ac9172..77d413781020de 100644
> --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> @@ -444,11 +444,8 @@ static bool intel_dvo_init_dev(struct drm_i915_private *dev_priv,
>  	 * the clock enabled before we attempt to initialize
>  	 * the device.
>  	 */
> -	for_each_pipe(dev_priv, pipe) {
> -		dpll[pipe] = intel_de_read(dev_priv, DPLL(pipe));
> -		intel_de_write(dev_priv, DPLL(pipe),
> -			       dpll[pipe] | DPLL_DVO_2X_MODE);
> -	}
> +	for_each_pipe(dev_priv, pipe)
> +		dpll[pipe] = intel_de_rmw(dev_priv, DPLL(pipe), 0, DPLL_DVO_2X_MODE);
>  
>  	ret = dvo->dev_ops->init(&intel_dvo->dev, i2c);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
> index aecec992cd0d2d..e8f47b7ef87649 100644
> --- a/drivers/gpu/drm/i915/display/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/display/intel_lvds.c
> @@ -316,11 +316,9 @@ static void intel_enable_lvds(struct intel_atomic_state *state,
>  	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> -	intel_de_write(dev_priv, lvds_encoder->reg,
> -		       intel_de_read(dev_priv, lvds_encoder->reg) | LVDS_PORT_EN);
> +	intel_de_rmw(dev_priv, lvds_encoder->reg, 0, LVDS_PORT_EN);
>  
> -	intel_de_write(dev_priv, PP_CONTROL(0),
> -		       intel_de_read(dev_priv, PP_CONTROL(0)) | PANEL_POWER_ON);
> +	intel_de_rmw(dev_priv, PP_CONTROL(0), 0, PANEL_POWER_ON);
>  	intel_de_posting_read(dev_priv, lvds_encoder->reg);
>  
>  	if (intel_de_wait_for_set(dev_priv, PP_STATUS(0), PP_ON, 5000))
> @@ -338,14 +336,12 @@ static void intel_disable_lvds(struct intel_atomic_state *state,
>  	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  
> -	intel_de_write(dev_priv, PP_CONTROL(0),
> -		       intel_de_read(dev_priv, PP_CONTROL(0)) & ~PANEL_POWER_ON);
> +	intel_de_rmw(dev_priv, PP_CONTROL(0), PANEL_POWER_ON, 0);
>  	if (intel_de_wait_for_clear(dev_priv, PP_STATUS(0), PP_ON, 1000))
>  		drm_err(&dev_priv->drm,
>  			"timed out waiting for panel to power off\n");
>  
> -	intel_de_write(dev_priv, lvds_encoder->reg,
> -		       intel_de_read(dev_priv, lvds_encoder->reg) & ~LVDS_PORT_EN);
> +	intel_de_rmw(dev_priv, lvds_encoder->reg, LVDS_PORT_EN, 0);
>  	intel_de_posting_read(dev_priv, lvds_encoder->reg);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c
> index b986bf075889a1..e1b0034db9be66 100644
> --- a/drivers/gpu/drm/i915/display/intel_tv.c
> +++ b/drivers/gpu/drm/i915/display/intel_tv.c
> @@ -930,8 +930,7 @@ intel_enable_tv(struct intel_atomic_state *state,
>  	/* Prevents vblank waits from timing out in intel_tv_detect_type() */
>  	intel_crtc_wait_for_next_vblank(to_intel_crtc(pipe_config->uapi.crtc));
>  
> -	intel_de_write(dev_priv, TV_CTL,
> -		       intel_de_read(dev_priv, TV_CTL) | TV_ENC_ENABLE);
> +	intel_de_rmw(dev_priv, TV_CTL, 0, TV_ENC_ENABLE);
>  }
>  
>  static void
> @@ -943,8 +942,7 @@ intel_disable_tv(struct intel_atomic_state *state,
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> -	intel_de_write(dev_priv, TV_CTL,
> -		       intel_de_read(dev_priv, TV_CTL) & ~TV_ENC_ENABLE);
> +	intel_de_rmw(dev_priv, TV_CTL, TV_ENC_ENABLE, 0);
>  }
>  
>  static const struct tv_mode *intel_tv_mode_find(const struct drm_connector_state *conn_state)
> @@ -1945,15 +1943,9 @@ intel_tv_init(struct drm_i915_private *dev_priv)
>  	 * Sanity check the TV output by checking to see if the
>  	 * DAC register holds a value
>  	 */
> -	save_tv_dac = intel_de_read(dev_priv, TV_DAC);
> -
> -	intel_de_write(dev_priv, TV_DAC, save_tv_dac | TVDAC_STATE_CHG_EN);
> -	tv_dac_on = intel_de_read(dev_priv, TV_DAC);
> -
> -	intel_de_write(dev_priv, TV_DAC, save_tv_dac & ~TVDAC_STATE_CHG_EN);
> -	tv_dac_off = intel_de_read(dev_priv, TV_DAC);
> -
> -	intel_de_write(dev_priv, TV_DAC, save_tv_dac);
> +	save_tv_dac = intel_de_rmw(dev_priv, TV_DAC, 0, TVDAC_STATE_CHG_EN);
> +	tv_dac_on = intel_de_rmw(dev_priv, TV_DAC, ~0, save_tv_dac & ~TVDAC_STATE_CHG_EN);
> +	tv_dac_off = intel_de_rmw(dev_priv, TV_DAC, ~0, save_tv_dac);

this chunck got me really confused... you are now doing a lot more reads then before.
I'd say the previous approach is better and more readable than the new one...

>  
>  	/*
>  	 * If the register does not hold the state change enable
> -- 
> 2.34.1
>