[PATCH 0/5] CHV PM fix & Improvements

[PATCH 0/5] CHV PM fix & Improvements

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

Adding few of PM fixes and Improvements for CHV/VLV.

Deepak S (5):
  drm/i915/chv: Remove Wait for a previous gfx force-off
  drm/i915: Re-adjusting rc6 promotional timer for chv
  drm/i915/chv: Set min freq to efficient frequency on chv
  drm/i915: Modifying RC6 Promotion timer for Media workloads.
  drm/i915: Setup static bias for GPU

 drivers/gpu/drm/i915/i915_drv.c            |  6 ++--
 drivers/gpu/drm/i915/i915_drv.h            |  3 ++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++++++++
 drivers/gpu/drm/i915/i915_reg.h            |  5 +++
 drivers/gpu/drm/i915/intel_drv.h           |  2 ++
 drivers/gpu/drm/i915/intel_lrc.c           | 15 +++++++++
 drivers/gpu/drm/i915/intel_pm.c            | 53 ++++++++++++++++++++++++++++--
 7 files changed, 94 insertions(+), 5 deletions(-)

-- 
1.9.1

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

[PATCH 1/5] drm/i915/chv: Remove Wait for a previous gfx force-off

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

On CHV, PUNIT team confirmed that 'VLV_GFX_CLK_STATUS_BIT' is not a
sticky bit and it will always be set. So ignore Check for previous
Gfx force off during suspend and allow the force clk as part S0ix
Sequence

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4badb23..b88b7b1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1193,11 +1193,13 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
 	int err;
 
 	val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
-	WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
 
 #define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT)
 	/* Wait for a previous force-off to settle */
-	if (force_on) {
+	if (force_on && !IS_CHERRYVIEW(dev_priv->dev)) {
+		/* WARN_ON only for the Valleyview */
+		WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
+
 		err = wait_for(!COND, 20);
 		if (err) {
 			DRM_ERROR("timeout waiting for GFX clock force-off (%08x)\n",
-- 
1.9.1

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

Re: [PATCH 1/5] drm/i915/chv: Remove Wait for a previous gfx force-off

[Ville Syrjälä]

On Thu, Feb 26, 2015 at 08:46:54PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> On CHV, PUNIT team confirmed that 'VLV_GFX_CLK_STATUS_BIT' is not a
> sticky bit and it will always be set. So ignore Check for previous
> Gfx force off during suspend and allow the force clk as part S0ix
> Sequence

Do we need the force clock at all since we don't do any gunit register
save/restore? I tried to peddle a patch to remove it totally in this bug
report, but never got any decent answer back:
https://bugs.freedesktop.org/show_bug.cgi?id=87086

> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 4badb23..b88b7b1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1193,11 +1193,13 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
>  	int err;
>  
>  	val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
> -	WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
>  
>  #define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT)
>  	/* Wait for a previous force-off to settle */
> -	if (force_on) {
> +	if (force_on && !IS_CHERRYVIEW(dev_priv->dev)) {
> +		/* WARN_ON only for the Valleyview */
> +		WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
> +
>  		err = wait_for(!COND, 20);
>  		if (err) {
>  			DRM_ERROR("timeout waiting for GFX clock force-off (%08x)\n",
> -- 
> 1.9.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/chv: Remove Wait for a previous gfx force-off

[Deepak S]

On Thursday 26 February 2015 09:13 PM, Ville Syrjälä wrote:
> On Thu, Feb 26, 2015 at 08:46:54PM +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@linux.intel.com>
>>
>> On CHV, PUNIT team confirmed that 'VLV_GFX_CLK_STATUS_BIT' is not a
>> sticky bit and it will always be set. So ignore Check for previous
>> Gfx force off during suspend and allow the force clk as part S0ix
>> Sequence
> Do we need the force clock at all since we don't do any gunit register
> save/restore? I tried to peddle a patch to remove it totally in this bug
> report, but never got any decent answer back:
> https://bugs.freedesktop.org/show_bug.cgi?id=87086

hmm. your right we might not need. Let me confirm

>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 4badb23..b88b7b1 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1193,11 +1193,13 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
>>   	int err;
>>   
>>   	val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
>> -	WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
>>   
>>   #define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT)
>>   	/* Wait for a previous force-off to settle */
>> -	if (force_on) {
>> +	if (force_on && !IS_CHERRYVIEW(dev_priv->dev)) {
>> +		/* WARN_ON only for the Valleyview */
>> +		WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
>> +
>>   		err = wait_for(!COND, 20);
>>   		if (err) {
>>   			DRM_ERROR("timeout waiting for GFX clock force-off (%08x)\n",
>> -- 
>> 1.9.1

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

Re: [PATCH 1/5] drm/i915/chv: Remove Wait for a previous gfx force-off

[Deepak S]

On Thursday 26 February 2015 09:12 PM, Deepak S wrote:
>
> On Thursday 26 February 2015 09:13 PM, Ville Syrjälä wrote:
>> On Thu, Feb 26, 2015 at 08:46:54PM +0530, deepak.s@linux.intel.com 
>> wrote:
>>> From: Deepak S <deepak.s@linux.intel.com>
>>>
>>> On CHV, PUNIT team confirmed that 'VLV_GFX_CLK_STATUS_BIT' is not a
>>> sticky bit and it will always be set. So ignore Check for previous
>>> Gfx force off during suspend and allow the force clk as part S0ix
>>> Sequence
>> Do we need the force clock at all since we don't do any gunit register
>> save/restore? I tried to peddle a patch to remove it totally in this bug
>> report, but never got any decent answer back:
>> https://bugs.freedesktop.org/show_bug.cgi?id=87086
>
> hmm. your right we might not need. Let me confirm

Hi Ville,

Based on the spec we still  need to follow the Gfx force clk sequence :(
Can you review the patch to skip wait for previous Gfx force-off?
This patch has gone through lot of S0ix testing.

Thanks
Deepak


>>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index 4badb23..b88b7b1 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -1193,11 +1193,13 @@ int vlv_force_gfx_clock(struct 
>>> drm_i915_private *dev_priv, bool force_on)
>>>       int err;
>>>         val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
>>> -    WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
>>>     #define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & 
>>> VLV_GFX_CLK_STATUS_BIT)
>>>       /* Wait for a previous force-off to settle */
>>> -    if (force_on) {
>>> +    if (force_on && !IS_CHERRYVIEW(dev_priv->dev)) {
>>> +        /* WARN_ON only for the Valleyview */
>>> +        WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
>>> +
>>>           err = wait_for(!COND, 20);
>>>           if (err) {
>>>               DRM_ERROR("timeout waiting for GFX clock force-off 
>>> (%08x)\n",
>>> -- 
>>> 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

[PATCH 2/5] drm/i915: Re-adjusting rc6 promotional timer for chv

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

After feedback from the hardware team we are changing the RC6
promotional timer to increase the power saving without
changing performance.

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7dcb5b6..90cb6c9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4799,8 +4799,8 @@ static void cherryview_enable_rps(struct drm_device *dev)
 		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
 	I915_WRITE(GEN6_RC_SLEEP, 0);
 
-	/* TO threshold set to 1750 us ( 0x557 * 1.28 us) */
-	I915_WRITE(GEN6_RC6_THRESHOLD, 0x557);
+	/* TO threshold set to 500 us ( 0x186 * 1.28 us) */
+	I915_WRITE(GEN6_RC6_THRESHOLD, 0x186);
 
 	/* allows RC6 residency counter to work */
 	I915_WRITE(VLV_COUNTER_CONTROL,
-- 
1.9.1

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

Re: [PATCH 2/5] drm/i915: Re-adjusting rc6 promotional timer for chv

[Paulo Zanoni]

2015-02-26 12:16 GMT-03:00  <deepak.s@linux.intel.com>:
> From: Deepak S <deepak.s@linux.intel.com>
>
> After feedback from the hardware team we are changing the RC6
> promotional timer to increase the power saving without
> changing performance.

I can't really say whether this is really what we want since I didn't
do any measurements, but the patch seems to do what it says, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7dcb5b6..90cb6c9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4799,8 +4799,8 @@ static void cherryview_enable_rps(struct drm_device *dev)
>                 I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
>         I915_WRITE(GEN6_RC_SLEEP, 0);
>
> -       /* TO threshold set to 1750 us ( 0x557 * 1.28 us) */
> -       I915_WRITE(GEN6_RC6_THRESHOLD, 0x557);
> +       /* TO threshold set to 500 us ( 0x186 * 1.28 us) */
> +       I915_WRITE(GEN6_RC6_THRESHOLD, 0x186);
>
>         /* allows RC6 residency counter to work */
>         I915_WRITE(VLV_COUNTER_CONTROL,
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 2/5] drm/i915: Re-adjusting rc6 promotional timer for chv

[Deepak S]

On Tuesday 24 March 2015 01:13 AM, Paulo Zanoni wrote:
> 2015-02-26 12:16 GMT-03:00  <deepak.s@linux.intel.com>:
>> From: Deepak S <deepak.s@linux.intel.com>
>>
>> After feedback from the hardware team we are changing the RC6
>> promotional timer to increase the power saving without
>> changing performance.
> I can't really say whether this is really what we want since I didn't
> do any measurements, but the patch seems to do what it says, so:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi Paulo,

Measurements was done by hardware team :)

Thanks
Deepak

>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 7dcb5b6..90cb6c9 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4799,8 +4799,8 @@ static void cherryview_enable_rps(struct drm_device *dev)
>>                  I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
>>          I915_WRITE(GEN6_RC_SLEEP, 0);
>>
>> -       /* TO threshold set to 1750 us ( 0x557 * 1.28 us) */
>> -       I915_WRITE(GEN6_RC6_THRESHOLD, 0x557);
>> +       /* TO threshold set to 500 us ( 0x186 * 1.28 us) */
>> +       I915_WRITE(GEN6_RC6_THRESHOLD, 0x186);
>>
>>          /* allows RC6 residency counter to work */
>>          I915_WRITE(VLV_COUNTER_CONTROL,
>> --
>> 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

[PATCH 3/5] drm/i915/chv: Set min freq to efficient frequency on chv

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

After feedback from the hardware team, now we set the GPU min freq to RPe.
If we drop the freq to RPn, we found that the punit was not setting the
voltage to Vnn, So recommendation is to set min freq to RPe.

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 90cb6c9..e8bd9b9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4755,7 +4755,7 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
 		dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq;
 
 	if (dev_priv->rps.min_freq_softlimit == 0)
-		dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq;
+		dev_priv->rps.min_freq_softlimit = dev_priv->rps.efficient_freq;
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
-- 
1.9.1

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

Re: [PATCH 3/5] drm/i915/chv: Set min freq to efficient frequency on chv

[Chris Wilson]

On Thu, Feb 26, 2015 at 08:46:56PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> After feedback from the hardware team, now we set the GPU min freq to RPe.
> If we drop the freq to RPn, we found that the punit was not setting the
> voltage to Vnn, So recommendation is to set min freq to RPe.

And does it change the voltage at all? Is there really any advantage to
the extra code on idle? Does efficient_freq really consume less power
than min_freq when active (assuming a min_freq/efficient_freq busy
workload i.e. does a workload that would be 100% busy at min_freq
consume less power when run at efficient_freq)?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/chv: Set min freq to efficient frequency on chv

[Deepak S]

On Thursday 26 February 2015 09:42 PM, Chris Wilson wrote:
> On Thu, Feb 26, 2015 at 08:46:56PM +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@linux.intel.com>
>>
>> After feedback from the hardware team, now we set the GPU min freq to RPe.
>> If we drop the freq to RPn, we found that the punit was not setting the
>> voltage to Vnn, So recommendation is to set min freq to RPe.
> And does it change the voltage at all?

Yes Voltage does change when we drop to RPe

> Is there really any advantage to
> the extra code on idle?Does efficient_freq really consume less power
> than min_freq when active (assuming a min_freq/efficient_freq busy
> workload i.e. does a workload that would be 100% busy at min_freq
> consume less power when run at efficient_freq)?

The delta voltage usage between RPn and RPe is very small like close to zero.
Also, if we run workload 100% busy at Rpe we get better performance without much of voltage loss right?
btw, Punit expects us to operate between Rpe & RP0.

> -Chris
>

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

Re: [PATCH 3/5] drm/i915/chv: Set min freq to efficient frequency on chv

[Chris Wilson]

On Wed, Mar 11, 2015 at 07:23:48PM +0530, Deepak S wrote:
> 
> 
> On Thursday 26 February 2015 09:42 PM, Chris Wilson wrote:
> >On Thu, Feb 26, 2015 at 08:46:56PM +0530, deepak.s@linux.intel.com wrote:
> >>From: Deepak S <deepak.s@linux.intel.com>
> >>
> >>After feedback from the hardware team, now we set the GPU min freq to RPe.
> >>If we drop the freq to RPn, we found that the punit was not setting the
> >>voltage to Vnn, So recommendation is to set min freq to RPe.
> >And does it change the voltage at all?
> 
> Yes Voltage does change when we drop to RPe
> 
> >Is there really any advantage to
> >the extra code on idle?Does efficient_freq really consume less power
> >than min_freq when active (assuming a min_freq/efficient_freq busy
> >workload i.e. does a workload that would be 100% busy at min_freq
> >consume less power when run at efficient_freq)?
> 
> The delta voltage usage between RPn and RPe is very small like close to zero.
> Also, if we run workload 100% busy at Rpe we get better performance without much of voltage loss right?
> btw, Punit expects us to operate between Rpe & RP0.

If you need 100% at RPe you obviously can't run at RPn (since that would
lead to dropped frames). The question is if you have a workload that
requires 100% at RPn do you save power if you ran e.g. 80% at RPe?

If the punit only works reliably between RPe and RP0, then the current
RPn is a bit of a misnomer, and that should be the explanation in the
commit log. Definitely do not conflate the idea of executing at RPn and
RPe with the idea of idling at RPn or RPe - this patch affects idle
frequency.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/chv: Set min freq to efficient frequency on chv

[Deepak S]

On Wednesday 11 March 2015 07:36 PM, Chris Wilson wrote:
> On Wed, Mar 11, 2015 at 07:23:48PM +0530, Deepak S wrote:
>>
>> On Thursday 26 February 2015 09:42 PM, Chris Wilson wrote:
>>> On Thu, Feb 26, 2015 at 08:46:56PM +0530, deepak.s@linux.intel.com wrote:
>>>> From: Deepak S <deepak.s@linux.intel.com>
>>>>
>>>> After feedback from the hardware team, now we set the GPU min freq to RPe.
>>>> If we drop the freq to RPn, we found that the punit was not setting the
>>>> voltage to Vnn, So recommendation is to set min freq to RPe.
>>> And does it change the voltage at all?
>> Yes Voltage does change when we drop to RPe
>>
>>> Is there really any advantage to
>>> the extra code on idle?Does efficient_freq really consume less power
>>> than min_freq when active (assuming a min_freq/efficient_freq busy
>>> workload i.e. does a workload that would be 100% busy at min_freq
>>> consume less power when run at efficient_freq)?
>> The delta voltage usage between RPn and RPe is very small like close to zero.
>> Also, if we run workload 100% busy at Rpe we get better performance without much of voltage loss right?
>> btw, Punit expects us to operate between Rpe & RP0.
> If you need 100% at RPe you obviously can't run at RPn (since that would
> lead to dropped frames). The question is if you have a workload that
> requires 100% at RPn do you save power if you ran e.g. 80% at RPe?

We do not expect much of power saving running at RPn.
If we need exact number I need to gather the data.

>
> If the punit only works reliably between RPe and RP0, then the current
> RPn is a bit of a misnomer, and that should be the explanation in the
> commit log. Definitely do not conflate the idea of executing at RPn and
> RPe with the idea of idling at RPn or RPe - this patch affects idle
> frequency.
> -Chris
>
Yes I understand it affects idle freq but running at RPe gives better performance at lower voltage and also punit drops voltage to help save power
I will update the commit msg to explain why we need lower freq at Rpe.

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

[PATCH 4/5] drm/i915: Modifying RC6 Promotion timer for Media workloads.

[deepak.s]

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

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  3 +++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++++++++++++
 drivers/gpu/drm/i915/intel_drv.h           |  2 ++
 drivers/gpu/drm/i915/intel_lrc.c           | 15 +++++++++++++
 drivers/gpu/drm/i915/intel_pm.c            | 35 ++++++++++++++++++++++++++++++
 5 files changed, 70 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a1dd8bc..e33bf0d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1160,6 +1160,9 @@ struct intel_gen6_power_mgmt {
 	 * Must be taken after struct_mutex if nested.
 	 */
 	struct mutex hw_lock;
+
+	/* Delayed work to adjust RC6 promotion timer */
+	struct delayed_work vlv_media_timeout_work;
 };
 
 /* defined intel_pm.c */
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 85a6ada..81f4066 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1272,6 +1272,21 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 	i915_gem_execbuffer_move_to_active(vmas, ring);
 	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
 
+	/* For vlv/chv, modify RC6 promotion timer upon hitting Media workload only
+	 * This will help in better power savings with media scenarios.
+	 */
+	if (((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) &&
+		IS_VALLEYVIEW(dev) && dev_priv->rps.enabled) {
+
+		vlv_modify_rc6_promotion_timer(dev_priv, true);
+
+		/* Start a timer for 1 sec to reset this value to original */
+		mod_delayed_work(dev_priv->wq,
+				&dev_priv->rps.vlv_media_timeout_work,
+				msecs_to_jiffies(1000));
+
+	}
+
 error:
 	kfree(cliprects);
 	return ret;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1fb1529..000f2a6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1234,6 +1234,8 @@ 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,
 			  struct skl_ddb_allocation *ddb /* out */);
+void vlv_modify_rc6_promotion_timer(struct drm_i915_private *dev_priv,
+		bool media_active);
 
 
 /* intel_sdvo.c */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index fcb074b..5f495e73 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -716,6 +716,21 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file,
 	i915_gem_execbuffer_move_to_active(vmas, ring);
 	i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj);
 
+	/*
+	 * CHV: Extend RC6 promotion timer upon hitting Media workload to help
+	 * increase power savings with media scenarios.
+	 */
+	if (((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) &&
+		IS_CHERRYVIEW(dev_priv->dev) && dev_priv->rps.enabled) {
+
+		vlv_modify_rc6_promotion_timer(dev_priv, true);
+
+		/* Start a timer for 1 sec to reset this value to original */
+		mod_delayed_work(dev_priv->wq,
+				&dev_priv->rps.vlv_media_timeout_work,
+				msecs_to_jiffies(1000));
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e8bd9b9..7716be9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3941,6 +3941,9 @@ static void cherryview_disable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	/* Cancel any pending work-item */
+	cancel_delayed_work_sync(&dev_priv->rps.vlv_media_timeout_work);
+
 	I915_WRITE(GEN6_RC_CONTROL, 0);
 }
 
@@ -3952,6 +3955,9 @@ static void valleyview_disable_rps(struct drm_device *dev)
 	 * This what the BIOS expects when going into suspend */
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
+	/* Cancel any pending work-item */
+	cancel_delayed_work_sync(&dev_priv->rps.vlv_media_timeout_work);
+
 	I915_WRITE(GEN6_RC_CONTROL, 0);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
@@ -4857,6 +4863,32 @@ static void cherryview_enable_rps(struct drm_device *dev)
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
 
+void vlv_modify_rc6_promotion_timer(struct drm_i915_private *dev_priv,
+				    bool media_active)
+{
+	if (media_active) {
+		/* TO threshold set to 250 us ( 0xC3 * 1.28 us) */
+		I915_WRITE(GEN6_RC6_THRESHOLD, 0xC3);
+	} else {
+		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);
+		}
+	}
+}
+
+static void vlv_media_timeout_work_func(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv = container_of(work, struct drm_i915_private,
+					    rps.vlv_media_timeout_work.work);
+
+	vlv_modify_rc6_promotion_timer(dev_priv, false);
+}
+
+
 static void valleyview_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -6687,5 +6719,8 @@ void intel_pm_setup(struct drm_device *dev)
 	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
 			  intel_gen6_powersave_work);
 
+	INIT_DELAYED_WORK(&dev_priv->rps.vlv_media_timeout_work,
+				vlv_media_timeout_work_func);
+
 	dev_priv->pm.suspended = false;
 }
-- 
1.9.1

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

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

[Chris Wilson]

On Thu, Feb 26, 2015 at 08:46:57PM +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

I would prefer it if you extended intel_mark_busy() to note the source of
work, and so extended the busy/idle tracking per-ring with the
appropriate hooks for vlv to modify RPS, like how we already do for the
general gen6_rps_busy and gen6_rps_idle.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Deepak S]

On Thursday 26 February 2015 09:38 PM, Chris Wilson wrote:
> On Thu, Feb 26, 2015 at 08:46:57PM +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
> I would prefer it if you extended intel_mark_busy() to note the source of
> work, and so extended the busy/idle tracking per-ring with the
> appropriate hooks for vlv to modify RPS, like how we already do for the
> general gen6_rps_busy and gen6_rps_idle.
> -Chris

Thanks Chris. Its better to use existing Framework


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

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

[deepak.s]

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>
---
 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);
+	}
 
 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

Re: [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@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

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

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

[Deepak S]

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

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

[Chris Wilson]

On Wed, Mar 11, 2015 at 07:07:12PM +0530, Deepak S wrote:
> 
> 
> 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.

It's gen6_rps_busy/gen6_rps_idle. They are called from intel_mark_busy
and intel_mark_idle. It is intel_mark_busy/intel_mark_idle that we want
to extend to cover the VCS case as well. I think if you add a ring
parameter to the functions, we can start specialising per ring and
global state changes. You will then also be in a position to judge what
is the best idle timer (and consider making i915_gem_idle_work_handler
per ring). The goal is simply to evolve the current infrastucture for
idle/busyness handling to cover your use case as well (and hopefully in
the process improving the old/general cases).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Deepak S]

On Wednesday 11 March 2015 07:26 PM, Chris Wilson wrote:
> On Wed, Mar 11, 2015 at 07:07:12PM +0530, Deepak S wrote:
>>
>> 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.
> It's gen6_rps_busy/gen6_rps_idle. They are called from intel_mark_busy
> and intel_mark_idle. It is intel_mark_busy/intel_mark_idle that we want
> to extend to cover the VCS case as well. I think if you add a ring
> parameter to the functions, we can start specialising per ring and
> global state changes. You will then also be in a position to judge what
> is the best idle timer (and consider making i915_gem_idle_work_handler
> per ring). The goal is simply to evolve the current infrastucture for
> idle/busyness handling to cover your use case as well (and hopefully in
> the process improving the old/general cases).
> -Chris
>
Thanks Chris. extending intel_mark_busy/intel_mark_idle
makes sense. I will work on adding the change

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

[PATCH 5/5] drm/i915: Setup static bias for GPU

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

Based on the spec, Setting up static BIAS for GPU to improve the
rps performace.

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  5 +++++
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 55143cb..dd776df 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -662,6 +662,11 @@ enum skl_disp_power_wells {
 #define   FB_FMAX_VMIN_FREQ_LO_SHIFT		27
 #define   FB_FMAX_VMIN_FREQ_LO_MASK		0xf8000000
 
+#define VLV_IOSFB_RPS_OVERRIDE	0x04
+#define VLV_OVERRIDE_RPS_MASK	1
+#define VLV_ENABLE_BIAS_SHARE	(1 << 1)
+#define VLV_BIAS_VAL	(6 << 2)
+
 #define VLV_CZ_CLOCK_TO_MILLI_SEC		100000
 #define VLV_RP_UP_EI_THRESHOLD			90
 #define VLV_RP_DOWN_EI_THRESHOLD		70
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7716be9..5bbb047 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4841,6 +4841,12 @@ static void cherryview_enable_rps(struct drm_device *dev)
 		   GEN6_RP_UP_BUSY_AVG |
 		   GEN6_RP_DOWN_IDLE_AVG);
 
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_RPS_MASK |
+		  VLV_ENABLE_BIAS_SHARE |
+		  VLV_BIAS_VAL;
+	vlv_punit_write(dev_priv, VLV_IOSFB_RPS_OVERRIDE, val);
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 
 	/* RPS code assumes GPLL is used */
@@ -4951,6 +4957,12 @@ static void valleyview_enable_rps(struct drm_device *dev)
 
 	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
 
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_RPS_MASK |
+		  VLV_ENABLE_BIAS_SHARE |
+		  VLV_BIAS_VAL;
+	vlv_punit_write(dev_priv, VLV_IOSFB_RPS_OVERRIDE, val);
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 
 	/* RPS code assumes GPLL is used */
-- 
1.9.1

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

Re: [PATCH 5/5] drm/i915: Setup static bias for GPU

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5840
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  282/282              282/282
ILK                                  308/308              308/308
SNB                                  326/326              326/326
IVB                                  379/379              379/379
BYT                                  294/294              294/294
HSW                 -1              387/387              386/387
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*HSW  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      PASS(2)      DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(15)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx