Re: [PATCH 2/2] drm/i915: Convert i915_reset.c over to using uncore mmio

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Em sex, 2019-04-05 às 19:57 +0100, Chris Wilson escreveu:
> Currently i915_reset.c mixes calls to intel_uncore, pci and our old
> style I915_READ mmio interfaces. Cast aside the old implicit macros,
> and harmonise on using uncore throughout.
> 
> add/remove: 1/1 grow/shrink: 0/4 up/down: 65/-207 (-142)
> Function                                     old     new   delta
> rmw_register                                   -      65     +65
> gen8_reset_engines                           945     942      -3
> g4x_do_reset                                 407     376     -31
> intel_gpu_reset                              545     509     -36
> clear_register                                63       -     -63
> i915_clear_error_registers                   461     387     -74
> 
> A little bit of pointer dancing elimination works wonders.
> 
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_reset.c | 125 +++++++++++++++++-------------
>  1 file changed, 73 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index d44dc8422e8c..f50534a833ca 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -18,6 +18,28 @@
>  /* XXX How to handle concurrent GGTT updates using tiling registers? */
>  #define RESET_UNDER_STOP_MACHINE 0
>  
> +static void rmw_register(struct intel_uncore *uncore,
> +			 i915_reg_t reg, u32 clr, u32 set)
> +{
> +	u32 val;
> +
> +	val = intel_uncore_read(uncore, reg);
> +	val &= ~clr;
> +	val |= set;
> +	intel_uncore_write(uncore, reg, val);
> +}
> +
> +static void rmw_register_fw(struct intel_uncore *uncore,
> +			    i915_reg_t reg, u32 clr, u32 set)
> +{
> +	u32 val;
> +
> +	val = intel_uncore_read_fw(uncore, reg);
> +	val &= ~clr;
> +	val |= set;
> +	intel_uncore_write_fw(uncore, reg, val);
> +}

I like the idea here, maybe we could expose these concepts to the whole
driver.

But one thing I did notice is that in all of the calls there's an
argument that's zero. It's easy to get confused on the set/clear
parameter order and make a mistake. Also, when reading the code we have
to keep remembering which one comes first. Perhaps some small wrappers
for the cases where we only set or clear something (currently, 100% of
the cases) would make the code even more readable:

static inline void rmw_register_set(uncore, reg, bits_to_set)
{
	rmw_register(uncore, reg, 0, bits);
}

static inline void rmw_register_clear(uncore, reg, bits_to_clear)
{
	rmw_register(uncore, reg, bits, 0);
}

IMHO that would make the code much more comfortable to read. But maybe
that's because my brain is just too small.



> +
>  static void engine_skip_context(struct i915_request *rq)
>  {
>  	struct intel_engine_cs *engine = rq->engine;
> @@ -119,7 +141,7 @@ void i915_reset_request(struct i915_request *rq, bool guilty)
>  
>  static void gen3_stop_engine(struct intel_engine_cs *engine)
>  {
> -	struct drm_i915_private *dev_priv = engine->i915;
> +	struct intel_uncore *uncore = engine->uncore;
>  	const u32 base = engine->mmio_base;
>  
>  	GEM_TRACE("%s\n", engine->name);
> @@ -127,20 +149,23 @@ static void gen3_stop_engine(struct intel_engine_cs *engine)
>  	if (intel_engine_stop_cs(engine))
>  		GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
>  
> -	I915_WRITE_FW(RING_HEAD(base), I915_READ_FW(RING_TAIL(base)));
> -	POSTING_READ_FW(RING_HEAD(base)); /* paranoia */
> +	intel_uncore_write_fw(uncore,
> +			      RING_HEAD(base),
> +			      intel_uncore_read_fw(uncore, RING_TAIL(base)));
> +	intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
>  
> -	I915_WRITE_FW(RING_HEAD(base), 0);
> -	I915_WRITE_FW(RING_TAIL(base), 0);
> -	POSTING_READ_FW(RING_TAIL(base));
> +	intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
> +	intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
> +	intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
>  
>  	/* The ring must be empty before it is disabled */
> -	I915_WRITE_FW(RING_CTL(base), 0);
> +	intel_uncore_write_fw(uncore, RING_CTL(base), 0);
>  
>  	/* Check acts as a post */
> -	if (I915_READ_FW(RING_HEAD(base)))
> +	if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
>  		GEM_TRACE("%s: ring head [%x] not parked\n",
> -			  engine->name, I915_READ_FW(RING_HEAD(base)));
> +			  engine->name,
> +			  intel_uncore_read_fw(uncore, RING_HEAD(base)));
>  }
>  
>  static void i915_stop_engines(struct drm_i915_private *i915,
> @@ -203,17 +228,17 @@ static int g33_do_reset(struct drm_i915_private *i915,
>  	return wait_for_atomic(g4x_reset_complete(pdev), 50);
>  }
>  
> -static int g4x_do_reset(struct drm_i915_private *dev_priv,
> +static int g4x_do_reset(struct drm_i915_private *i915,
>  			intel_engine_mask_t engine_mask,
>  			unsigned int retry)
>  {
> -	struct pci_dev *pdev = dev_priv->drm.pdev;
> +	struct pci_dev *pdev = i915->drm.pdev;
> +	struct intel_uncore *uncore = &i915->uncore;
>  	int ret;
>  
>  	/* WaVcpClkGateDisableForMediaReset:ctg,elk */
> -	I915_WRITE_FW(VDECCLK_GATE_D,
> -		      I915_READ(VDECCLK_GATE_D) | VCP_UNIT_CLOCK_GATE_DISABLE);
> -	POSTING_READ_FW(VDECCLK_GATE_D);
> +	rmw_register_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,
>  			      GRDOM_MEDIA | GRDOM_RESET_ENABLE);
> @@ -234,18 +259,17 @@ static int g4x_do_reset(struct drm_i915_private *dev_priv,
>  out:
>  	pci_write_config_byte(pdev, I915_GDRST, 0);
>  
> -	I915_WRITE_FW(VDECCLK_GATE_D,
> -		      I915_READ(VDECCLK_GATE_D) & ~VCP_UNIT_CLOCK_GATE_DISABLE);
> -	POSTING_READ_FW(VDECCLK_GATE_D);
> +	rmw_register_fw(uncore, VDECCLK_GATE_D, VCP_UNIT_CLOCK_GATE_DISABLE, 0);
> +	intel_uncore_posting_read_fw(uncore, VDECCLK_GATE_D);
>  
>  	return ret;
>  }
>  
> -static int ironlake_do_reset(struct drm_i915_private *dev_priv,
> +static int ironlake_do_reset(struct drm_i915_private *i915,
>  			     intel_engine_mask_t engine_mask,
>  			     unsigned int retry)
>  {
> -	struct intel_uncore *uncore = &dev_priv->uncore;
> +	struct intel_uncore *uncore = &i915->uncore;
>  	int ret;
>  
>  	intel_uncore_write_fw(uncore, ILK_GDSR,
> @@ -277,10 +301,10 @@ static int ironlake_do_reset(struct drm_i915_private *dev_priv,
>  }
>  
>  /* Reset the hardware domains (GENX_GRDOM_*) specified by mask */
> -static int gen6_hw_domain_reset(struct drm_i915_private *dev_priv,
> +static int gen6_hw_domain_reset(struct drm_i915_private *i915,
>  				u32 hw_domain_mask)
>  {
> -	struct intel_uncore *uncore = &dev_priv->uncore;
> +	struct intel_uncore *uncore = &i915->uncore;
>  	int err;
>  
>  	/*
> @@ -381,7 +405,7 @@ static u32 gen11_lock_sfc(struct intel_engine_cs *engine)
>  	 * ends up being locked to the engine we want to reset, we have to reset
>  	 * it as well (we will unlock it once the reset sequence is completed).
>  	 */
> -	intel_uncore_rmw_or_fw(uncore, sfc_forced_lock, sfc_forced_lock_bit);

We killed the only caller for that, might as well kill the definition.
Btw this name is (was) very confusing to me.



> +	rmw_register_fw(uncore, sfc_forced_lock, 0, sfc_forced_lock_bit);
>  
>  	if (__intel_wait_for_register_fw(uncore,
>  					 sfc_forced_lock_ack,
> @@ -404,7 +428,6 @@ static void gen11_unlock_sfc(struct intel_engine_cs *engine)
>  	u8 vdbox_sfc_access = RUNTIME_INFO(engine->i915)->vdbox_sfc_access;
>  	i915_reg_t sfc_forced_lock;
>  	u32 sfc_forced_lock_bit;
> -	u32 val;
>  
>  	switch (engine->class) {
>  	case VIDEO_DECODE_CLASS:
> @@ -424,9 +447,7 @@ static void gen11_unlock_sfc(struct intel_engine_cs *engine)
>  		return;
>  	}
>  
> -	val = intel_uncore_read_fw(uncore, sfc_forced_lock);
> -	val &= ~sfc_forced_lock_bit;
> -	intel_uncore_write_fw(uncore, sfc_forced_lock, val);
> +	rmw_register_fw(uncore, sfc_forced_lock, sfc_forced_lock_bit, 0);
>  }
>  
>  static int gen11_reset_engines(struct drm_i915_private *i915,
> @@ -491,10 +512,9 @@ static int gen8_engine_reset_prepare(struct intel_engine_cs *engine)
>  
>  static void gen8_engine_reset_cancel(struct intel_engine_cs *engine)
>  {
> -	struct drm_i915_private *dev_priv = engine->i915;
> -
> -	I915_WRITE_FW(RING_RESET_CTL(engine->mmio_base),
> -		      _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
> +	intel_uncore_write_fw(engine->uncore,
> +			      RING_RESET_CTL(engine->mmio_base),
> +			      _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
>  }
>  
>  static int gen8_reset_engines(struct drm_i915_private *i915,
> @@ -1178,49 +1198,50 @@ static void i915_reset_device(struct drm_i915_private *i915,
>  		kobject_uevent_env(kobj, KOBJ_CHANGE, reset_done_event);
>  }
>  
> -static void clear_register(struct drm_i915_private *dev_priv, i915_reg_t reg)
> +static void clear_register(struct intel_uncore *uncore, i915_reg_t reg)
>  {
> -	I915_WRITE(reg, I915_READ(reg));
> +	rmw_register(uncore, reg, 0, 0);
>  }
>  
> -void i915_clear_error_registers(struct drm_i915_private *dev_priv)
> +void i915_clear_error_registers(struct drm_i915_private *i915)
>  {
> +	struct intel_uncore *uncore = &i915->uncore;
>  	u32 eir;
>  
> -	if (!IS_GEN(dev_priv, 2))
> -		clear_register(dev_priv, PGTBL_ER);
> +	if (!IS_GEN(i915, 2))
> +		clear_register(uncore, PGTBL_ER);
>  
> -	if (INTEL_GEN(dev_priv) < 4)
> -		clear_register(dev_priv, IPEIR(RENDER_RING_BASE));
> +	if (INTEL_GEN(i915) < 4)
> +		clear_register(uncore, IPEIR(RENDER_RING_BASE));
>  	else
> -		clear_register(dev_priv, IPEIR_I965);
> +		clear_register(uncore, IPEIR_I965);
>  
> -	clear_register(dev_priv, EIR);
> -	eir = I915_READ(EIR);
> +	clear_register(uncore, EIR);
> +	eir = intel_uncore_read(uncore, EIR);
>  	if (eir) {
>  		/*
>  		 * some errors might have become stuck,
>  		 * mask them.
>  		 */
>  		DRM_DEBUG_DRIVER("EIR stuck: 0x%08x, masking\n", eir);
> -		I915_WRITE(EMR, I915_READ(EMR) | eir);
> -		I915_WRITE(IIR, I915_MASTER_ERROR_INTERRUPT);
> +		rmw_register(uncore, EMR, 0, eir);
> +		intel_uncore_write(uncore, IIR, I915_MASTER_ERROR_INTERRUPT);
>  	}
>  
> -	if (INTEL_GEN(dev_priv) >= 8) {
> -		I915_WRITE(GEN8_RING_FAULT_REG,
> -			   I915_READ(GEN8_RING_FAULT_REG) & ~RING_FAULT_VALID);
> -		POSTING_READ(GEN8_RING_FAULT_REG);
> -	} else if (INTEL_GEN(dev_priv) >= 6) {
> +	if (INTEL_GEN(i915) >= 8) {
> +		rmw_register(uncore, GEN8_RING_FAULT_REG, RING_FAULT_VALID, 0);
> +		intel_uncore_posting_read(uncore, GEN8_RING_FAULT_REG);
> +	} else if (INTEL_GEN(i915) >= 6) {
>  		struct intel_engine_cs *engine;
>  		enum intel_engine_id id;
>  
> -		for_each_engine(engine, dev_priv, id) {
> -			I915_WRITE(RING_FAULT_REG(engine),
> -				   I915_READ(RING_FAULT_REG(engine)) &
> -				   ~RING_FAULT_VALID);
> +		for_each_engine(engine, i915, id) {
> +			rmw_register(uncore,
> +				     RING_FAULT_REG(engine),
> +				     RING_FAULT_VALID, 0);
> +			intel_uncore_posting_read(uncore,
> +						  RING_FAULT_REG(engine));
>  		}
> -		POSTING_READ(RING_FAULT_REG(dev_priv->engine[RCS0]));

That's a small behavior change, I'll assume it's on purpose.

With or without changes (but I'd really love to see the clr/set
wrappers):
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>

(r-b on patch 1 also stands in the newer version)

>  	}
>  }
>  

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux