[Intel-gfx] [PATCH v2] drm/i915: Modifying RC6 Promotion timer for Media workloads.

[Daniel Vetter]

On Thu, Mar 05, 2015 at 09:27:59PM +0530, deepak.s at linux.intel.com wrote:
> From: Deepak S <deepak.s at linux.intel.com>
> 
> In normal cases, RC6 promotion timer is 1700us/500us. This will
> result in more time spent in C1 state. For more residency in
> C6 in case of media workloads, this is changed to 250us.
> Not doing this for 3D workloads as too many C6-C0
> transition delays can result in performance impact.
> 
> v2: Extend GPU busy & idle detection framework for rc6 Promotion
> timer changes (Chris)
> 
> Signed-off-by: Deepak S <deepak.s at linux.intel.com>

I've thougth Chris' idea was to put this into the gen6_rps_boost/idle
functions? You could check from within them I think for whether the vcs is
still busy ... One more comment below.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c      | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_display.c |  3 ++-
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 27 +++++++++++++++++++++++++++
>  4 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3831cc0..85f8aa6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2428,7 +2428,7 @@ int __i915_add_request(struct intel_engine_cs *ring,
>  	struct drm_i915_gem_request *request;
>  	struct intel_ringbuffer *ringbuf;
>  	u32 request_start;
> -	int ret;
> +	int ret, was_empty;
>  
>  	request = ring->outstanding_lazy_request;
>  	if (WARN_ON(request == NULL))
> @@ -2495,6 +2495,7 @@ int __i915_add_request(struct intel_engine_cs *ring,
>  	}
>  
>  	request->emitted_jiffies = jiffies;
> +	was_empty = list_empty(&ring->request_list);
>  	list_add_tail(&request->list, &ring->request_list);
>  	request->file_priv = NULL;
>  
> @@ -2519,6 +2520,10 @@ int __i915_add_request(struct intel_engine_cs *ring,
>  	queue_delayed_work(dev_priv->wq,
>  			   &dev_priv->mm.retire_work,
>  			   round_jiffies_up_relative(HZ));
> +
> +	if ((ring->id == VCS) && was_empty)
> +		vlv_media_promotion_timer_busy(dev_priv);
> +
>  	intel_mark_busy(dev_priv->dev);
>  
>  	return 0;
> @@ -2802,6 +2807,9 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
>  	}
>  
>  	WARN_ON(i915_verify_lists(ring->dev));
> +
> +	if (ring->id == VCS && list_empty(&ring->request_list))
> +		vlv_media_promotion_timer_idle(dev_priv);
>  }
>  
>  bool
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 597c10b..5d121b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9172,8 +9172,9 @@ void intel_mark_idle(struct drm_device *dev)
>  		intel_decrease_pllclock(crtc);
>  	}
>  
> -	if (INTEL_INFO(dev)->gen >= 6)
> +	if (INTEL_INFO(dev)->gen >= 6) {
>  		gen6_rps_idle(dev->dev_private);
> +	}

Uncessary hunk. And a bikeshed: I think generally if we name something
vlv_ we put the platform checks outside of the function. Or have some
other guarantee in place to make sure it's only called on the right
platforms. Otherwise we generally pick an intel_ prefix.
>  
>  out:
>  	intel_runtime_pm_put(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2a6ec4b..f1a90b8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1233,6 +1233,8 @@ void ironlake_teardown_rc6(struct drm_device *dev);
>  void gen6_update_ring_freq(struct drm_device *dev);
>  void gen6_rps_idle(struct drm_i915_private *dev_priv);
>  void gen6_rps_boost(struct drm_i915_private *dev_priv);
> +void vlv_media_promotion_timer_idle(struct drm_i915_private *dev_priv);
> +void vlv_media_promotion_timer_busy(struct drm_i915_private *dev_priv);
>  void ilk_wm_get_hw_state(struct drm_device *dev);
>  void skl_wm_get_hw_state(struct drm_device *dev);
>  void skl_ddb_get_hw_state(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 e710b43..d23b60a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3961,6 +3961,33 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> +void vlv_media_promotion_timer_idle(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	if (!IS_VALLEYVIEW(dev))
> +		return;
> +
> +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> +		/* TO threshold set to 500 us ( 0x186 * 1.28 us) */
> +		I915_WRITE(GEN6_RC6_THRESHOLD, 0x186);
> +	} else {
> +		/* TO threshold set to 1750 us ( 0x557 * 1.28 us) */
> +		I915_WRITE(GEN6_RC6_THRESHOLD, 0x557);
> +	}
> +}
> +
> +void vlv_media_promotion_timer_busy(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	if (!IS_VALLEYVIEW(dev))
> +		return;
> +
> +	/* TO threshold set to 250 us ( 0xC3 * 1.28 us) */
> +	I915_WRITE(GEN6_RC6_THRESHOLD, 0xC3);
> +}
> +
>  void intel_set_rps(struct drm_device *dev, u8 val)
>  {
>  	if (IS_VALLEYVIEW(dev))
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch