| Message ID | 1395279079-12704-8-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 19, 2014 at 06:31:14PM -0700, Ben Widawsky wrote: > Programming it outside of the rp0-rp1 range is considered a programming > error. Since we do not know that the previous value would actually be in > the range, program something we've read from the hardware, and therefore > know will work. > > This is potentially an issue for platforms whose ranges are outside the > norms given in the programming guide (ie. early silicon) > > v2: Use RP1 instead of RPn > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> Do you have a reference for GEN6_RC_VIDEO_FREQ? I still have no idea what that controls, nor its valid range. -Chris
On Thu, Mar 20, 2014 at 07:24:38AM +0000, Chris Wilson wrote: > On Wed, Mar 19, 2014 at 06:31:14PM -0700, Ben Widawsky wrote: > > Programming it outside of the rp0-rp1 range is considered a programming > > error. Since we do not know that the previous value would actually be in > > the range, program something we've read from the hardware, and therefore > > know will work. > > > > This is potentially an issue for platforms whose ranges are outside the > > norms given in the programming guide (ie. early silicon) > > > > v2: Use RP1 instead of RPn > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > Do you have a reference for GEN6_RC_VIDEO_FREQ? I still have no idea > what that controls, nor its valid range. > -Chris > I have no reference for the video freq other than the brief mention in the programming guide, and know nothing more than you do about it. It's there because the original spec I had said to program it to 600MHz. The reason for /this/ patch was that I noticed the default values happened to be a *really* close to our max freq. and figured someone, somewhere might get a part whose lower, or upper bound precludes setting the constant provided in the programming guide. Interestingly, the programming guide has been updated since I originally wrote this patch to clearly indicate both of these registers need to be programmed between Rp1-Rp0. So I guess that means that Rp1-Rp0 is the valid range. Therefore, I think we should either proceed with this patch, or create a new patch to avoid writing it at all. The current code seems like the worst solution of all. If you want to argue we can drop the write to GEN6_RPNSWREQ since we do the correct thing after step 6: gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8); I wouldn't be too opposed. I was just trying to follow the spec as closely as possible, and it says to write the register value in this sequence, so I did.
On Sat, Mar 22, 2014 at 11:42:17AM -0700, Ben Widawsky wrote: > On Thu, Mar 20, 2014 at 07:24:38AM +0000, Chris Wilson wrote: > > On Wed, Mar 19, 2014 at 06:31:14PM -0700, Ben Widawsky wrote: > > > Programming it outside of the rp0-rp1 range is considered a programming > > > error. Since we do not know that the previous value would actually be in > > > the range, program something we've read from the hardware, and therefore > > > know will work. > > > > > > This is potentially an issue for platforms whose ranges are outside the > > > norms given in the programming guide (ie. early silicon) > > > > > > v2: Use RP1 instead of RPn > > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > Do you have a reference for GEN6_RC_VIDEO_FREQ? I still have no idea > > what that controls, nor its valid range. > > -Chris > > > > I have no reference for the video freq other than the brief mention in > the programming guide, and know nothing more than you do about it. It's > there because the original spec I had said to program it to 600MHz. The > reason for /this/ patch was that I noticed the default values happened > to be a *really* close to our max freq. and figured someone, somewhere > might get a part whose lower, or upper bound precludes setting the > constant provided in the programming guide. > > Interestingly, the programming guide has been updated since I originally > wrote this patch to clearly indicate both of these registers need to be > programmed between Rp1-Rp0. So I guess that means that Rp1-Rp0 is the > valid range. Therefore, I think we should either proceed with this > patch, or create a new patch to avoid writing it at all. The current > code seems like the worst solution of all. > > If you want to argue we can drop the write to GEN6_RPNSWREQ since we do > the correct thing after step 6: > gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8); > > I wouldn't be too opposed. I was just trying to follow the spec as > closely as possible, and it says to write the register value in this > sequence, so I did. Let's mark that as Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> and move on. Though I may double check to see if I can find some information on the video frequency. -Chris
On Sat, Mar 22, 2014 at 09:06:00PM +0000, Chris Wilson wrote: > On Sat, Mar 22, 2014 at 11:42:17AM -0700, Ben Widawsky wrote: > > On Thu, Mar 20, 2014 at 07:24:38AM +0000, Chris Wilson wrote: > > > On Wed, Mar 19, 2014 at 06:31:14PM -0700, Ben Widawsky wrote: > > > > Programming it outside of the rp0-rp1 range is considered a programming > > > > error. Since we do not know that the previous value would actually be in > > > > the range, program something we've read from the hardware, and therefore > > > > know will work. > > > > > > > > This is potentially an issue for platforms whose ranges are outside the > > > > norms given in the programming guide (ie. early silicon) > > > > > > > > v2: Use RP1 instead of RPn > > > > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > > > Do you have a reference for GEN6_RC_VIDEO_FREQ? I still have no idea > > > what that controls, nor its valid range. > > > -Chris > > > > > > > I have no reference for the video freq other than the brief mention in > > the programming guide, and know nothing more than you do about it. It's > > there because the original spec I had said to program it to 600MHz. The > > reason for /this/ patch was that I noticed the default values happened > > to be a *really* close to our max freq. and figured someone, somewhere > > might get a part whose lower, or upper bound precludes setting the > > constant provided in the programming guide. > > > > Interestingly, the programming guide has been updated since I originally > > wrote this patch to clearly indicate both of these registers need to be > > programmed between Rp1-Rp0. So I guess that means that Rp1-Rp0 is the > > valid range. Therefore, I think we should either proceed with this > > patch, or create a new patch to avoid writing it at all. The current > > code seems like the worst solution of all. > > > > If you want to argue we can drop the write to GEN6_RPNSWREQ since we do > > the correct thing after step 6: > > gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8); > > > > I wouldn't be too opposed. I was just trying to follow the spec as > > closely as possible, and it says to write the register value in this > > sequence, so I did. > > Let's mark that as > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > and move on. Though I may double check to see if I can find some > information on the video frequency. > -Chris > Danvet if/when this is merged, can you please reword the subject: s/RP0/RP1 I'd say it was originally a typo, but that seems unlikely. > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fd68f93..8a64ecc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3285,8 +3285,10 @@ static void gen8_enable_rps(struct drm_device *dev) rc6_mask); /* 4 Program defaults and thresholds for RPS*/ - I915_WRITE(GEN6_RPNSWREQ, HSW_FREQUENCY(10)); /* Request 500 MHz */ - I915_WRITE(GEN6_RC_VIDEO_FREQ, HSW_FREQUENCY(12)); /* Request 600 MHz */ + I915_WRITE(GEN6_RPNSWREQ, + HSW_FREQUENCY(dev_priv->rps.rp1_freq)); + I915_WRITE(GEN6_RC_VIDEO_FREQ, + HSW_FREQUENCY(dev_priv->rps.rp1_freq)); /* NB: Docs say 1s, and 1000000 - which aren't equivalent */ I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 100000000 / 128); /* 1 second timeout */
Programming it outside of the rp0-rp1 range is considered a programming error. Since we do not know that the previous value would actually be in the range, program something we've read from the hardware, and therefore know will work. This is potentially an issue for platforms whose ranges are outside the norms given in the programming guide (ie. early silicon) v2: Use RP1 instead of RPn Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/intel_pm.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)