Re: [PATCH 08/10] drm/i915: Defer enabling rc6 til after we submit the first batch/context

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Some hardware requires a valid render context before it can initiate
> rc6 power gating of the GPU; the default state of the GPU is not
> sufficient and may lead to undefined behaviour. The first execution of
> any batch will load the "golden render state", at which point it is safe
> to enable rc6. As we do not forcibly load the kernel context at resume,
> we have to hook into the batch submission to be sure that the render
> state is setup before enabling rc6.
>
> However, since we don't enable powersaving until that first batch, we
> queued a delayed task in order to guarantee that the batch is indeed
> submitted.
>
> v2: Rearrange intel_disable_gt_powersave() to match.
> v3: Apply user specified cur_freq (or idle_freq if not set).
> v4: Give in, and supply a delayed work to autoenable rc6
> v5: Mika suggested a couple of better names for delayed_resume_work
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |   7 +-
>  drivers/gpu/drm/i915/i915_drv.h      |   2 +-
>  drivers/gpu/drm/i915/i915_gem.c      |   1 +
>  drivers/gpu/drm/i915/intel_display.c |   2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 122 +++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_uncore.c  |   2 +-
>  7 files changed, 89 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b9a811750ca8..ff3342b78a74 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1343,7 +1343,7 @@ void i915_driver_unload(struct drm_device *dev)
>  	i915_destroy_error_state(dev);
>  
>  	/* Flush any outstanding unpin_work. */
> -	flush_workqueue(dev_priv->wq);
> +	drain_workqueue(dev_priv->wq);
>

For this to make sense, should we also use the dev priv workqueue
for the gt autoenabling?

-Mika


>  	intel_guc_fini(dev);
>  	i915_gem_fini(dev);
> @@ -1652,6 +1652,7 @@ static int i915_drm_resume(struct drm_device *dev)
>  
>  	intel_opregion_notify_adapter(dev_priv, PCI_D0);
>  
> +	intel_autoenable_gt_powersave(dev_priv);
>  	drm_kms_helper_poll_enable(dev);
>  
>  	enable_rpm_wakeref_asserts(dev_priv);
> @@ -1835,8 +1836,7 @@ int i915_reset(struct drm_i915_private *dev_priv)
>  	 * previous concerns that it doesn't respond well to some forms
>  	 * of re-init after reset.
>  	 */
> -	if (INTEL_INFO(dev)->gen > 5)
> -		intel_enable_gt_powersave(dev_priv);
> +	intel_autoenable_gt_powersave(dev_priv);
>  
>  	return 0;
>  
> @@ -2459,7 +2459,6 @@ static int intel_runtime_resume(struct device *device)
>  	 * we can do is to hope that things will still work (and disable RPM).
>  	 */
>  	i915_gem_init_swizzling(dev);
> -	gen6_update_ring_freq(dev_priv);
>  
>  	intel_runtime_pm_enable_interrupts(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bb59bc637f7b..8d74db096189 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1188,7 +1188,7 @@ struct intel_gen6_power_mgmt {
>  	bool client_boost;
>  
>  	bool enabled;
> -	struct delayed_work delayed_resume_work;
> +	struct delayed_work autoenable_work;
>  	unsigned boosts;
>  
>  	struct intel_rps_client semaphores, mmioflips;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index adeca0ec4cfb..6f2227b24711 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2842,6 +2842,7 @@ static void i915_gem_mark_busy(const struct intel_engine_cs *engine)
>  	intel_runtime_pm_get_noresume(dev_priv);
>  	dev_priv->gt.awake = true;
>  
> +	intel_enable_gt_powersave(dev_priv);
>  	i915_update_gfx_val(dev_priv);
>  	if (INTEL_GEN(dev_priv) >= 6)
>  		gen6_rps_busy(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index be3b2cab2640..92cd0a70f8c8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15456,7 +15456,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
>  	dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
>  
>  	intel_init_clock_gating(dev);
> -	intel_enable_gt_powersave(dev_priv);
>  }
>  
>  /*
> @@ -16252,6 +16251,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> +	intel_suspend_gt_powersave(dev_priv);
>  	intel_disable_gt_powersave(dev_priv);
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 55aeaf041749..c68dd6166c52 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1697,7 +1697,9 @@ void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  void intel_gpu_ips_teardown(void);
>  void intel_init_gt_powersave(struct drm_i915_private *dev_priv);
>  void intel_cleanup_gt_powersave(struct drm_i915_private *dev_priv);
> +void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv);
>  void intel_enable_gt_powersave(struct drm_i915_private *dev_priv);
> +void intel_autoenable_gt_powersave(struct drm_i915_private *dev_priv);
>  void intel_disable_gt_powersave(struct drm_i915_private *dev_priv);
>  void intel_suspend_gt_powersave(struct drm_i915_private *dev_priv);
>  void intel_reset_gt_powersave(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index aab1e0b5d2eb..c6ba923425ce 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6549,13 +6549,6 @@ void intel_cleanup_gt_powersave(struct drm_i915_private *dev_priv)
>  		intel_runtime_pm_put(dev_priv);
>  }
>  
> -static void gen6_suspend_rps(struct drm_i915_private *dev_priv)
> -{
> -	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> -	gen6_disable_rps_interrupts(dev_priv);
> -}
> -
>  /**
>   * intel_suspend_gt_powersave - suspend PM work and helper threads
>   * @dev_priv: i915 device
> @@ -6569,50 +6562,65 @@ void intel_suspend_gt_powersave(struct drm_i915_private *dev_priv)
>  	if (INTEL_GEN(dev_priv) < 6)
>  		return;
>  
> -	gen6_suspend_rps(dev_priv);
> +	cancel_delayed_work_sync(&dev_priv->rps.autoenable_work);
> +
> +	gen6_disable_rps_interrupts(dev_priv);
>  
>  	/* Force GPU to min freq during suspend */
>  	gen6_rps_idle(dev_priv);
>  }
>  
> +void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv)
> +{
> +	dev_priv->rps.enabled = true; /* force disabling */
> +	intel_disable_gt_powersave(dev_priv);
> +
> +	gen6_reset_rps_interrupts(dev_priv);
> +}
> +
>  void intel_disable_gt_powersave(struct drm_i915_private *dev_priv)
>  {
> -	if (IS_IRONLAKE_M(dev_priv)) {
> -		ironlake_disable_drps(dev_priv);
> -	} else if (INTEL_INFO(dev_priv)->gen >= 6) {
> -		intel_suspend_gt_powersave(dev_priv);
> +	if (!READ_ONCE(dev_priv->rps.enabled))
> +		return;
>  
> -		mutex_lock(&dev_priv->rps.hw_lock);
> -		if (INTEL_INFO(dev_priv)->gen >= 9) {
> -			gen9_disable_rc6(dev_priv);
> -			gen9_disable_rps(dev_priv);
> -		} else if (IS_CHERRYVIEW(dev_priv))
> -			cherryview_disable_rps(dev_priv);
> -		else if (IS_VALLEYVIEW(dev_priv))
> -			valleyview_disable_rps(dev_priv);
> -		else
> -			gen6_disable_rps(dev_priv);
> +	mutex_lock(&dev_priv->rps.hw_lock);
>  
> -		dev_priv->rps.enabled = false;
> -		mutex_unlock(&dev_priv->rps.hw_lock);
> +	if (INTEL_GEN(dev_priv) >= 9) {
> +		gen9_disable_rc6(dev_priv);
> +		gen9_disable_rps(dev_priv);
> +	} else if (IS_CHERRYVIEW(dev_priv)) {
> +		cherryview_disable_rps(dev_priv);
> +	} else if (IS_VALLEYVIEW(dev_priv)) {
> +		valleyview_disable_rps(dev_priv);
> +	} else if (INTEL_GEN(dev_priv) >= 6) {
> +		gen6_disable_rps(dev_priv);
> +	}  else if (IS_IRONLAKE_M(dev_priv)) {
> +		ironlake_disable_drps(dev_priv);
>  	}
> +
> +	dev_priv->rps.enabled = false;
> +	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> -static void intel_gen6_powersave_work(struct work_struct *work)
> +void intel_enable_gt_powersave(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_i915_private *dev_priv =
> -		container_of(work, struct drm_i915_private,
> -			     rps.delayed_resume_work.work);
> +	/* We shouldn't be disabling as we submit, so this should be less
> +	 * racy than it appears!
> +	 */
> +	if (READ_ONCE(dev_priv->rps.enabled))
> +		return;
>  
> -	mutex_lock(&dev_priv->rps.hw_lock);
> +	/* Powersaving is controlled by the host when inside a VM */
> +	if (intel_vgpu_active(dev_priv))
> +		return;
>  
> -	gen6_reset_rps_interrupts(dev_priv);
> +	mutex_lock(&dev_priv->rps.hw_lock);
>  
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		cherryview_enable_rps(dev_priv);
>  	} else if (IS_VALLEYVIEW(dev_priv)) {
>  		valleyview_enable_rps(dev_priv);
> -	} else if (INTEL_INFO(dev_priv)->gen >= 9) {
> +	} else if (INTEL_GEN(dev_priv) >= 9) {
>  		gen9_enable_rc6(dev_priv);
>  		gen9_enable_rps(dev_priv);
>  		if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
> @@ -6620,9 +6628,12 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>  	} else if (IS_BROADWELL(dev_priv)) {
>  		gen8_enable_rps(dev_priv);
>  		__gen6_update_ring_freq(dev_priv);
> -	} else {
> +	} else if (INTEL_GEN(dev_priv) >= 6) {
>  		gen6_enable_rps(dev_priv);
>  		__gen6_update_ring_freq(dev_priv);
> +	} else if (IS_IRONLAKE_M(dev_priv)) {
> +		ironlake_enable_drps(dev_priv);
> +		intel_init_emon(dev_priv);
>  	}
>  
>  	WARN_ON(dev_priv->rps.max_freq < dev_priv->rps.min_freq);
> @@ -6632,18 +6643,45 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>  	WARN_ON(dev_priv->rps.efficient_freq > dev_priv->rps.max_freq);
>  
>  	dev_priv->rps.enabled = true;
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +}
>  
> -	gen6_enable_rps_interrupts(dev_priv);
> +static void __intel_autoenable_gt_powersave(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, typeof(*dev_priv), rps.autoenable_work.work);
> +	struct intel_engine_cs *rcs;
> +	struct drm_i915_gem_request *req;
>  
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> +	if (READ_ONCE(dev_priv->rps.enabled))
> +		return;
> +
> +	rcs = &dev_priv->engine[RCS];
> +	if (rcs->last_context)
> +		return;
> +
> +	if (!rcs->init_context)
> +		return;
> +
> +	intel_runtime_pm_get(dev_priv);
> +	mutex_lock(&dev_priv->drm.struct_mutex);
>  
> +	req = i915_gem_request_alloc(rcs, dev_priv->kernel_context);
> +	if (IS_ERR(req))
> +		goto unlock;
> +
> +	if (!i915.enable_execlists && i915_switch_context(req) == 0)
> +		rcs->init_context(req);
> +	i915_add_request_no_flush(req);
> +
> +unlock:
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> -void intel_enable_gt_powersave(struct drm_i915_private *dev_priv)
> +void intel_autoenable_gt_powersave(struct drm_i915_private *dev_priv)
>  {
> -	/* Powersaving is controlled by the host when inside a VM */
> -	if (intel_vgpu_active(dev_priv))
> +	if (READ_ONCE(dev_priv->rps.enabled))
>  		return;
>  
>  	if (IS_IRONLAKE_M(dev_priv)) {
> @@ -6664,8 +6702,8 @@ void intel_enable_gt_powersave(struct drm_i915_private *dev_priv)
>  		 * paths, so the _noresume version is enough (and in case of
>  		 * runtime resume it's necessary).
>  		 */
> -		if (schedule_delayed_work(&dev_priv->rps.delayed_resume_work,
> -					   round_jiffies_up_relative(HZ)))
> +		if (schedule_delayed_work(&dev_priv->rps.autoenable_work,
> +					  round_jiffies_up_relative(HZ)))
>  			intel_runtime_pm_get_noresume(dev_priv);
>  	}
>  }
> @@ -6675,7 +6713,7 @@ void intel_reset_gt_powersave(struct drm_i915_private *dev_priv)
>  	if (INTEL_INFO(dev_priv)->gen < 6)
>  		return;
>  
> -	gen6_suspend_rps(dev_priv);
> +	gen6_disable_rps_interrupts(dev_priv);
>  	dev_priv->rps.enabled = false;
>  }
>  
> @@ -7785,8 +7823,8 @@ void intel_pm_setup(struct drm_device *dev)
>  	mutex_init(&dev_priv->rps.hw_lock);
>  	spin_lock_init(&dev_priv->rps.client_lock);
>  
> -	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
> -			  intel_gen6_powersave_work);
> +	INIT_DELAYED_WORK(&dev_priv->rps.autoenable_work,
> +			  __intel_autoenable_gt_powersave);
>  	INIT_LIST_HEAD(&dev_priv->rps.clients);
>  	INIT_LIST_HEAD(&dev_priv->rps.semaphores.link);
>  	INIT_LIST_HEAD(&dev_priv->rps.mmioflips.link);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index ff80a81b1a84..eeb4cbce19ff 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -435,7 +435,7 @@ void intel_uncore_sanitize(struct drm_i915_private *dev_priv)
>  	i915.enable_rc6 = sanitize_rc6_option(dev_priv, i915.enable_rc6);
>  
>  	/* BIOS often leaves RC6 enabled, but disable it for hw init */
> -	intel_disable_gt_powersave(dev_priv);
> +	intel_sanitize_gt_powersave(dev_priv);
>  }
>  
>  static void __intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
> -- 
> 2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx