Re: [Intel-gfx] [PATCH] drm/i915: use intel_uncore_rmw when appropriate

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

On Mon, Oct 17, 2022 at 12:37:56PM +0200, Andrzej Hajda wrote:
> This patch replaces all occurences of the form
> intel_uncore_write(reg, intel_uncore_read(reg) OP val)
> with intel_uncore_rmw.
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
> Apparently I have missed this pattern during refactoring.
> 
> Regards
> Andrzej
> ---
>  drivers/gpu/drm/i915/gt/intel_rps.c |   4 +-
>  drivers/gpu/drm/i915/intel_pm.c     | 190 ++++++++++------------------
>  2 files changed, 68 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> index fc23c562d9b2a7..070005dd0da476 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -625,9 +625,7 @@ static void gen5_rps_disable(struct intel_rps *rps)
>  	rgvswctl = intel_uncore_read16(uncore, MEMSWCTL);
>  
>  	/* Ack interrupts, disable EFC interrupt */
> -	intel_uncore_write(uncore, MEMINTREN,
> -			   intel_uncore_read(uncore, MEMINTREN) &
> -			   ~MEMINT_EVAL_CHG_EN);
> +	intel_uncore_rmw(uncore, MEMINTREN, MEMINT_EVAL_CHG_EN, 0);
>  	intel_uncore_write(uncore, MEMINTRSTS, MEMINT_EVAL_CHG);
>  
>  	/* Go back to the starting frequency */

Maybe split the gt stuff to a separate patch?

> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 19d4a88184d7a1..4d264147ada94b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
<snip>
> @@ -4293,14 +4272,12 @@ static void lpt_init_clock_gating(struct drm_i915_private *dev_priv)
>  	 * disabled when not needed anymore in order to save power.
>  	 */
>  	if (HAS_PCH_LPT_LP(dev_priv))
> -		intel_uncore_write(&dev_priv->uncore, SOUTH_DSPCLK_GATE_D,
> -			   intel_uncore_read(&dev_priv->uncore, SOUTH_DSPCLK_GATE_D) |
> -			   PCH_LP_PARTITION_LEVEL_DISABLE);
> +		intel_uncore_rmw(&dev_priv->uncore, SOUTH_DSPCLK_GATE_D, 0,
                                                                        ^

I'd put the newline there in these cases. That way everything we're
doing to the register value would be neatly on the same line instead
of spread around like this.

> +				 PCH_LP_PARTITION_LEVEL_DISABLE);
>  
>  	/* WADPOClockGatingDisable:hsw */
> -	intel_uncore_write(&dev_priv->uncore, TRANS_CHICKEN1(PIPE_A),
> -		   intel_uncore_read(&dev_priv->uncore, TRANS_CHICKEN1(PIPE_A)) |
> -		   TRANS_CHICKEN1_DP0UNIT_GC_DISABLE);
> +	intel_uncore_rmw(&dev_priv->uncore, TRANS_CHICKEN1(PIPE_A), 0,
> +			 TRANS_CHICKEN1_DP0UNIT_GC_DISABLE);
>  }
>  
>  static void lpt_suspend_hw(struct drm_i915_private *dev_priv)
<snip>
> @@ -4532,43 +4494,37 @@ static void bdw_init_clock_gating(struct drm_i915_private *dev_priv)
>  	enum pipe pipe;
>  
>  	/* WaFbcAsynchFlipDisableFbcQueue:hsw,bdw */
> -	intel_uncore_write(&dev_priv->uncore, CHICKEN_PIPESL_1(PIPE_A),
> -		   intel_uncore_read(&dev_priv->uncore, CHICKEN_PIPESL_1(PIPE_A)) |
> -		   HSW_FBCQ_DIS);
> +	intel_uncore_rmw(&dev_priv->uncore, CHICKEN_PIPESL_1(PIPE_A), 0, HSW_FBCQ_DIS);
>  
>  	/* WaSwitchSolVfFArbitrationPriority:bdw */
> -	intel_uncore_write(&dev_priv->uncore, GAM_ECOCHK, intel_uncore_read(&dev_priv->uncore, GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
> +	intel_uncore_rmw(&dev_priv->uncore, GAM_ECOCHK, 0, HSW_ECOCHK_ARB_PRIO_SOL);
>  
>  	/* WaPsrDPAMaskVBlankInSRD:bdw */
> -	intel_uncore_write(&dev_priv->uncore, CHICKEN_PAR1_1,
> -		   intel_uncore_read(&dev_priv->uncore, CHICKEN_PAR1_1) | DPA_MASK_VBLANK_SRD);
> +	intel_uncore_rmw(&dev_priv->uncore, CHICKEN_PAR1_1, 0, DPA_MASK_VBLANK_SRD);
>  
>  	for_each_pipe(dev_priv, pipe) {
>  		/* WaPsrDPRSUnmaskVBlankInSRD:bdw */
> -		intel_uncore_write(&dev_priv->uncore, CHICKEN_PIPESL_1(pipe),
> -			   intel_uncore_read(&dev_priv->uncore, CHICKEN_PIPESL_1(pipe)) |
> -			   BDW_DPRS_MASK_VBLANK_SRD);
> +		intel_uncore_rmw(&dev_priv->uncore, CHICKEN_PIPESL_1(pipe), 0,
> +				 BDW_DPRS_MASK_VBLANK_SRD);
>  	}
>  
>  	/* WaVSRefCountFullforceMissDisable:bdw */
>  	/* WaDSRefCountFullforceMissDisable:bdw */
> -	intel_uncore_write(&dev_priv->uncore, GEN7_FF_THREAD_MODE,
> -		   intel_uncore_read(&dev_priv->uncore, GEN7_FF_THREAD_MODE) &
> -		   ~(GEN8_FF_DS_REF_CNT_FFME | GEN7_FF_VS_REF_CNT_FFME));
> +	intel_uncore_rmw(&dev_priv->uncore, GEN7_FF_THREAD_MODE,
> +			 (GEN8_FF_DS_REF_CNT_FFME | GEN7_FF_VS_REF_CNT_FFME), 0);

Useless parens there. Ditto in the other copy.

This stuff really doesn't belong here anyway. I thought someone would have
hoisted all the gt stuff into a more appropriate place by now. But I guess
not.

Rest of the patch looked ok to me.

-- 
Ville Syrjälä
Intel