Re: [Intel-gfx] [PATCH v2 3/4] drm/i915/chv: Set min freq to efficient frequency on chv
On Friday 27 March 2015 03:13 AM, Chris Wilson wrote: On Thu, Mar 26, 2015 at 06:32:15PM -0300, Paulo Zanoni wrote: 2015-03-19 11:14 GMT-03:00 deepa...@linux.intel.com: From: Deepak S deepa...@linux.intel.com After feedback from the hardware team, now we set the GPU min/idel freq to RPe. Punit is expecting us to operate GPU between Rpe Rp0. If we drop the frequency to RPn, punit is failing to change the input voltage to minimum :( Since this is far away from the obvious, I am imagining some programmer from the future looking at this code and imagining min_freq_softlimit was accidentally set to RPe instead of RPn. Can't we add a comment in the code - not just the commit message -, to make it clear that we're doing this since the punit is weird? Another thing which I noticed is that your patch title mentions CHV, but your patch touches the VLV function instead of the CHV one. This also leads me to think that maybe the power measurement experiments you did were done using the non-patched CHV code... Can you please clarify your intentions here? And also maybe redo the power measurements if needed. Also, I think we need at least an ACK from Chris here, especially since he was already discussing the previous version of this patch. If you include a comment like (and note we want to set dev_priv-rps.min_freq not dev_priv-rps.min_freq_softlimit): /* PUnit validated range is only [RPe, RP0] */ dev_priv-rps.min_freq = dev_priv-rps.efficient_freq; and make sure that is set before we derive dev_priv-rps.idle_freq. You can have my Acked-by: Chris Wilson ch...@chris-wilson.co.uk -Chris Thanks Chris. I will address comments rebase the patch. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/4] drm/i915/chv: Set min freq to efficient frequency on chv
On Thu, Mar 26, 2015 at 06:32:15PM -0300, Paulo Zanoni wrote: 2015-03-19 11:14 GMT-03:00 deepa...@linux.intel.com: From: Deepak S deepa...@linux.intel.com After feedback from the hardware team, now we set the GPU min/idel freq to RPe. Punit is expecting us to operate GPU between Rpe Rp0. If we drop the frequency to RPn, punit is failing to change the input voltage to minimum :( Since this is far away from the obvious, I am imagining some programmer from the future looking at this code and imagining min_freq_softlimit was accidentally set to RPe instead of RPn. Can't we add a comment in the code - not just the commit message -, to make it clear that we're doing this since the punit is weird? Another thing which I noticed is that your patch title mentions CHV, but your patch touches the VLV function instead of the CHV one. This also leads me to think that maybe the power measurement experiments you did were done using the non-patched CHV code... Can you please clarify your intentions here? And also maybe redo the power measurements if needed. Also, I think we need at least an ACK from Chris here, especially since he was already discussing the previous version of this patch. If you include a comment like (and note we want to set dev_priv-rps.min_freq not dev_priv-rps.min_freq_softlimit): /* PUnit validated range is only [RPe, RP0] */ dev_priv-rps.min_freq = dev_priv-rps.efficient_freq; and make sure that is set before we derive dev_priv-rps.idle_freq. You can have my Acked-by: Chris Wilson ch...@chris-wilson.co.uk -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: [Intel-gfx] [PATCH v2 3/4] drm/i915/chv: Set min freq to efficient frequency on chv
2015-03-19 11:14 GMT-03:00 deepa...@linux.intel.com: From: Deepak S deepa...@linux.intel.com After feedback from the hardware team, now we set the GPU min/idel freq to RPe. Punit is expecting us to operate GPU between Rpe Rp0. If we drop the frequency to RPn, punit is failing to change the input voltage to minimum :( Since this is far away from the obvious, I am imagining some programmer from the future looking at this code and imagining min_freq_softlimit was accidentally set to RPe instead of RPn. Can't we add a comment in the code - not just the commit message -, to make it clear that we're doing this since the punit is weird? Another thing which I noticed is that your patch title mentions CHV, but your patch touches the VLV function instead of the CHV one. This also leads me to think that maybe the power measurement experiments you did were done using the non-patched CHV code... Can you please clarify your intentions here? And also maybe redo the power measurements if needed. Also, I think we need at least an ACK from Chris here, especially since he was already discussing the previous version of this patch. Thanks, Paulo v2: Change commit message Signed-off-by: Deepak S deepa...@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 6d04147..b9b4d16 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4859,7 +4859,7 @@ static void valleyview_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 -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 3/4] drm/i915/chv: Set min freq to efficient frequency on chv
From: Deepak S deepa...@linux.intel.com After feedback from the hardware team, now we set the GPU min/idel freq to RPe. Punit is expecting us to operate GPU between Rpe Rp0. If we drop the frequency to RPn, punit is failing to change the input voltage to minimum :( v2: Change commit message Signed-off-by: Deepak S deepa...@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 6d04147..b9b4d16 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4859,7 +4859,7 @@ static void valleyview_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
