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 <[email protected]>
> ---
> 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
