[RFC,1/3] drm/i915: split out uncore_mmio_debug
diff mbox series

Message ID 20190808014423.20377-2-daniele.ceraolospurio@intel.com
State New
Headers show
Series
  • Display uncore
Related show

Commit Message

Daniele Ceraolo Spurio Aug. 8, 2019, 1:44 a.m. UTC
Multiple uncore structures will share the debug infrastructure, so
move it to a common place and add extra locking around it.
Also, since we now have a separate object, it is cleaner to have
dedicated functions working on the object to stop and restart the
mmio debug. Apart from the cosmetic changes, this patch introduces
2 functional updates:

- All calls to check_for_unclaimed_mmio will now return false when
  the debug is suspended, not just the ones that are active only when
  i915_modparams.mmio_debug is set. If we don't trust the result of the
  check while a user is doing mmio access then we shouldn't attempt the
  check anywhere.

- i915_modparams.mmio_debug is not save/restored anymore around user
  access. The value is now never touched by the kernel while debug is
  disabled so no need for save/restore.

v2: squash mmio_debug patches, restrict mmio_debug lock usage (Chris)

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
 drivers/gpu/drm/i915/i915_drv.c     |  3 +-
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_uncore.c | 91 ++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_uncore.h | 18 +++---
 5 files changed, 79 insertions(+), 36 deletions(-)

Comments

Mika Kuoppala Aug. 8, 2019, 1:08 p.m. UTC | #1
Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> writes:

> Multiple uncore structures will share the debug infrastructure, so
> move it to a common place and add extra locking around it.
> Also, since we now have a separate object, it is cleaner to have
> dedicated functions working on the object to stop and restart the
> mmio debug. Apart from the cosmetic changes, this patch introduces
> 2 functional updates:
>
> - All calls to check_for_unclaimed_mmio will now return false when
>   the debug is suspended, not just the ones that are active only when
>   i915_modparams.mmio_debug is set. If we don't trust the result of the
>   check while a user is doing mmio access then we shouldn't attempt the
>   check anywhere.
>
> - i915_modparams.mmio_debug is not save/restored anymore around user
>   access. The value is now never touched by the kernel while debug is
>   disabled so no need for save/restore.
>
> v2: squash mmio_debug patches, restrict mmio_debug lock usage (Chris)
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
>  drivers/gpu/drm/i915/i915_drv.c     |  3 +-
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_uncore.c | 91 ++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_uncore.h | 18 +++---
>  5 files changed, 79 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3b15266c54fd..2310512111ab 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1129,7 +1129,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data)
>  	unsigned int tmp;
>  
>  	seq_printf(m, "user.bypass_count = %u\n",
> -		   uncore->user_forcewake.count);
> +		   uncore->user_forcewake_count);
>  
>  	for_each_fw_domain(fw_domain, uncore, tmp)
>  		seq_printf(m, "%s.wake_count = %u\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3480db36b63f..fbbff4a133ba 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -744,6 +744,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>  
>  	intel_device_info_subplatform_init(dev_priv);
>  
> +	intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug);
>  	intel_uncore_init_early(&dev_priv->uncore, dev_priv);
>  
>  	spin_lock_init(&dev_priv->irq_lock);
> @@ -2044,7 +2045,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  
>  out:
>  	enable_rpm_wakeref_asserts(rpm);
> -	if (!dev_priv->uncore.user_forcewake.count)
> +	if (!dev_priv->uncore.user_forcewake_count)
>  		intel_runtime_pm_driver_release(rpm);
>  
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c9476f24f5c1..13c27a75dca8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1346,6 +1346,7 @@ struct drm_i915_private {
>  	resource_size_t stolen_usable_size;	/* Total size minus reserved ranges */
>  
>  	struct intel_uncore uncore;
> +	struct intel_uncore_mmio_debug mmio_debug;
>  
>  	struct i915_virtual_gpu vgpu;
>  
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 39e8710afdd9..9e583f13a9e4 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -34,6 +34,32 @@
>  
>  #define __raw_posting_read(...) ((void)__raw_uncore_read32(__VA_ARGS__))
>  
> +void
> +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug)
> +{
> +	spin_lock_init(&mmio_debug->lock);
> +	mmio_debug->unclaimed_mmio_check = 1;
> +}
> +
> +static void mmio_debug_suspend(struct intel_uncore_mmio_debug *mmio_debug)
> +{
> +	lockdep_assert_held(&mmio_debug->lock);
> +
> +	/* Save and disable mmio debugging for the user bypass */
> +	if (!mmio_debug->suspend_count++) {
> +		mmio_debug->saved_mmio_check = mmio_debug->unclaimed_mmio_check;
> +		mmio_debug->unclaimed_mmio_check = 0;
> +	}
> +}
> +
> +static void mmio_debug_resume(struct intel_uncore_mmio_debug *mmio_debug)
> +{
> +	lockdep_assert_held(&mmio_debug->lock);
> +
> +	if (!--mmio_debug->suspend_count)
> +		mmio_debug->unclaimed_mmio_check = mmio_debug->saved_mmio_check;
> +}
> +
>  static const char * const forcewake_domain_names[] = {
>  	"render",
>  	"blitter",
> @@ -476,6 +502,11 @@ check_for_unclaimed_mmio(struct intel_uncore *uncore)
>  {
>  	bool ret = false;
>  
> +	lockdep_assert_held(&uncore->debug->lock);
> +
> +	if (uncore->debug->suspend_count)
> +		return false;
> +
>  	if (intel_uncore_has_fpga_dbg_unclaimed(uncore))
>  		ret |= fpga_check_for_unclaimed_mmio(uncore);
>  
> @@ -608,17 +639,11 @@ void intel_uncore_forcewake_get(struct intel_uncore *uncore,
>  void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
>  {
>  	spin_lock_irq(&uncore->lock);
> -	if (!uncore->user_forcewake.count++) {
> +	if (!uncore->user_forcewake_count++) {
>  		intel_uncore_forcewake_get__locked(uncore, FORCEWAKE_ALL);
> -
> -		/* Save and disable mmio debugging for the user bypass */
> -		uncore->user_forcewake.saved_mmio_check =
> -			uncore->unclaimed_mmio_check;
> -		uncore->user_forcewake.saved_mmio_debug =
> -			i915_modparams.mmio_debug;
> -
> -		uncore->unclaimed_mmio_check = 0;
> -		i915_modparams.mmio_debug = 0;
> +		spin_lock(&uncore->debug->lock);

For me it looks like you need spin_lock_irq here also
like with the parent lock above.
-Mika

> +		mmio_debug_suspend(uncore->debug);
> +		spin_unlock(&uncore->debug->lock);
>  	}
>  	spin_unlock_irq(&uncore->lock);
>  }
> @@ -633,15 +658,14 @@ void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
>  void intel_uncore_forcewake_user_put(struct intel_uncore *uncore)
>  {
>  	spin_lock_irq(&uncore->lock);
> -	if (!--uncore->user_forcewake.count) {
> -		if (intel_uncore_unclaimed_mmio(uncore))
> +	if (!--uncore->user_forcewake_count) {
> +		spin_lock(&uncore->debug->lock);
> +		mmio_debug_resume(uncore->debug);
> +
> +		if (check_for_unclaimed_mmio(uncore))
>  			dev_info(uncore->i915->drm.dev,
>  				 "Invalid mmio detected during user access\n");
> -
> -		uncore->unclaimed_mmio_check =
> -			uncore->user_forcewake.saved_mmio_check;
> -		i915_modparams.mmio_debug =
> -			uncore->user_forcewake.saved_mmio_debug;
> +		spin_unlock(&uncore->debug->lock);
>  
>  		intel_uncore_forcewake_put__locked(uncore, FORCEWAKE_ALL);
>  	}
> @@ -1088,7 +1112,16 @@ unclaimed_reg_debug(struct intel_uncore *uncore,
>  	if (likely(!i915_modparams.mmio_debug))
>  		return;
>  
> +	/* interrupts are disabled and re-enabled around uncore->lock usage */
> +	lockdep_assert_held(&uncore->lock);
> +
> +	if (before)
> +		spin_lock(&uncore->debug->lock);
> +
>  	__unclaimed_reg_debug(uncore, reg, read, before);
> +
> +	if (!before)
> +		spin_unlock(&uncore->debug->lock);
>  }
>  
>  #define GEN2_READ_HEADER(x) \
> @@ -1607,6 +1640,7 @@ void intel_uncore_init_early(struct intel_uncore *uncore,
>  	spin_lock_init(&uncore->lock);
>  	uncore->i915 = i915;
>  	uncore->rpm = &i915->runtime_pm;
> +	uncore->debug = &i915->mmio_debug;
>  }
>  
>  static void uncore_raw_init(struct intel_uncore *uncore)
> @@ -1632,7 +1666,6 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
>  	ret = intel_uncore_fw_domains_init(uncore);
>  	if (ret)
>  		return ret;
> -
>  	forcewake_early_sanitize(uncore, 0);
>  
>  	if (IS_GEN_RANGE(i915, 6, 7)) {
> @@ -1681,8 +1714,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>  	if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
>  		uncore->flags |= UNCORE_HAS_FORCEWAKE;
>  
> -	uncore->unclaimed_mmio_check = 1;
> -
>  	if (!intel_uncore_has_forcewake(uncore)) {
>  		uncore_raw_init(uncore);
>  	} else {
> @@ -1707,7 +1738,7 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>  		uncore->flags |= UNCORE_HAS_FIFO;
>  
>  	/* clear out unclaimed reg detection bit */
> -	if (check_for_unclaimed_mmio(uncore))
> +	if (intel_uncore_unclaimed_mmio(uncore))
>  		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
>  
>  	return 0;
> @@ -1952,7 +1983,13 @@ int __intel_wait_for_register(struct intel_uncore *uncore,
>  
>  bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore)
>  {
> -	return check_for_unclaimed_mmio(uncore);
> +	bool ret;
> +
> +	spin_lock_irq(&uncore->debug->lock);
> +	ret = check_for_unclaimed_mmio(uncore);
> +	spin_unlock_irq(&uncore->debug->lock);
> +
> +	return ret;
>  }
>  
>  bool
> @@ -1960,24 +1997,24 @@ intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore)
>  {
>  	bool ret = false;
>  
> -	spin_lock_irq(&uncore->lock);
> +	spin_lock_irq(&uncore->debug->lock);
>  
> -	if (unlikely(uncore->unclaimed_mmio_check <= 0))
> +	if (unlikely(uncore->debug->unclaimed_mmio_check <= 0))
>  		goto out;
>  
> -	if (unlikely(intel_uncore_unclaimed_mmio(uncore))) {
> +	if (unlikely(check_for_unclaimed_mmio(uncore))) {
>  		if (!i915_modparams.mmio_debug) {
>  			DRM_DEBUG("Unclaimed register detected, "
>  				  "enabling oneshot unclaimed register reporting. "
>  				  "Please use i915.mmio_debug=N for more information.\n");
>  			i915_modparams.mmio_debug++;
>  		}
> -		uncore->unclaimed_mmio_check--;
> +		uncore->debug->unclaimed_mmio_check--;
>  		ret = true;
>  	}
>  
>  out:
> -	spin_unlock_irq(&uncore->lock);
> +	spin_unlock_irq(&uncore->debug->lock);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index e603d210a34d..414fc2cb0459 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -36,6 +36,13 @@ struct drm_i915_private;
>  struct intel_runtime_pm;
>  struct intel_uncore;
>  
> +struct intel_uncore_mmio_debug {
> +	spinlock_t lock; /** lock is also taken in irq contexts. */
> +	int unclaimed_mmio_check;
> +	int saved_mmio_check;
> +	u32 suspend_count;
> +};
> +
>  enum forcewake_domain_id {
>  	FW_DOMAIN_ID_RENDER = 0,
>  	FW_DOMAIN_ID_BLITTER,
> @@ -137,14 +144,9 @@ struct intel_uncore {
>  		u32 __iomem *reg_ack;
>  	} *fw_domain[FW_DOMAIN_ID_COUNT];
>  
> -	struct {
> -		unsigned int count;
> -
> -		int saved_mmio_check;
> -		int saved_mmio_debug;
> -	} user_forcewake;
> +	unsigned int user_forcewake_count;
>  
> -	int unclaimed_mmio_check;
> +	struct intel_uncore_mmio_debug *debug;
>  };
>  
>  /* Iterate over initialised fw domains */
> @@ -179,6 +181,8 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
>  	return uncore->flags & UNCORE_HAS_FIFO;
>  }
>  
> +void
> +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug);
>  void intel_uncore_init_early(struct intel_uncore *uncore,
>  			     struct drm_i915_private *i915);
>  int intel_uncore_init_mmio(struct intel_uncore *uncore);
> -- 
> 2.22.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniele Ceraolo Spurio Aug. 8, 2019, 4:43 p.m. UTC | #2
On 8/8/19 6:08 AM, Mika Kuoppala wrote:
> Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> writes:
> 
>> Multiple uncore structures will share the debug infrastructure, so
>> move it to a common place and add extra locking around it.
>> Also, since we now have a separate object, it is cleaner to have
>> dedicated functions working on the object to stop and restart the
>> mmio debug. Apart from the cosmetic changes, this patch introduces
>> 2 functional updates:
>>
>> - All calls to check_for_unclaimed_mmio will now return false when
>>    the debug is suspended, not just the ones that are active only when
>>    i915_modparams.mmio_debug is set. If we don't trust the result of the
>>    check while a user is doing mmio access then we shouldn't attempt the
>>    check anywhere.
>>
>> - i915_modparams.mmio_debug is not save/restored anymore around user
>>    access. The value is now never touched by the kernel while debug is
>>    disabled so no need for save/restore.
>>
>> v2: squash mmio_debug patches, restrict mmio_debug lock usage (Chris)
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
>>   drivers/gpu/drm/i915/i915_drv.c     |  3 +-
>>   drivers/gpu/drm/i915/i915_drv.h     |  1 +
>>   drivers/gpu/drm/i915/intel_uncore.c | 91 ++++++++++++++++++++---------
>>   drivers/gpu/drm/i915/intel_uncore.h | 18 +++---
>>   5 files changed, 79 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 3b15266c54fd..2310512111ab 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1129,7 +1129,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data)
>>   	unsigned int tmp;
>>   
>>   	seq_printf(m, "user.bypass_count = %u\n",
>> -		   uncore->user_forcewake.count);
>> +		   uncore->user_forcewake_count);
>>   
>>   	for_each_fw_domain(fw_domain, uncore, tmp)
>>   		seq_printf(m, "%s.wake_count = %u\n",
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 3480db36b63f..fbbff4a133ba 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -744,6 +744,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>>   
>>   	intel_device_info_subplatform_init(dev_priv);
>>   
>> +	intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug);
>>   	intel_uncore_init_early(&dev_priv->uncore, dev_priv);
>>   
>>   	spin_lock_init(&dev_priv->irq_lock);
>> @@ -2044,7 +2045,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>>   
>>   out:
>>   	enable_rpm_wakeref_asserts(rpm);
>> -	if (!dev_priv->uncore.user_forcewake.count)
>> +	if (!dev_priv->uncore.user_forcewake_count)
>>   		intel_runtime_pm_driver_release(rpm);
>>   
>>   	return ret;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index c9476f24f5c1..13c27a75dca8 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1346,6 +1346,7 @@ struct drm_i915_private {
>>   	resource_size_t stolen_usable_size;	/* Total size minus reserved ranges */
>>   
>>   	struct intel_uncore uncore;
>> +	struct intel_uncore_mmio_debug mmio_debug;
>>   
>>   	struct i915_virtual_gpu vgpu;
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 39e8710afdd9..9e583f13a9e4 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -34,6 +34,32 @@
>>   
>>   #define __raw_posting_read(...) ((void)__raw_uncore_read32(__VA_ARGS__))
>>   
>> +void
>> +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug)
>> +{
>> +	spin_lock_init(&mmio_debug->lock);
>> +	mmio_debug->unclaimed_mmio_check = 1;
>> +}
>> +
>> +static void mmio_debug_suspend(struct intel_uncore_mmio_debug *mmio_debug)
>> +{
>> +	lockdep_assert_held(&mmio_debug->lock);
>> +
>> +	/* Save and disable mmio debugging for the user bypass */
>> +	if (!mmio_debug->suspend_count++) {
>> +		mmio_debug->saved_mmio_check = mmio_debug->unclaimed_mmio_check;
>> +		mmio_debug->unclaimed_mmio_check = 0;
>> +	}
>> +}
>> +
>> +static void mmio_debug_resume(struct intel_uncore_mmio_debug *mmio_debug)
>> +{
>> +	lockdep_assert_held(&mmio_debug->lock);
>> +
>> +	if (!--mmio_debug->suspend_count)
>> +		mmio_debug->unclaimed_mmio_check = mmio_debug->saved_mmio_check;
>> +}
>> +
>>   static const char * const forcewake_domain_names[] = {
>>   	"render",
>>   	"blitter",
>> @@ -476,6 +502,11 @@ check_for_unclaimed_mmio(struct intel_uncore *uncore)
>>   {
>>   	bool ret = false;
>>   
>> +	lockdep_assert_held(&uncore->debug->lock);
>> +
>> +	if (uncore->debug->suspend_count)
>> +		return false;
>> +
>>   	if (intel_uncore_has_fpga_dbg_unclaimed(uncore))
>>   		ret |= fpga_check_for_unclaimed_mmio(uncore);
>>   
>> @@ -608,17 +639,11 @@ void intel_uncore_forcewake_get(struct intel_uncore *uncore,
>>   void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
>>   {
>>   	spin_lock_irq(&uncore->lock);
>> -	if (!uncore->user_forcewake.count++) {
>> +	if (!uncore->user_forcewake_count++) {
>>   		intel_uncore_forcewake_get__locked(uncore, FORCEWAKE_ALL);
>> -
>> -		/* Save and disable mmio debugging for the user bypass */
>> -		uncore->user_forcewake.saved_mmio_check =
>> -			uncore->unclaimed_mmio_check;
>> -		uncore->user_forcewake.saved_mmio_debug =
>> -			i915_modparams.mmio_debug;
>> -
>> -		uncore->unclaimed_mmio_check = 0;
>> -		i915_modparams.mmio_debug = 0;
>> +		spin_lock(&uncore->debug->lock);
> 
> For me it looks like you need spin_lock_irq here also
> like with the parent lock above.

The parent lock should ensure that the irqs are already off at this 
point, so no need to disable them twice. Also, spin_unlock_irq can't be 
called twice because the first of the 2 calls would re-enable interrupts 
while the second lock is still taken. Irqflags would solve that, but 
still wouldn't give any benefit compared to a straight lock IMO.

Daniele

> -Mika
> 
>> +		mmio_debug_suspend(uncore->debug);
>> +		spin_unlock(&uncore->debug->lock);
>>   	}
>>   	spin_unlock_irq(&uncore->lock);
>>   }
>> @@ -633,15 +658,14 @@ void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
>>   void intel_uncore_forcewake_user_put(struct intel_uncore *uncore)
>>   {
>>   	spin_lock_irq(&uncore->lock);
>> -	if (!--uncore->user_forcewake.count) {
>> -		if (intel_uncore_unclaimed_mmio(uncore))
>> +	if (!--uncore->user_forcewake_count) {
>> +		spin_lock(&uncore->debug->lock);
>> +		mmio_debug_resume(uncore->debug);
>> +
>> +		if (check_for_unclaimed_mmio(uncore))
>>   			dev_info(uncore->i915->drm.dev,
>>   				 "Invalid mmio detected during user access\n");
>> -
>> -		uncore->unclaimed_mmio_check =
>> -			uncore->user_forcewake.saved_mmio_check;
>> -		i915_modparams.mmio_debug =
>> -			uncore->user_forcewake.saved_mmio_debug;
>> +		spin_unlock(&uncore->debug->lock);
>>   
>>   		intel_uncore_forcewake_put__locked(uncore, FORCEWAKE_ALL);
>>   	}
>> @@ -1088,7 +1112,16 @@ unclaimed_reg_debug(struct intel_uncore *uncore,
>>   	if (likely(!i915_modparams.mmio_debug))
>>   		return;
>>   
>> +	/* interrupts are disabled and re-enabled around uncore->lock usage */
>> +	lockdep_assert_held(&uncore->lock);
>> +
>> +	if (before)
>> +		spin_lock(&uncore->debug->lock);
>> +
>>   	__unclaimed_reg_debug(uncore, reg, read, before);
>> +
>> +	if (!before)
>> +		spin_unlock(&uncore->debug->lock);
>>   }
>>   
>>   #define GEN2_READ_HEADER(x) \
>> @@ -1607,6 +1640,7 @@ void intel_uncore_init_early(struct intel_uncore *uncore,
>>   	spin_lock_init(&uncore->lock);
>>   	uncore->i915 = i915;
>>   	uncore->rpm = &i915->runtime_pm;
>> +	uncore->debug = &i915->mmio_debug;
>>   }
>>   
>>   static void uncore_raw_init(struct intel_uncore *uncore)
>> @@ -1632,7 +1666,6 @@ static int uncore_forcewake_init(struct intel_uncore *uncore)
>>   	ret = intel_uncore_fw_domains_init(uncore);
>>   	if (ret)
>>   		return ret;
>> -
>>   	forcewake_early_sanitize(uncore, 0);
>>   
>>   	if (IS_GEN_RANGE(i915, 6, 7)) {
>> @@ -1681,8 +1714,6 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>>   	if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
>>   		uncore->flags |= UNCORE_HAS_FORCEWAKE;
>>   
>> -	uncore->unclaimed_mmio_check = 1;
>> -
>>   	if (!intel_uncore_has_forcewake(uncore)) {
>>   		uncore_raw_init(uncore);
>>   	} else {
>> @@ -1707,7 +1738,7 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
>>   		uncore->flags |= UNCORE_HAS_FIFO;
>>   
>>   	/* clear out unclaimed reg detection bit */
>> -	if (check_for_unclaimed_mmio(uncore))
>> +	if (intel_uncore_unclaimed_mmio(uncore))
>>   		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
>>   
>>   	return 0;
>> @@ -1952,7 +1983,13 @@ int __intel_wait_for_register(struct intel_uncore *uncore,
>>   
>>   bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore)
>>   {
>> -	return check_for_unclaimed_mmio(uncore);
>> +	bool ret;
>> +
>> +	spin_lock_irq(&uncore->debug->lock);
>> +	ret = check_for_unclaimed_mmio(uncore);
>> +	spin_unlock_irq(&uncore->debug->lock);
>> +
>> +	return ret;
>>   }
>>   
>>   bool
>> @@ -1960,24 +1997,24 @@ intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore)
>>   {
>>   	bool ret = false;
>>   
>> -	spin_lock_irq(&uncore->lock);
>> +	spin_lock_irq(&uncore->debug->lock);
>>   
>> -	if (unlikely(uncore->unclaimed_mmio_check <= 0))
>> +	if (unlikely(uncore->debug->unclaimed_mmio_check <= 0))
>>   		goto out;
>>   
>> -	if (unlikely(intel_uncore_unclaimed_mmio(uncore))) {
>> +	if (unlikely(check_for_unclaimed_mmio(uncore))) {
>>   		if (!i915_modparams.mmio_debug) {
>>   			DRM_DEBUG("Unclaimed register detected, "
>>   				  "enabling oneshot unclaimed register reporting. "
>>   				  "Please use i915.mmio_debug=N for more information.\n");
>>   			i915_modparams.mmio_debug++;
>>   		}
>> -		uncore->unclaimed_mmio_check--;
>> +		uncore->debug->unclaimed_mmio_check--;
>>   		ret = true;
>>   	}
>>   
>>   out:
>> -	spin_unlock_irq(&uncore->lock);
>> +	spin_unlock_irq(&uncore->debug->lock);
>>   
>>   	return ret;
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
>> index e603d210a34d..414fc2cb0459 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.h
>> +++ b/drivers/gpu/drm/i915/intel_uncore.h
>> @@ -36,6 +36,13 @@ struct drm_i915_private;
>>   struct intel_runtime_pm;
>>   struct intel_uncore;
>>   
>> +struct intel_uncore_mmio_debug {
>> +	spinlock_t lock; /** lock is also taken in irq contexts. */
>> +	int unclaimed_mmio_check;
>> +	int saved_mmio_check;
>> +	u32 suspend_count;
>> +};
>> +
>>   enum forcewake_domain_id {
>>   	FW_DOMAIN_ID_RENDER = 0,
>>   	FW_DOMAIN_ID_BLITTER,
>> @@ -137,14 +144,9 @@ struct intel_uncore {
>>   		u32 __iomem *reg_ack;
>>   	} *fw_domain[FW_DOMAIN_ID_COUNT];
>>   
>> -	struct {
>> -		unsigned int count;
>> -
>> -		int saved_mmio_check;
>> -		int saved_mmio_debug;
>> -	} user_forcewake;
>> +	unsigned int user_forcewake_count;
>>   
>> -	int unclaimed_mmio_check;
>> +	struct intel_uncore_mmio_debug *debug;
>>   };
>>   
>>   /* Iterate over initialised fw domains */
>> @@ -179,6 +181,8 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
>>   	return uncore->flags & UNCORE_HAS_FIFO;
>>   }
>>   
>> +void
>> +intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug);
>>   void intel_uncore_init_early(struct intel_uncore *uncore,
>>   			     struct drm_i915_private *i915);
>>   int intel_uncore_init_mmio(struct intel_uncore *uncore);
>> -- 
>> 2.22.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Aug. 8, 2019, 4:59 p.m. UTC | #3
Quoting Daniele Ceraolo Spurio (2019-08-08 02:44:21)
> Multiple uncore structures will share the debug infrastructure, so
> move it to a common place and add extra locking around it.
> Also, since we now have a separate object, it is cleaner to have
> dedicated functions working on the object to stop and restart the
> mmio debug. Apart from the cosmetic changes, this patch introduces
> 2 functional updates:
> 
> - All calls to check_for_unclaimed_mmio will now return false when
>   the debug is suspended, not just the ones that are active only when
>   i915_modparams.mmio_debug is set. If we don't trust the result of the
>   check while a user is doing mmio access then we shouldn't attempt the
>   check anywhere.
> 
> - i915_modparams.mmio_debug is not save/restored anymore around user
>   access. The value is now never touched by the kernel while debug is
>   disabled so no need for save/restore.

Yup. Early return from check_for_unclaimed_mmio() if suspended, with
user_forcewake calling mmio_suspend.

> v2: squash mmio_debug patches, restrict mmio_debug lock usage (Chris)
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3b15266c54fd..2310512111ab 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1129,7 +1129,7 @@  static int i915_forcewake_domains(struct seq_file *m, void *data)
 	unsigned int tmp;
 
 	seq_printf(m, "user.bypass_count = %u\n",
-		   uncore->user_forcewake.count);
+		   uncore->user_forcewake_count);
 
 	for_each_fw_domain(fw_domain, uncore, tmp)
 		seq_printf(m, "%s.wake_count = %u\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3480db36b63f..fbbff4a133ba 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -744,6 +744,7 @@  static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 
 	intel_device_info_subplatform_init(dev_priv);
 
+	intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug);
 	intel_uncore_init_early(&dev_priv->uncore, dev_priv);
 
 	spin_lock_init(&dev_priv->irq_lock);
@@ -2044,7 +2045,7 @@  static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 
 out:
 	enable_rpm_wakeref_asserts(rpm);
-	if (!dev_priv->uncore.user_forcewake.count)
+	if (!dev_priv->uncore.user_forcewake_count)
 		intel_runtime_pm_driver_release(rpm);
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c9476f24f5c1..13c27a75dca8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1346,6 +1346,7 @@  struct drm_i915_private {
 	resource_size_t stolen_usable_size;	/* Total size minus reserved ranges */
 
 	struct intel_uncore uncore;
+	struct intel_uncore_mmio_debug mmio_debug;
 
 	struct i915_virtual_gpu vgpu;
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 39e8710afdd9..9e583f13a9e4 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -34,6 +34,32 @@ 
 
 #define __raw_posting_read(...) ((void)__raw_uncore_read32(__VA_ARGS__))
 
+void
+intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug)
+{
+	spin_lock_init(&mmio_debug->lock);
+	mmio_debug->unclaimed_mmio_check = 1;
+}
+
+static void mmio_debug_suspend(struct intel_uncore_mmio_debug *mmio_debug)
+{
+	lockdep_assert_held(&mmio_debug->lock);
+
+	/* Save and disable mmio debugging for the user bypass */
+	if (!mmio_debug->suspend_count++) {
+		mmio_debug->saved_mmio_check = mmio_debug->unclaimed_mmio_check;
+		mmio_debug->unclaimed_mmio_check = 0;
+	}
+}
+
+static void mmio_debug_resume(struct intel_uncore_mmio_debug *mmio_debug)
+{
+	lockdep_assert_held(&mmio_debug->lock);
+
+	if (!--mmio_debug->suspend_count)
+		mmio_debug->unclaimed_mmio_check = mmio_debug->saved_mmio_check;
+}
+
 static const char * const forcewake_domain_names[] = {
 	"render",
 	"blitter",
@@ -476,6 +502,11 @@  check_for_unclaimed_mmio(struct intel_uncore *uncore)
 {
 	bool ret = false;
 
+	lockdep_assert_held(&uncore->debug->lock);
+
+	if (uncore->debug->suspend_count)
+		return false;
+
 	if (intel_uncore_has_fpga_dbg_unclaimed(uncore))
 		ret |= fpga_check_for_unclaimed_mmio(uncore);
 
@@ -608,17 +639,11 @@  void intel_uncore_forcewake_get(struct intel_uncore *uncore,
 void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
 {
 	spin_lock_irq(&uncore->lock);
-	if (!uncore->user_forcewake.count++) {
+	if (!uncore->user_forcewake_count++) {
 		intel_uncore_forcewake_get__locked(uncore, FORCEWAKE_ALL);
-
-		/* Save and disable mmio debugging for the user bypass */
-		uncore->user_forcewake.saved_mmio_check =
-			uncore->unclaimed_mmio_check;
-		uncore->user_forcewake.saved_mmio_debug =
-			i915_modparams.mmio_debug;
-
-		uncore->unclaimed_mmio_check = 0;
-		i915_modparams.mmio_debug = 0;
+		spin_lock(&uncore->debug->lock);
+		mmio_debug_suspend(uncore->debug);
+		spin_unlock(&uncore->debug->lock);
 	}
 	spin_unlock_irq(&uncore->lock);
 }
@@ -633,15 +658,14 @@  void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
 void intel_uncore_forcewake_user_put(struct intel_uncore *uncore)
 {
 	spin_lock_irq(&uncore->lock);
-	if (!--uncore->user_forcewake.count) {
-		if (intel_uncore_unclaimed_mmio(uncore))
+	if (!--uncore->user_forcewake_count) {
+		spin_lock(&uncore->debug->lock);
+		mmio_debug_resume(uncore->debug);
+
+		if (check_for_unclaimed_mmio(uncore))
 			dev_info(uncore->i915->drm.dev,
 				 "Invalid mmio detected during user access\n");
-
-		uncore->unclaimed_mmio_check =
-			uncore->user_forcewake.saved_mmio_check;
-		i915_modparams.mmio_debug =
-			uncore->user_forcewake.saved_mmio_debug;
+		spin_unlock(&uncore->debug->lock);
 
 		intel_uncore_forcewake_put__locked(uncore, FORCEWAKE_ALL);
 	}
@@ -1088,7 +1112,16 @@  unclaimed_reg_debug(struct intel_uncore *uncore,
 	if (likely(!i915_modparams.mmio_debug))
 		return;
 
+	/* interrupts are disabled and re-enabled around uncore->lock usage */
+	lockdep_assert_held(&uncore->lock);
+
+	if (before)
+		spin_lock(&uncore->debug->lock);
+
 	__unclaimed_reg_debug(uncore, reg, read, before);
+
+	if (!before)
+		spin_unlock(&uncore->debug->lock);
 }
 
 #define GEN2_READ_HEADER(x) \
@@ -1607,6 +1640,7 @@  void intel_uncore_init_early(struct intel_uncore *uncore,
 	spin_lock_init(&uncore->lock);
 	uncore->i915 = i915;
 	uncore->rpm = &i915->runtime_pm;
+	uncore->debug = &i915->mmio_debug;
 }
 
 static void uncore_raw_init(struct intel_uncore *uncore)
@@ -1632,7 +1666,6 @@  static int uncore_forcewake_init(struct intel_uncore *uncore)
 	ret = intel_uncore_fw_domains_init(uncore);
 	if (ret)
 		return ret;
-
 	forcewake_early_sanitize(uncore, 0);
 
 	if (IS_GEN_RANGE(i915, 6, 7)) {
@@ -1681,8 +1714,6 @@  int intel_uncore_init_mmio(struct intel_uncore *uncore)
 	if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
 		uncore->flags |= UNCORE_HAS_FORCEWAKE;
 
-	uncore->unclaimed_mmio_check = 1;
-
 	if (!intel_uncore_has_forcewake(uncore)) {
 		uncore_raw_init(uncore);
 	} else {
@@ -1707,7 +1738,7 @@  int intel_uncore_init_mmio(struct intel_uncore *uncore)
 		uncore->flags |= UNCORE_HAS_FIFO;
 
 	/* clear out unclaimed reg detection bit */
-	if (check_for_unclaimed_mmio(uncore))
+	if (intel_uncore_unclaimed_mmio(uncore))
 		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
 
 	return 0;
@@ -1952,7 +1983,13 @@  int __intel_wait_for_register(struct intel_uncore *uncore,
 
 bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore)
 {
-	return check_for_unclaimed_mmio(uncore);
+	bool ret;
+
+	spin_lock_irq(&uncore->debug->lock);
+	ret = check_for_unclaimed_mmio(uncore);
+	spin_unlock_irq(&uncore->debug->lock);
+
+	return ret;
 }
 
 bool
@@ -1960,24 +1997,24 @@  intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore)
 {
 	bool ret = false;
 
-	spin_lock_irq(&uncore->lock);
+	spin_lock_irq(&uncore->debug->lock);
 
-	if (unlikely(uncore->unclaimed_mmio_check <= 0))
+	if (unlikely(uncore->debug->unclaimed_mmio_check <= 0))
 		goto out;
 
-	if (unlikely(intel_uncore_unclaimed_mmio(uncore))) {
+	if (unlikely(check_for_unclaimed_mmio(uncore))) {
 		if (!i915_modparams.mmio_debug) {
 			DRM_DEBUG("Unclaimed register detected, "
 				  "enabling oneshot unclaimed register reporting. "
 				  "Please use i915.mmio_debug=N for more information.\n");
 			i915_modparams.mmio_debug++;
 		}
-		uncore->unclaimed_mmio_check--;
+		uncore->debug->unclaimed_mmio_check--;
 		ret = true;
 	}
 
 out:
-	spin_unlock_irq(&uncore->lock);
+	spin_unlock_irq(&uncore->debug->lock);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index e603d210a34d..414fc2cb0459 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -36,6 +36,13 @@  struct drm_i915_private;
 struct intel_runtime_pm;
 struct intel_uncore;
 
+struct intel_uncore_mmio_debug {
+	spinlock_t lock; /** lock is also taken in irq contexts. */
+	int unclaimed_mmio_check;
+	int saved_mmio_check;
+	u32 suspend_count;
+};
+
 enum forcewake_domain_id {
 	FW_DOMAIN_ID_RENDER = 0,
 	FW_DOMAIN_ID_BLITTER,
@@ -137,14 +144,9 @@  struct intel_uncore {
 		u32 __iomem *reg_ack;
 	} *fw_domain[FW_DOMAIN_ID_COUNT];
 
-	struct {
-		unsigned int count;
-
-		int saved_mmio_check;
-		int saved_mmio_debug;
-	} user_forcewake;
+	unsigned int user_forcewake_count;
 
-	int unclaimed_mmio_check;
+	struct intel_uncore_mmio_debug *debug;
 };
 
 /* Iterate over initialised fw domains */
@@ -179,6 +181,8 @@  intel_uncore_has_fifo(const struct intel_uncore *uncore)
 	return uncore->flags & UNCORE_HAS_FIFO;
 }
 
+void
+intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug);
 void intel_uncore_init_early(struct intel_uncore *uncore,
 			     struct drm_i915_private *i915);
 int intel_uncore_init_mmio(struct intel_uncore *uncore);