Re: [PATCH 15/36] drm/i915: Mark up Ironlake ips with rpm wakerefs

[Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>]

On 3/14/2018 3:07 PM, Chris Wilson wrote:
> Currently Ironlake operates under the assumption that rpm awake (and its
> error checking is disabled). As such, we have missed a few places where we
> access registers without taking the rpm wakeref and thus trigger
> warnings. intel_ips being one culprit.
>
> As this involved adding a potentially sleeping rpm_get, we have to
> rearrange the spinlocks slightly and so switch to acquiring a device-ref
> under the spinlock rather than hold the spinlock for the whole
> operation. To be consistent, we make the change in pattern common to the
> intel_ips interface even though this adds a few more atomic operations
> than necessary in a few cases.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_drv.c |   3 +
>   drivers/gpu/drm/i915/intel_pm.c | 138 ++++++++++++++++++++--------------------
>   2 files changed, 73 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3d0b7353fb09..5c28990aab7f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1440,6 +1440,9 @@ void i915_driver_unload(struct drm_device *dev)
>   
>   	i915_driver_unregister(dev_priv);
>   
> +	/* Flush any external code that still may be under the RCU lock */
> +	synchronize_rcu();
> +
Hi Chris,

Will this rcu change be equivalent to

rcu_assign_pointer(i915_mch_dev, dev_priv) in gpu_ips_init
rcu_assign_pointer(i915_mch_dev, NULL) in gpu_ips_teardown

eliminating smp_store_mb from init/teardown and synchronize_rcu here.

Thanks,
Sagar
>   	if (i915_gem_suspend(dev_priv))
>   		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>   
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 447811c5be35..a2ebf66ff9ed 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5930,10 +5930,6 @@ void intel_init_ipc(struct drm_i915_private *dev_priv)
>    */
>   DEFINE_SPINLOCK(mchdev_lock);
>   
> -/* Global for IPS driver to get at the current i915 device. Protected by
> - * mchdev_lock. */
> -static struct drm_i915_private *i915_mch_dev;
> -
>   bool ironlake_set_drps(struct drm_i915_private *dev_priv, u8 val)
>   {
>   	u16 rgvswctl;
> @@ -7577,11 +7573,13 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
>   	if (!IS_GEN5(dev_priv))
>   		return 0;
>   
> +	intel_runtime_pm_get(dev_priv);
>   	spin_lock_irq(&mchdev_lock);
>   
>   	val = __i915_chipset_val(dev_priv);
>   
>   	spin_unlock_irq(&mchdev_lock);
> +	intel_runtime_pm_put(dev_priv);
>   
>   	return val;
>   }
> @@ -7661,11 +7659,13 @@ void i915_update_gfx_val(struct drm_i915_private *dev_priv)
>   	if (!IS_GEN5(dev_priv))
>   		return;
>   
> +	intel_runtime_pm_get(dev_priv);
>   	spin_lock_irq(&mchdev_lock);
>   
>   	__i915_update_gfx_val(dev_priv);
>   
>   	spin_unlock_irq(&mchdev_lock);
> +	intel_runtime_pm_put(dev_priv);
>   }
>   
>   static unsigned long __i915_gfx_val(struct drm_i915_private *dev_priv)
> @@ -7712,15 +7712,32 @@ unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
>   	if (!IS_GEN5(dev_priv))
>   		return 0;
>   
> +	intel_runtime_pm_get(dev_priv);
>   	spin_lock_irq(&mchdev_lock);
>   
>   	val = __i915_gfx_val(dev_priv);
>   
>   	spin_unlock_irq(&mchdev_lock);
> +	intel_runtime_pm_put(dev_priv);
>   
>   	return val;
>   }
>   
> +static struct drm_i915_private *i915_mch_dev;
> +
> +static struct drm_i915_private *mchdev_get(void)
> +{
> +	struct drm_i915_private *i915;
> +
> +	rcu_read_lock();
> +	i915 = i915_mch_dev;
> +	if (!kref_get_unless_zero(&i915->drm.ref))
> +		i915 = NULL;
> +	rcu_read_unlock();
> +
> +	return i915;
> +}
> +
>   /**
>    * i915_read_mch_val - return value for IPS use
>    *
> @@ -7729,23 +7746,22 @@ unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
>    */
>   unsigned long i915_read_mch_val(void)
>   {
> -	struct drm_i915_private *dev_priv;
> -	unsigned long chipset_val, graphics_val, ret = 0;
> -
> -	spin_lock_irq(&mchdev_lock);
> -	if (!i915_mch_dev)
> -		goto out_unlock;
> -	dev_priv = i915_mch_dev;
> -
> -	chipset_val = __i915_chipset_val(dev_priv);
> -	graphics_val = __i915_gfx_val(dev_priv);
> +	struct drm_i915_private *i915;
> +	unsigned long chipset_val, graphics_val;
>   
> -	ret = chipset_val + graphics_val;
> +	i915 = mchdev_get();
> +	if (!i915)
> +		return 0;
>   
> -out_unlock:
> +	intel_runtime_pm_get(i915);
> +	spin_lock_irq(&mchdev_lock);
> +	chipset_val = __i915_chipset_val(i915);
> +	graphics_val = __i915_gfx_val(i915);
>   	spin_unlock_irq(&mchdev_lock);
> +	intel_runtime_pm_put(i915);
>   
> -	return ret;
> +	drm_dev_put(&i915->drm);
> +	return chipset_val + graphics_val;
>   }
>   EXPORT_SYMBOL_GPL(i915_read_mch_val);
>   
> @@ -7756,23 +7772,19 @@ EXPORT_SYMBOL_GPL(i915_read_mch_val);
>    */
>   bool i915_gpu_raise(void)
>   {
> -	struct drm_i915_private *dev_priv;
> -	bool ret = true;
> -
> -	spin_lock_irq(&mchdev_lock);
> -	if (!i915_mch_dev) {
> -		ret = false;
> -		goto out_unlock;
> -	}
> -	dev_priv = i915_mch_dev;
> +	struct drm_i915_private *i915;
>   
> -	if (dev_priv->ips.max_delay > dev_priv->ips.fmax)
> -		dev_priv->ips.max_delay--;
> +	i915 = mchdev_get();
> +	if (!i915)
> +		return false;
>   
> -out_unlock:
> +	spin_lock_irq(&mchdev_lock);
> +	if (i915->ips.max_delay > i915->ips.fmax)
> +		i915->ips.max_delay--;
>   	spin_unlock_irq(&mchdev_lock);
>   
> -	return ret;
> +	drm_dev_put(&i915->drm);
> +	return true;
>   }
>   EXPORT_SYMBOL_GPL(i915_gpu_raise);
>   
> @@ -7784,23 +7796,19 @@ EXPORT_SYMBOL_GPL(i915_gpu_raise);
>    */
>   bool i915_gpu_lower(void)
>   {
> -	struct drm_i915_private *dev_priv;
> -	bool ret = true;
> +	struct drm_i915_private *i915;
>   
> -	spin_lock_irq(&mchdev_lock);
> -	if (!i915_mch_dev) {
> -		ret = false;
> -		goto out_unlock;
> -	}
> -	dev_priv = i915_mch_dev;
> -
> -	if (dev_priv->ips.max_delay < dev_priv->ips.min_delay)
> -		dev_priv->ips.max_delay++;
> +	i915 = mchdev_get();
> +	if (!i915)
> +		return false;
>   
> -out_unlock:
> +	spin_lock_irq(&mchdev_lock);
> +	if (i915->ips.max_delay < i915->ips.min_delay)
> +		i915->ips.max_delay++;
>   	spin_unlock_irq(&mchdev_lock);
>   
> -	return ret;
> +	drm_dev_put(&i915->drm);
> +	return true;
>   }
>   EXPORT_SYMBOL_GPL(i915_gpu_lower);
>   
> @@ -7811,13 +7819,16 @@ EXPORT_SYMBOL_GPL(i915_gpu_lower);
>    */
>   bool i915_gpu_busy(void)
>   {
> -	bool ret = false;
> +	struct drm_i915_private *i915;
> +	bool ret;
>   
> -	spin_lock_irq(&mchdev_lock);
> -	if (i915_mch_dev)
> -		ret = i915_mch_dev->gt.awake;
> -	spin_unlock_irq(&mchdev_lock);
> +	i915 = mchdev_get();
> +	if (!i915)
> +		return false;
> +
> +	ret = i915->gt.awake;
>   
> +	drm_dev_put(&i915->drm);
>   	return ret;
>   }
>   EXPORT_SYMBOL_GPL(i915_gpu_busy);
> @@ -7830,24 +7841,19 @@ EXPORT_SYMBOL_GPL(i915_gpu_busy);
>    */
>   bool i915_gpu_turbo_disable(void)
>   {
> -	struct drm_i915_private *dev_priv;
> -	bool ret = true;
> -
> -	spin_lock_irq(&mchdev_lock);
> -	if (!i915_mch_dev) {
> -		ret = false;
> -		goto out_unlock;
> -	}
> -	dev_priv = i915_mch_dev;
> -
> -	dev_priv->ips.max_delay = dev_priv->ips.fstart;
> +	struct drm_i915_private *i915;
> +	bool ret;
>   
> -	if (!ironlake_set_drps(dev_priv, dev_priv->ips.fstart))
> -		ret = false;
> +	i915 = mchdev_get();
> +	if (!i915)
> +		return false;
>   
> -out_unlock:
> +	spin_lock_irq(&mchdev_lock);
> +	i915->ips.max_delay = i915->ips.fstart;
> +	ret = ironlake_set_drps(i915, i915->ips.fstart);
>   	spin_unlock_irq(&mchdev_lock);
>   
> +	drm_dev_put(&i915->drm);
>   	return ret;
>   }
>   EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
> @@ -7876,18 +7882,14 @@ void intel_gpu_ips_init(struct drm_i915_private *dev_priv)
>   {
>   	/* We only register the i915 ips part with intel-ips once everything is
>   	 * set up, to avoid intel-ips sneaking in and reading bogus values. */
> -	spin_lock_irq(&mchdev_lock);
> -	i915_mch_dev = dev_priv;
> -	spin_unlock_irq(&mchdev_lock);
> +	smp_store_mb(i915_mch_dev, dev_priv);
>   
>   	ips_ping_for_i915_load();
>   }
>   
>   void intel_gpu_ips_teardown(void)
>   {
> -	spin_lock_irq(&mchdev_lock);
> -	i915_mch_dev = NULL;
> -	spin_unlock_irq(&mchdev_lock);
> +	smp_store_mb(i915_mch_dev, NULL);
>   }
>   
>   static void intel_init_emon(struct drm_i915_private *dev_priv)

--
Thanks,
Sagar

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