Re: [Intel-gfx] [PATCH] drm/i915/gt: remove some limited use register access wrappers

2022-11-28 Thread Andrzej Hajda 

On 23.11.2022 17:49, Jani Nikula wrote:

Remove rmw_set(), rmw_clear(), clear_register(), rmw_set_fw(), and
rmw_clear_fw(). They're just one too many levels of abstraction for
register access, for very specific purposes.

clear_register() seems like a micro-optimization bypassing the write
when the register is already clear, but that trick has ceased to work
since commit 06b975d58fd6 ("drm/i915: make intel_uncore_rmw() write
unconditionally"). Just clear the register in the most obvious way.

Signed-off-by: Jani Nikula 


Reviewed-by: Andrzej Hajda 

Regards
Andrzej

Re: [Intel-gfx] [PATCH] drm/i915/gt: remove some limited use register access wrappers

2022-11-28 Thread Matt Roper 
On Wed, Nov 23, 2022 at 06:49:16PM +0200, Jani Nikula wrote:
> Remove rmw_set(), rmw_clear(), clear_register(), rmw_set_fw(), and
> rmw_clear_fw(). They're just one too many levels of abstraction for
> register access, for very specific purposes.
> 
> clear_register() seems like a micro-optimization bypassing the write
> when the register is already clear, but that trick has ceased to work
> since commit 06b975d58fd6 ("drm/i915: make intel_uncore_rmw() write
> unconditionally"). Just clear the register in the most obvious way.
> 
> Signed-off-by: Jani Nikula 

There's also set() in intel_rc6.c that could be dropped as a follow-up.

Reviewed-by: Matt Roper 

> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c| 29 +++
>  drivers/gpu/drm/i915/gt/intel_reset.c | 18 -
>  2 files changed, 11 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index b5ad9caa5537..efd9d722d77f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -210,21 +210,6 @@ int intel_gt_init_hw(struct intel_gt *gt)
>   return ret;
>  }
>  
> -static void rmw_set(struct intel_uncore *uncore, i915_reg_t reg, u32 set)
> -{
> - intel_uncore_rmw(uncore, reg, 0, set);
> -}
> -
> -static void rmw_clear(struct intel_uncore *uncore, i915_reg_t reg, u32 clr)
> -{
> - intel_uncore_rmw(uncore, reg, clr, 0);
> -}
> -
> -static void clear_register(struct intel_uncore *uncore, i915_reg_t reg)
> -{
> - intel_uncore_rmw(uncore, reg, 0, 0);
> -}
> -
>  static void gen6_clear_engine_error_register(struct intel_engine_cs *engine)
>  {
>   GEN6_RING_FAULT_REG_RMW(engine, RING_FAULT_VALID, 0);
> @@ -250,14 +235,14 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
>   u32 eir;
>  
>   if (GRAPHICS_VER(i915) != 2)
> - clear_register(uncore, PGTBL_ER);
> + intel_uncore_write(uncore, PGTBL_ER, 0);
>  
>   if (GRAPHICS_VER(i915) < 4)
> - clear_register(uncore, IPEIR(RENDER_RING_BASE));
> + intel_uncore_write(uncore, IPEIR(RENDER_RING_BASE), 0);
>   else
> - clear_register(uncore, IPEIR_I965);
> + intel_uncore_write(uncore, IPEIR_I965, 0);
>  
> - clear_register(uncore, EIR);
> + intel_uncore_write(uncore, EIR, 0);
>   eir = intel_uncore_read(uncore, EIR);
>   if (eir) {
>   /*
> @@ -265,7 +250,7 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
>* mask them.
>*/
>   drm_dbg(>i915->drm, "EIR stuck: 0x%08x, masking\n", eir);
> - rmw_set(uncore, EMR, eir);
> + intel_uncore_rmw(uncore, EMR, 0, eir);
>   intel_uncore_write(uncore, GEN2_IIR,
>  I915_MASTER_ERROR_INTERRUPT);
>   }
> @@ -275,10 +260,10 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
>  RING_FAULT_VALID, 0);
>   intel_gt_mcr_read_any(gt, XEHP_RING_FAULT_REG);
>   } else if (GRAPHICS_VER(i915) >= 12) {
> - rmw_clear(uncore, GEN12_RING_FAULT_REG, RING_FAULT_VALID);
> + intel_uncore_rmw(uncore, GEN12_RING_FAULT_REG, 
> RING_FAULT_VALID, 0);
>   intel_uncore_posting_read(uncore, GEN12_RING_FAULT_REG);
>   } else if (GRAPHICS_VER(i915) >= 8) {
> - rmw_clear(uncore, GEN8_RING_FAULT_REG, RING_FAULT_VALID);
> + intel_uncore_rmw(uncore, GEN8_RING_FAULT_REG, RING_FAULT_VALID, 
> 0);
>   intel_uncore_posting_read(uncore, GEN8_RING_FAULT_REG);
>   } else if (GRAPHICS_VER(i915) >= 6) {
>   struct intel_engine_cs *engine;
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c 
> b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 24736ebee17c..ffde89c5835a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -35,16 +35,6 @@
>  /* XXX How to handle concurrent GGTT updates using tiling registers? */
>  #define RESET_UNDER_STOP_MACHINE 0
>  
> -static void rmw_set_fw(struct intel_uncore *uncore, i915_reg_t reg, u32 set)
> -{
> - intel_uncore_rmw_fw(uncore, reg, 0, set);
> -}
> -
> -static void rmw_clear_fw(struct intel_uncore *uncore, i915_reg_t reg, u32 
> clr)
> -{
> - intel_uncore_rmw_fw(uncore, reg, clr, 0);
> -}
> -
>  static void client_mark_guilty(struct i915_gem_context *ctx, bool banned)
>  {
>   struct drm_i915_file_private *file_priv = ctx->file_priv;
> @@ -212,7 +202,7 @@ static int g4x_do_reset(struct intel_gt *gt,
>   int ret;
>  
>   /* WaVcpClkGateDisableForMediaReset:ctg,elk */
> - rmw_set_fw(uncore, VDECCLK_GATE_D, VCP_UNIT_CLOCK_GATE_DISABLE);
> + intel_uncore_rmw_fw(uncore, VDECCLK_GATE_D, 0, 
> VCP_UNIT_CLOCK_GATE_DISABLE);
>   intel_uncore_posting_read_fw(uncore, VDECCLK_GATE_D);
>  
>   pci_write_config_byte(pdev, I915_GDRST,
> @@ -234,7 +224,7 @@ static int

Re: [Intel-gfx] [PATCH] drm/i915/gt: remove some limited use register access wrappers

2022-11-23 Thread Rodrigo Vivi 
On Wed, Nov 23, 2022 at 06:49:16PM +0200, Jani Nikula wrote:
> Remove rmw_set(), rmw_clear(), clear_register(), rmw_set_fw(), and
> rmw_clear_fw(). They're just one too many levels of abstraction for
> register access, for very specific purposes.
> 
> clear_register() seems like a micro-optimization bypassing the write
> when the register is already clear, but that trick has ceased to work
> since commit 06b975d58fd6 ("drm/i915: make intel_uncore_rmw() write
> unconditionally"). Just clear the register in the most obvious way.
> 
> Signed-off-by: Jani Nikula 

Reviewed-by: Rodrigo Vivi 
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c| 29 +++
>  drivers/gpu/drm/i915/gt/intel_reset.c | 18 -
>  2 files changed, 11 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index b5ad9caa5537..efd9d722d77f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -210,21 +210,6 @@ int intel_gt_init_hw(struct intel_gt *gt)
>   return ret;
>  }
>  
> -static void rmw_set(struct intel_uncore *uncore, i915_reg_t reg, u32 set)
> -{
> - intel_uncore_rmw(uncore, reg, 0, set);
> -}
> -
> -static void rmw_clear(struct intel_uncore *uncore, i915_reg_t reg, u32 clr)
> -{
> - intel_uncore_rmw(uncore, reg, clr, 0);
> -}
> -
> -static void clear_register(struct intel_uncore *uncore, i915_reg_t reg)
> -{
> - intel_uncore_rmw(uncore, reg, 0, 0);
> -}
> -
>  static void gen6_clear_engine_error_register(struct intel_engine_cs *engine)
>  {
>   GEN6_RING_FAULT_REG_RMW(engine, RING_FAULT_VALID, 0);
> @@ -250,14 +235,14 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
>   u32 eir;
>  
>   if (GRAPHICS_VER(i915) != 2)
> - clear_register(uncore, PGTBL_ER);
> + intel_uncore_write(uncore, PGTBL_ER, 0);
>  
>   if (GRAPHICS_VER(i915) < 4)
> - clear_register(uncore, IPEIR(RENDER_RING_BASE));
> + intel_uncore_write(uncore, IPEIR(RENDER_RING_BASE), 0);
>   else
> - clear_register(uncore, IPEIR_I965);
> + intel_uncore_write(uncore, IPEIR_I965, 0);
>  
> - clear_register(uncore, EIR);
> + intel_uncore_write(uncore, EIR, 0);
>   eir = intel_uncore_read(uncore, EIR);
>   if (eir) {
>   /*
> @@ -265,7 +250,7 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
>* mask them.
>*/
>   drm_dbg(>i915->drm, "EIR stuck: 0x%08x, masking\n", eir);
> - rmw_set(uncore, EMR, eir);
> + intel_uncore_rmw(uncore, EMR, 0, eir);
>   intel_uncore_write(uncore, GEN2_IIR,
>  I915_MASTER_ERROR_INTERRUPT);
>   }
> @@ -275,10 +260,10 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
>  RING_FAULT_VALID, 0);
>   intel_gt_mcr_read_any(gt, XEHP_RING_FAULT_REG);
>   } else if (GRAPHICS_VER(i915) >= 12) {
> - rmw_clear(uncore, GEN12_RING_FAULT_REG, RING_FAULT_VALID);
> + intel_uncore_rmw(uncore, GEN12_RING_FAULT_REG, 
> RING_FAULT_VALID, 0);
>   intel_uncore_posting_read(uncore, GEN12_RING_FAULT_REG);
>   } else if (GRAPHICS_VER(i915) >= 8) {
> - rmw_clear(uncore, GEN8_RING_FAULT_REG, RING_FAULT_VALID);
> + intel_uncore_rmw(uncore, GEN8_RING_FAULT_REG, RING_FAULT_VALID, 
> 0);
>   intel_uncore_posting_read(uncore, GEN8_RING_FAULT_REG);
>   } else if (GRAPHICS_VER(i915) >= 6) {
>   struct intel_engine_cs *engine;
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c 
> b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 24736ebee17c..ffde89c5835a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -35,16 +35,6 @@
>  /* XXX How to handle concurrent GGTT updates using tiling registers? */
>  #define RESET_UNDER_STOP_MACHINE 0
>  
> -static void rmw_set_fw(struct intel_uncore *uncore, i915_reg_t reg, u32 set)
> -{
> - intel_uncore_rmw_fw(uncore, reg, 0, set);
> -}
> -
> -static void rmw_clear_fw(struct intel_uncore *uncore, i915_reg_t reg, u32 
> clr)
> -{
> - intel_uncore_rmw_fw(uncore, reg, clr, 0);
> -}
> -
>  static void client_mark_guilty(struct i915_gem_context *ctx, bool banned)
>  {
>   struct drm_i915_file_private *file_priv = ctx->file_priv;
> @@ -212,7 +202,7 @@ static int g4x_do_reset(struct intel_gt *gt,
>   int ret;
>  
>   /* WaVcpClkGateDisableForMediaReset:ctg,elk */
> - rmw_set_fw(uncore, VDECCLK_GATE_D, VCP_UNIT_CLOCK_GATE_DISABLE);
> + intel_uncore_rmw_fw(uncore, VDECCLK_GATE_D, 0, 
> VCP_UNIT_CLOCK_GATE_DISABLE);
>   intel_uncore_posting_read_fw(uncore, VDECCLK_GATE_D);
>  
>   pci_write_config_byte(pdev, I915_GDRST,
> @@ -234,7 +224,7 @@ static int g4x_do_reset(struct intel_gt *gt,
>  out:
>

[Intel-gfx] [PATCH] drm/i915/gt: remove some limited use register access wrappers

2022-11-23 Thread Jani Nikula 
Remove rmw_set(), rmw_clear(), clear_register(), rmw_set_fw(), and
rmw_clear_fw(). They're just one too many levels of abstraction for
register access, for very specific purposes.

clear_register() seems like a micro-optimization bypassing the write
when the register is already clear, but that trick has ceased to work
since commit 06b975d58fd6 ("drm/i915: make intel_uncore_rmw() write
unconditionally"). Just clear the register in the most obvious way.

Signed-off-by: Jani Nikula 
---
 drivers/gpu/drm/i915/gt/intel_gt.c| 29 +++
 drivers/gpu/drm/i915/gt/intel_reset.c | 18 -
 2 files changed, 11 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
b/drivers/gpu/drm/i915/gt/intel_gt.c
index b5ad9caa5537..efd9d722d77f 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -210,21 +210,6 @@ int intel_gt_init_hw(struct intel_gt *gt)
return ret;
 }
 
-static void rmw_set(struct intel_uncore *uncore, i915_reg_t reg, u32 set)
-{
-   intel_uncore_rmw(uncore, reg, 0, set);
-}
-
-static void rmw_clear(struct intel_uncore *uncore, i915_reg_t reg, u32 clr)
-{
-   intel_uncore_rmw(uncore, reg, clr, 0);
-}
-
-static void clear_register(struct intel_uncore *uncore, i915_reg_t reg)
-{
-   intel_uncore_rmw(uncore, reg, 0, 0);
-}
-
 static void gen6_clear_engine_error_register(struct intel_engine_cs *engine)
 {
GEN6_RING_FAULT_REG_RMW(engine, RING_FAULT_VALID, 0);
@@ -250,14 +235,14 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
u32 eir;
 
if (GRAPHICS_VER(i915) != 2)
-   clear_register(uncore, PGTBL_ER);
+   intel_uncore_write(uncore, PGTBL_ER, 0);
 
if (GRAPHICS_VER(i915) < 4)
-   clear_register(uncore, IPEIR(RENDER_RING_BASE));
+   intel_uncore_write(uncore, IPEIR(RENDER_RING_BASE), 0);
else
-   clear_register(uncore, IPEIR_I965);
+   intel_uncore_write(uncore, IPEIR_I965, 0);
 
-   clear_register(uncore, EIR);
+   intel_uncore_write(uncore, EIR, 0);
eir = intel_uncore_read(uncore, EIR);
if (eir) {
/*
@@ -265,7 +250,7 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
 * mask them.
 */
drm_dbg(>i915->drm, "EIR stuck: 0x%08x, masking\n", eir);
-   rmw_set(uncore, EMR, eir);
+   intel_uncore_rmw(uncore, EMR, 0, eir);
intel_uncore_write(uncore, GEN2_IIR,
   I915_MASTER_ERROR_INTERRUPT);
}
@@ -275,10 +260,10 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
   RING_FAULT_VALID, 0);
intel_gt_mcr_read_any(gt, XEHP_RING_FAULT_REG);
} else if (GRAPHICS_VER(i915) >= 12) {
-   rmw_clear(uncore, GEN12_RING_FAULT_REG, RING_FAULT_VALID);
+   intel_uncore_rmw(uncore, GEN12_RING_FAULT_REG, 
RING_FAULT_VALID, 0);
intel_uncore_posting_read(uncore, GEN12_RING_FAULT_REG);
} else if (GRAPHICS_VER(i915) >= 8) {
-   rmw_clear(uncore, GEN8_RING_FAULT_REG, RING_FAULT_VALID);
+   intel_uncore_rmw(uncore, GEN8_RING_FAULT_REG, RING_FAULT_VALID, 
0);
intel_uncore_posting_read(uncore, GEN8_RING_FAULT_REG);
} else if (GRAPHICS_VER(i915) >= 6) {
struct intel_engine_cs *engine;
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c 
b/drivers/gpu/drm/i915/gt/intel_reset.c
index 24736ebee17c..ffde89c5835a 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -35,16 +35,6 @@
 /* XXX How to handle concurrent GGTT updates using tiling registers? */
 #define RESET_UNDER_STOP_MACHINE 0
 
-static void rmw_set_fw(struct intel_uncore *uncore, i915_reg_t reg, u32 set)
-{
-   intel_uncore_rmw_fw(uncore, reg, 0, set);
-}
-
-static void rmw_clear_fw(struct intel_uncore *uncore, i915_reg_t reg, u32 clr)
-{
-   intel_uncore_rmw_fw(uncore, reg, clr, 0);
-}
-
 static void client_mark_guilty(struct i915_gem_context *ctx, bool banned)
 {
struct drm_i915_file_private *file_priv = ctx->file_priv;
@@ -212,7 +202,7 @@ static int g4x_do_reset(struct intel_gt *gt,
int ret;
 
/* WaVcpClkGateDisableForMediaReset:ctg,elk */
-   rmw_set_fw(uncore, VDECCLK_GATE_D, VCP_UNIT_CLOCK_GATE_DISABLE);
+   intel_uncore_rmw_fw(uncore, VDECCLK_GATE_D, 0, 
VCP_UNIT_CLOCK_GATE_DISABLE);
intel_uncore_posting_read_fw(uncore, VDECCLK_GATE_D);
 
pci_write_config_byte(pdev, I915_GDRST,
@@ -234,7 +224,7 @@ static int g4x_do_reset(struct intel_gt *gt,
 out:
pci_write_config_byte(pdev, I915_GDRST, 0);
 
-   rmw_clear_fw(uncore, VDECCLK_GATE_D, VCP_UNIT_CLOCK_GATE_DISABLE);
+   intel_uncore_rmw_fw(uncore, VDECCLK_GATE_D, 
VCP_UNIT_CLOCK_GATE_DISABLE, 0);
intel_uncore_posting_read_fw(uncore,