* [PATCH 1/3] drm/i915: don't use stolen space on VLV
@ 2013-05-01 23:09 Jesse Barnes
2013-05-01 23:09 ` [PATCH 2/3] drm/i915: read current freq from Punit " Jesse Barnes
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jesse Barnes @ 2013-05-01 23:09 UTC (permalink / raw)
To: intel-gfx
BIOS stuffs things here we can't clobber.
Signed-off-by: Jesse Barnes <jbarnes@virtuosugeek.org>
---
drivers/gpu/drm/i915/i915_gem_stolen.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 67d3510..85b3ea9 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -184,6 +184,9 @@ int i915_gem_init_stolen(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (IS_VALLEYVIEW(dev))
+ return 0;
+
dev_priv->mm.stolen_base = i915_stolen_to_physical(dev);
if (dev_priv->mm.stolen_base == 0)
return 0;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/3] drm/i915: read current freq from Punit on VLV 2013-05-01 23:09 [PATCH 1/3] drm/i915: don't use stolen space on VLV Jesse Barnes @ 2013-05-01 23:09 ` Jesse Barnes 2013-05-02 3:14 ` Kenneth Graunke 2013-05-01 23:09 ` [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection Jesse Barnes 2013-05-02 1:00 ` [PATCH 1/3] drm/i915: don't use stolen space on VLV Ben Widawsky 2 siblings, 1 reply; 10+ messages in thread From: Jesse Barnes @ 2013-05-01 23:09 UTC (permalink / raw) To: intel-gfx Instead of returning the cached value, which is just what the kernel requested. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_sysfs.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index ca00df2..c0d7875 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -212,10 +212,13 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev, int ret; mutex_lock(&dev_priv->rps.hw_lock); - if (IS_VALLEYVIEW(dev_priv->dev)) - ret = vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.cur_delay); - else + if (IS_VALLEYVIEW(dev_priv->dev)) { + u32 freq; + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &freq); + ret = vlv_gpu_freq(dev_priv->mem_freq, (freq >> 8) & 0xff); + } else { ret = dev_priv->rps.cur_delay * GT_FREQUENCY_MULTIPLIER; + } mutex_unlock(&dev_priv->rps.hw_lock); return snprintf(buf, PAGE_SIZE, "%d\n", ret); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/i915: read current freq from Punit on VLV 2013-05-01 23:09 ` [PATCH 2/3] drm/i915: read current freq from Punit " Jesse Barnes @ 2013-05-02 3:14 ` Kenneth Graunke 0 siblings, 0 replies; 10+ messages in thread From: Kenneth Graunke @ 2013-05-02 3:14 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On 05/01/2013 04:09 PM, Jesse Barnes wrote: > Instead of returning the cached value, which is just what the kernel > requested. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_sysfs.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > index ca00df2..c0d7875 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -212,10 +212,13 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev, > int ret; > > mutex_lock(&dev_priv->rps.hw_lock); > - if (IS_VALLEYVIEW(dev_priv->dev)) > - ret = vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.cur_delay); > - else > + if (IS_VALLEYVIEW(dev_priv->dev)) { > + u32 freq; > + valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &freq); > + ret = vlv_gpu_freq(dev_priv->mem_freq, (freq >> 8) & 0xff); > + } else { > ret = dev_priv->rps.cur_delay * GT_FREQUENCY_MULTIPLIER; > + } > mutex_unlock(&dev_priv->rps.hw_lock); > > return snprintf(buf, PAGE_SIZE, "%d\n", ret); I'd really like to see this for the rest of the platforms as well. Knowing what the kernel requested isn't nearly as interesting as knowing the real value. But this VLV patch looks great, so: Reviewed-by: Kenneth Graunke <kenneth@whitecape.org> I'm not planning on reviewing patch 1; I have no idea about any of that stuff. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection 2013-05-01 23:09 [PATCH 1/3] drm/i915: don't use stolen space on VLV Jesse Barnes 2013-05-01 23:09 ` [PATCH 2/3] drm/i915: read current freq from Punit " Jesse Barnes @ 2013-05-01 23:09 ` Jesse Barnes 2013-05-02 1:26 ` Ben Widawsky 2013-05-02 1:00 ` [PATCH 1/3] drm/i915: don't use stolen space on VLV Ben Widawsky 2 siblings, 1 reply; 10+ messages in thread From: Jesse Barnes @ 2013-05-01 23:09 UTC (permalink / raw) To: intel-gfx We need to catch the invalid case and override it. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 0f4b46e..0a3b0b3 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2902,7 +2902,21 @@ static void valleyview_enable_rps(struct drm_device *dev) GEN7_RC_CTL_TO_MODE); valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); - dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3); + switch ((val >> 6) & 3) { + case 0: + dev_priv->mem_freq = 800; + break; + case 1: + dev_priv->mem_freq = 1066; + break; + case 2: + dev_priv->mem_freq = 1333; + break; + case 3: + DRM_ERROR("invalid mem freq, assuming 800MHz\n"); + dev_priv->mem_freq = 800; + break; + } DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no"); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection 2013-05-01 23:09 ` [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection Jesse Barnes @ 2013-05-02 1:26 ` Ben Widawsky 2013-05-02 1:38 ` Kenneth Graunke 2013-05-02 1:40 ` Ben Widawsky 0 siblings, 2 replies; 10+ messages in thread From: Ben Widawsky @ 2013-05-02 1:26 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Wed, May 01, 2013 at 04:09:22PM -0700, Jesse Barnes wrote: > We need to catch the invalid case and override it. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 0f4b46e..0a3b0b3 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2902,7 +2902,21 @@ static void valleyview_enable_rps(struct drm_device *dev) > GEN7_RC_CTL_TO_MODE); > > valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); > - dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3); > + switch ((val >> 6) & 3) { > + case 0: > + dev_priv->mem_freq = 800; > + break; > + case 1: > + dev_priv->mem_freq = 1066; > + break; > + case 2: > + dev_priv->mem_freq = 1333; > + break; > + case 3: > + DRM_ERROR("invalid mem freq, assuming 800MHz\n"); > + dev_priv->mem_freq = 800; > + break; > + } > DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); > > DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no"); I guess it doesn't handle the last case, but: dev_priv->mem_freq = 800 + (266 * (val))) + val/2 or u32 freqs[] = {800,1066,1333,800}; dev_priv->mem_freq = freqs[(val >> 6) & 3]; Are two ways I would have used before making a switch block :P Just a thought, but perhaps we don't want to enable RPS if we can't reliably figure out the memory freq. (case 3) Reviewed-by: Ben Widawsky <ben@bwidawsk.net> -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection 2013-05-02 1:26 ` Ben Widawsky @ 2013-05-02 1:38 ` Kenneth Graunke 2013-05-02 1:45 ` Ben Widawsky 2013-05-02 1:40 ` Ben Widawsky 1 sibling, 1 reply; 10+ messages in thread From: Kenneth Graunke @ 2013-05-02 1:38 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On 05/01/2013 06:26 PM, Ben Widawsky wrote: > On Wed, May 01, 2013 at 04:09:22PM -0700, Jesse Barnes wrote: >> We need to catch the invalid case and override it. >> >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Jesse, it's actually worse than that: Your parenthesis were off in the previous version, causing it to do the multiplication by 266 before the bitwise-and: 800 + ((266 * (val >> 6)) & 3) This meant that you always got 800 or 802 Mhz. Also, even when corrected, your previous formula would return 1332 Mhz instead of 1333 Mhz, which is slightly off and doesn't work since you use it in explicit switch statements. It might be worth mentioning in the commit message. >> --- >> drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 0f4b46e..0a3b0b3 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -2902,7 +2902,21 @@ static void valleyview_enable_rps(struct drm_device *dev) >> GEN7_RC_CTL_TO_MODE); >> >> valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); >> - dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3); >> + switch ((val >> 6) & 3) { >> + case 0: >> + dev_priv->mem_freq = 800; >> + break; >> + case 1: >> + dev_priv->mem_freq = 1066; >> + break; >> + case 2: >> + dev_priv->mem_freq = 1333; >> + break; >> + case 3: >> + DRM_ERROR("invalid mem freq, assuming 800MHz\n"); >> + dev_priv->mem_freq = 800; >> + break; >> + } >> DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); >> >> DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no"); > > > I guess it doesn't handle the last case, but: > dev_priv->mem_freq = 800 + (266 * (val))) + val/2 Don't use this method. Presumably Ben meant (val >> 6) here, but even with that fix this formula returns 1067 rather than 1066, which is wrong. > or > > u32 freqs[] = {800,1066,1333,800}; > dev_priv->mem_freq = freqs[(val >> 6) & 3]; I like this table based approach best. It's much more succinct than the switch statement, and exact. Might be worth preserving the DRM_ERROR. > Are two ways I would have used before making a switch block :P > > Just a thought, but perhaps we don't want to enable RPS if we can't > reliably figure out the memory freq. (case 3) > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> For either the switch or Ben's table: Reviewed-by: Kenneth Graunke <kenneth@whitecape.org> Incidentally, my Punit returns 3 here, but I believe the memory is actually 1066. Not sure what the deal is. --Ken ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection 2013-05-02 1:38 ` Kenneth Graunke @ 2013-05-02 1:45 ` Ben Widawsky 2013-05-02 3:16 ` Kenneth Graunke 0 siblings, 1 reply; 10+ messages in thread From: Ben Widawsky @ 2013-05-02 1:45 UTC (permalink / raw) To: Kenneth Graunke; +Cc: intel-gfx On Wed, May 01, 2013 at 06:38:17PM -0700, Kenneth Graunke wrote: > On 05/01/2013 06:26 PM, Ben Widawsky wrote: > >On Wed, May 01, 2013 at 04:09:22PM -0700, Jesse Barnes wrote: > >>We need to catch the invalid case and override it. > >> > >>Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > Jesse, it's actually worse than that: > > Your parenthesis were off in the previous version, causing it to do > the multiplication by 266 before the bitwise-and: > > 800 + ((266 * (val >> 6)) & 3) > > This meant that you always got 800 or 802 Mhz. > > Also, even when corrected, your previous formula would return 1332 > Mhz instead of 1333 Mhz, which is slightly off and doesn't work > since you use it in explicit switch statements. > > It might be worth mentioning in the commit message. > > >>--- > >> drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++++++- > >> 1 file changed, 15 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >>index 0f4b46e..0a3b0b3 100644 > >>--- a/drivers/gpu/drm/i915/intel_pm.c > >>+++ b/drivers/gpu/drm/i915/intel_pm.c > >>@@ -2902,7 +2902,21 @@ static void valleyview_enable_rps(struct drm_device *dev) > >> GEN7_RC_CTL_TO_MODE); > >> > >> valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); > >>- dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3); > >>+ switch ((val >> 6) & 3) { > >>+ case 0: > >>+ dev_priv->mem_freq = 800; > >>+ break; > >>+ case 1: > >>+ dev_priv->mem_freq = 1066; > >>+ break; > >>+ case 2: > >>+ dev_priv->mem_freq = 1333; > >>+ break; > >>+ case 3: > >>+ DRM_ERROR("invalid mem freq, assuming 800MHz\n"); > >>+ dev_priv->mem_freq = 800; > >>+ break; > >>+ } > >> DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); > >> > >> DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no"); > > > > > >I guess it doesn't handle the last case, but: > >dev_priv->mem_freq = 800 + (266 * (val))) + val/2 > > Don't use this method. Presumably Ben meant (val >> 6) here, but > even with that fix this formula returns 1067 rather than 1066, which > is wrong. You're right about the missing shift, but otherwise the formula is correct. Val is 1 in the case of 1066MHz... > > >or > > > >u32 freqs[] = {800,1066,1333,800}; > >dev_priv->mem_freq = freqs[(val >> 6) & 3]; > > I like this table based approach best. It's much more succinct than > the switch statement, and exact. Might be worth preserving the > DRM_ERROR. > > >Are two ways I would have used before making a switch block :P > > > >Just a thought, but perhaps we don't want to enable RPS if we can't > >reliably figure out the memory freq. (case 3) > > > >Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > > For either the switch or Ben's table: > Reviewed-by: Kenneth Graunke <kenneth@whitecape.org> > > Incidentally, my Punit returns 3 here, but I believe the memory is > actually 1066. Not sure what the deal is. > > --Ken > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection 2013-05-02 1:45 ` Ben Widawsky @ 2013-05-02 3:16 ` Kenneth Graunke 0 siblings, 0 replies; 10+ messages in thread From: Kenneth Graunke @ 2013-05-02 3:16 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On 05/01/2013 06:45 PM, Ben Widawsky wrote: > On Wed, May 01, 2013 at 06:38:17PM -0700, Kenneth Graunke wrote: >> On 05/01/2013 06:26 PM, Ben Widawsky wrote: >>> On Wed, May 01, 2013 at 04:09:22PM -0700, Jesse Barnes wrote: >>>> We need to catch the invalid case and override it. >>>> >>>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> >> Jesse, it's actually worse than that: >> >> Your parenthesis were off in the previous version, causing it to do >> the multiplication by 266 before the bitwise-and: >> >> 800 + ((266 * (val >> 6)) & 3) >> >> This meant that you always got 800 or 802 Mhz. >> >> Also, even when corrected, your previous formula would return 1332 >> Mhz instead of 1333 Mhz, which is slightly off and doesn't work >> since you use it in explicit switch statements. >> >> It might be worth mentioning in the commit message. >> >>>> --- >>>> drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++++++- >>>> 1 file changed, 15 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >>>> index 0f4b46e..0a3b0b3 100644 >>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>> @@ -2902,7 +2902,21 @@ static void valleyview_enable_rps(struct drm_device *dev) >>>> GEN7_RC_CTL_TO_MODE); >>>> >>>> valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); >>>> - dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3); >>>> + switch ((val >> 6) & 3) { >>>> + case 0: >>>> + dev_priv->mem_freq = 800; >>>> + break; >>>> + case 1: >>>> + dev_priv->mem_freq = 1066; >>>> + break; >>>> + case 2: >>>> + dev_priv->mem_freq = 1333; >>>> + break; >>>> + case 3: >>>> + DRM_ERROR("invalid mem freq, assuming 800MHz\n"); >>>> + dev_priv->mem_freq = 800; >>>> + break; >>>> + } >>>> DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); >>>> >>>> DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no"); >>> >>> >>> I guess it doesn't handle the last case, but: >>> dev_priv->mem_freq = 800 + (266 * (val))) + val/2 >> >> Don't use this method. Presumably Ben meant (val >> 6) here, but >> even with that fix this formula returns 1067 rather than 1066, which >> is wrong. > > You're right about the missing shift, but otherwise the formula is > correct. Val is 1 in the case of 1066MHz... > >> >>> or >>> >>> u32 freqs[] = {800,1066,1333,800}; >>> dev_priv->mem_freq = freqs[(val >> 6) & 3]; >> >> I like this table based approach best. It's much more succinct than >> the switch statement, and exact. Might be worth preserving the >> DRM_ERROR. >> >>> Are two ways I would have used before making a switch block :P >>> >>> Just a thought, but perhaps we don't want to enable RPS if we can't >>> reliably figure out the memory freq. (case 3) >>> >>> Reviewed-by: Ben Widawsky <ben@bwidawsk.net> >> >> For either the switch or Ben's table: >> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org> >> >> Incidentally, my Punit returns 3 here, but I believe the memory is >> actually 1066. Not sure what the deal is. You're right - my mistake, sorry. I had been doing this in a python interpreter and accidentally put 0b01 for the first val and 0b10 for the second val. --Ken ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection 2013-05-02 1:26 ` Ben Widawsky 2013-05-02 1:38 ` Kenneth Graunke @ 2013-05-02 1:40 ` Ben Widawsky 1 sibling, 0 replies; 10+ messages in thread From: Ben Widawsky @ 2013-05-02 1:40 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Wed, May 01, 2013 at 06:26:28PM -0700, Ben Widawsky wrote: > On Wed, May 01, 2013 at 04:09:22PM -0700, Jesse Barnes wrote: > > We need to catch the invalid case and override it. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 0f4b46e..0a3b0b3 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -2902,7 +2902,21 @@ static void valleyview_enable_rps(struct drm_device *dev) > > GEN7_RC_CTL_TO_MODE); > > > > valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &val); > > - dev_priv->mem_freq = 800 + (266 * (val >> 6) & 3); > > + switch ((val >> 6) & 3) { > > + case 0: > > + dev_priv->mem_freq = 800; > > + break; > > + case 1: > > + dev_priv->mem_freq = 1066; > > + break; > > + case 2: > > + dev_priv->mem_freq = 1333; > > + break; > > + case 3: > > + DRM_ERROR("invalid mem freq, assuming 800MHz\n"); > > + dev_priv->mem_freq = 800; > > + break; > > + } > > DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); > > > > DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no"); > > > I guess it doesn't handle the last case, but: > dev_priv->mem_freq = 800 + (266 * (val))) + val/2 > conveniently, I've given you the reason not to use this one... error prone. It's missing a: val = (val >> 6) & 3 before the math. > or > > u32 freqs[] = {800,1066,1333,800}; > dev_priv->mem_freq = freqs[(val >> 6) & 3]; > > Are two ways I would have used before making a switch block :P > > Just a thought, but perhaps we don't want to enable RPS if we can't > reliably figure out the memory freq. (case 3) > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > > -- > Ben Widawsky, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/i915: don't use stolen space on VLV 2013-05-01 23:09 [PATCH 1/3] drm/i915: don't use stolen space on VLV Jesse Barnes 2013-05-01 23:09 ` [PATCH 2/3] drm/i915: read current freq from Punit " Jesse Barnes 2013-05-01 23:09 ` [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection Jesse Barnes @ 2013-05-02 1:00 ` Ben Widawsky 2 siblings, 0 replies; 10+ messages in thread From: Ben Widawsky @ 2013-05-02 1:00 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Wed, May 01, 2013 at 04:09:20PM -0700, Jesse Barnes wrote: > BIOS stuffs things here we can't clobber. > > Signed-off-by: Jesse Barnes <jbarnes@virtuosugeek.org> As I said in private email, I think the quirk is a bit better suited in i915_stolen_to_physical(), and the commit message *could* explain things a bit clearer, oh, and you're welcome. Reviewed-by: Ben Widawsky <ben@bwidawsk.net> -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-05-02 3:15 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-05-01 23:09 [PATCH 1/3] drm/i915: don't use stolen space on VLV Jesse Barnes 2013-05-01 23:09 ` [PATCH 2/3] drm/i915: read current freq from Punit " Jesse Barnes 2013-05-02 3:14 ` Kenneth Graunke 2013-05-01 23:09 ` [PATCH 3/3] drm/i915: go back to switch for VLV mem freq detection Jesse Barnes 2013-05-02 1:26 ` Ben Widawsky 2013-05-02 1:38 ` Kenneth Graunke 2013-05-02 1:45 ` Ben Widawsky 2013-05-02 3:16 ` Kenneth Graunke 2013-05-02 1:40 ` Ben Widawsky 2013-05-02 1:00 ` [PATCH 1/3] drm/i915: don't use stolen space on VLV Ben Widawsky
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.