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

[Andrzej Hajda <andrzej.hajda@xxxxxxxxx>]

On 09.01.2023 20:27, Rodrigo Vivi wrote:
> 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@xxxxxxxxx>
> > ---
> >   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.

Every read/write is replaced by exactly one rmw, nothing added, simple 
proof by reading the lines: previously 3 reads, 3 writes, now 3 rmw :)

> I'd say the previous approach is better and more readable than the new one...

You are right, even if functionally it is the same, the previous code 
better empahises we check if writes are effective.
I will send v3 without this chunk.

Regards
Andrzej

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