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

[Deepak S <deepak.s@linux.intel.com>]

On Friday 06 March 2015 10:10 PM, Daniel Vetter wrote:
> On Thu, Mar 05, 2015 at 09:27:59PM +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@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@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

Hi Daniel,

gen6_rps_boost/idle will be called only for RCS right? Also we get gen6_rps_boost during  __wait_request
But we want to program promotion timer when we add request to VCS to apply the value immediately.

Thanks
Deepak

>> ---
>>   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.

Thanks Daniel. I will create intel_ prefix, we might need to extend this for future platforms.

>>   
>>   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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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