| Message ID | 1398067454-7581-4-git-send-email-deepak.s@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 21, 2014 at 01:34:07PM +0530, deepak.s@linux.intel.com wrote: > From: Deepak S <deepak.s@linux.intel.com> > > v2: Configure PCBR if BIOS fails allocate pcbr (deepak) > > v3: Fix PCBR condition check during CHV RC6 Enable flag set > > Signed-off-by: Deepak S <deepak.s@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++- > 2 files changed, 99 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index b951d61..7090b42 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -5134,6 +5134,7 @@ enum punit_power_well { > #define GEN6_GT_GFX_RC6 0x138108 > #define GEN6_GT_GFX_RC6p 0x13810C > #define GEN6_GT_GFX_RC6pp 0x138110 > +#define VLV_PCBR_ADDR_SHIFT 12 > > #define GEN6_PCODE_MAILBOX 0x138124 > #define GEN6_PCODE_READY (1<<31) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index f3c5bce..421a4cc 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev) > gen6_disable_rps_interrupts(dev); > } > > +static void cherryview_disable_rps(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + I915_WRITE(GEN6_RC_CONTROL, 0); > + > + if (dev_priv->vlv_pctx) { > + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); > + dev_priv->vlv_pctx = NULL; > + } > +} > + > static void valleyview_disable_rps(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv) > dev_priv->vlv_pctx->stolen->start); > } > > +static void cherryview_setup_pctx(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned long pctx_paddr; > + struct i915_gtt *gtt = &dev_priv->gtt; > + u32 pcbr; > + int pctx_size = 32*1024; > + > + pcbr = I915_READ(VLV_PCBR); > + if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { > + /* > + * From the Gunit register HAS: > + * The Gfx driver is expected to program this register and ensure > + * proper allocation within Gfx stolen memory. For example, this > + * register should be programmed such than the PCBR range does not > + * overlap with other relevant ranges. > + */ > + pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size); > + I915_WRITE(VLV_PCBR, pctx_paddr); > + } > +} > + Is there a reason we did not follow the same idioms as Valleyview? Shouldn't we be building a stolen object like we do there, and then using that? Furthermore, we need to make sure we make the stolen allocator aware for the case where pcbr is not zero, like we do for valleyview. I think the best solution here is to try to combine the valleyview and cherryview logic for this function. Extract out size, and most of the rest looks pretty similar. For enabling, I am fine with it as is though provided it's hidden by preliminary flag. > static void valleyview_setup_pctx(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev) > dev_priv->vlv_pctx = NULL; > } > > +static void cherryview_enable_rps(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_ring_buffer *ring; > + u32 gtfifodbg, rc6_mode = 0, pcbr; > + int i; > + > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > + > + if ((gtfifodbg = I915_READ(GTFIFODBG))) { > + DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n", > + gtfifodbg); > + I915_WRITE(GTFIFODBG, gtfifodbg); > + } > + > + cherryview_setup_pctx(dev); > + > + /* 1a & 1b: Get forcewake during program sequence. Although the driver > + * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ > + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > + > + /* 2a: Program RC6 thresholds.*/ > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16); > + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */ > + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */ > + > + for_each_ring(ring, dev_priv, i) > + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); > + > + I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */ > + > + /* allows RC6 residency counter to work */ > + I915_WRITE(VLV_COUNTER_CONTROL, > + _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH | > + VLV_MEDIA_RC6_COUNT_EN | > + VLV_RENDER_RC6_COUNT_EN)); > + > + /* Todo: If BIOS has not configured PCBR > + * then allocate in BIOS Reserved */ > + > + /* For now we assume BIOS is allocating and populating the PCBR */ > + pcbr = I915_READ(VLV_PCBR); > + > + DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr); > + > + /* 3: Enable RC6 */ > + if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) && > + (pcbr >> VLV_PCBR_ADDR_SHIFT)) > + rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL; > + > + I915_WRITE(GEN6_RC_CONTROL, rc6_mode); > + > + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > +} > + Can you send me a link to the doc for this off-list? I am having trouble tracking it down. > static void valleyview_enable_rps(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) > cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); > cancel_work_sync(&dev_priv->rps.work); > mutex_lock(&dev_priv->rps.hw_lock); > - if (IS_VALLEYVIEW(dev)) > + if (IS_CHERRYVIEW(dev)) > + cherryview_disable_rps(dev); > + else if (IS_VALLEYVIEW(dev)) > valleyview_disable_rps(dev); > else > gen6_disable_rps(dev); > @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work) > > mutex_lock(&dev_priv->rps.hw_lock); > > - if (IS_VALLEYVIEW(dev)) { > + if (IS_CHERRYVIEW(dev)) { > + cherryview_enable_rps(dev); > + } else if (IS_VALLEYVIEW(dev)) { > valleyview_enable_rps(dev); > } else if (IS_BROADWELL(dev)) { > gen8_enable_rps(dev); > @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev) > ironlake_enable_rc6(dev); > intel_init_emon(dev); > } else if (IS_GEN6(dev) || IS_GEN7(dev)) { > + if (IS_VALLEYVIEW(dev)) > + valleyview_setup_pctx(dev); Spurious hunk? > /* > * PCU communication is slow and this doesn't need to be > * done at any specific time, so do this out of our fast path > @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev) > dev_priv->mem_freq = 1333; > break; > } > + Spurious hunk? > DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); > > dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv); I think for enabling this patch is fine as is. It does need some of the fixes I mentioned about. So I'd suggest adding FIXMes with what I request, and calling this Acked-by: Ben Widawsky <ben@bwidawsk.net> To move it up to a reviewed by, I need to find the PM doc (which I probably have, just don't know it), and I'd like the setup function fixed.
Also, s/Cheeryview/Cherryview On Fri, Apr 25, 2014 at 02:42:26PM -0700, Ben Widawsky wrote: > On Mon, Apr 21, 2014 at 01:34:07PM +0530, deepak.s@linux.intel.com wrote: > > From: Deepak S <deepak.s@linux.intel.com> > > > > v2: Configure PCBR if BIOS fails allocate pcbr (deepak) > > > > v3: Fix PCBR condition check during CHV RC6 Enable flag set > > > > Signed-off-by: Deepak S <deepak.s@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 99 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index b951d61..7090b42 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -5134,6 +5134,7 @@ enum punit_power_well { > > #define GEN6_GT_GFX_RC6 0x138108 > > #define GEN6_GT_GFX_RC6p 0x13810C > > #define GEN6_GT_GFX_RC6pp 0x138110 > > +#define VLV_PCBR_ADDR_SHIFT 12 > > > > #define GEN6_PCODE_MAILBOX 0x138124 > > #define GEN6_PCODE_READY (1<<31) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index f3c5bce..421a4cc 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev) > > gen6_disable_rps_interrupts(dev); > > } > > > > +static void cherryview_disable_rps(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + > > + I915_WRITE(GEN6_RC_CONTROL, 0); > > + > > + if (dev_priv->vlv_pctx) { > > + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); > > + dev_priv->vlv_pctx = NULL; > > + } > > +} > > + > > static void valleyview_disable_rps(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv) > > dev_priv->vlv_pctx->stolen->start); > > } > > > > +static void cherryview_setup_pctx(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + unsigned long pctx_paddr; > > + struct i915_gtt *gtt = &dev_priv->gtt; > > + u32 pcbr; > > + int pctx_size = 32*1024; > > + > > + pcbr = I915_READ(VLV_PCBR); > > + if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { > > + /* > > + * From the Gunit register HAS: > > + * The Gfx driver is expected to program this register and ensure > > + * proper allocation within Gfx stolen memory. For example, this > > + * register should be programmed such than the PCBR range does not > > + * overlap with other relevant ranges. > > + */ > > + pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size); > > + I915_WRITE(VLV_PCBR, pctx_paddr); > > + } > > +} > > + > > Is there a reason we did not follow the same idioms as Valleyview? > Shouldn't we be building a stolen object like we do there, and then > using that? > > Furthermore, we need to make sure we make the stolen allocator aware for > the case where pcbr is not zero, like we do for valleyview. > > I think the best solution here is to try to combine the valleyview and > cherryview logic for this function. Extract out size, and most of the > rest looks pretty similar. > > For enabling, I am fine with it as is though provided it's hidden by > preliminary flag. > > > static void valleyview_setup_pctx(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev) > > dev_priv->vlv_pctx = NULL; > > } > > > > +static void cherryview_enable_rps(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct intel_ring_buffer *ring; > > + u32 gtfifodbg, rc6_mode = 0, pcbr; > > + int i; > > + > > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > > + > > + if ((gtfifodbg = I915_READ(GTFIFODBG))) { > > + DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n", > > + gtfifodbg); > > + I915_WRITE(GTFIFODBG, gtfifodbg); > > + } > > + > > + cherryview_setup_pctx(dev); > > + > > + /* 1a & 1b: Get forcewake during program sequence. Although the driver > > + * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ > > + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > > + > > + /* 2a: Program RC6 thresholds.*/ > > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16); > > + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */ > > + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */ > > + > > + for_each_ring(ring, dev_priv, i) > > + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); > > + > > + I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */ > > + > > + /* allows RC6 residency counter to work */ > > + I915_WRITE(VLV_COUNTER_CONTROL, > > + _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH | > > + VLV_MEDIA_RC6_COUNT_EN | > > + VLV_RENDER_RC6_COUNT_EN)); > > + > > + /* Todo: If BIOS has not configured PCBR > > + * then allocate in BIOS Reserved */ > > + > > + /* For now we assume BIOS is allocating and populating the PCBR */ > > + pcbr = I915_READ(VLV_PCBR); > > + > > + DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr); > > + > > + /* 3: Enable RC6 */ > > + if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) && > > + (pcbr >> VLV_PCBR_ADDR_SHIFT)) > > + rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL; > > + > > + I915_WRITE(GEN6_RC_CONTROL, rc6_mode); > > + > > + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > > +} > > + > > Can you send me a link to the doc for this off-list? I am having trouble > tracking it down. > > > static void valleyview_enable_rps(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) > > cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); > > cancel_work_sync(&dev_priv->rps.work); > > mutex_lock(&dev_priv->rps.hw_lock); > > - if (IS_VALLEYVIEW(dev)) > > + if (IS_CHERRYVIEW(dev)) > > + cherryview_disable_rps(dev); > > + else if (IS_VALLEYVIEW(dev)) > > valleyview_disable_rps(dev); > > else > > gen6_disable_rps(dev); > > @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work) > > > > mutex_lock(&dev_priv->rps.hw_lock); > > > > - if (IS_VALLEYVIEW(dev)) { > > + if (IS_CHERRYVIEW(dev)) { > > + cherryview_enable_rps(dev); > > + } else if (IS_VALLEYVIEW(dev)) { > > valleyview_enable_rps(dev); > > } else if (IS_BROADWELL(dev)) { > > gen8_enable_rps(dev); > > @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev) > > ironlake_enable_rc6(dev); > > intel_init_emon(dev); > > } else if (IS_GEN6(dev) || IS_GEN7(dev)) { > > + if (IS_VALLEYVIEW(dev)) > > + valleyview_setup_pctx(dev); > > Spurious hunk? > > > /* > > * PCU communication is slow and this doesn't need to be > > * done at any specific time, so do this out of our fast path > > @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev) > > dev_priv->mem_freq = 1333; > > break; > > } > > + > > Spurious hunk? > > > DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); > > > > dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv); > > I think for enabling this patch is fine as is. It does need some of the > fixes I mentioned about. So I'd suggest adding FIXMes with what I > request, and calling this > Acked-by: Ben Widawsky <ben@bwidawsk.net> > > To move it up to a reviewed by, I need to find the PM doc (which I > probably have, just don't know it), and I'd like the setup function > fixed. > > > > -- > Ben Widawsky, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2014-04-21 at 13:34 +0530, deepak.s@linux.intel.com wrote: > From: Deepak S <deepak.s@linux.intel.com> > > v2: Configure PCBR if BIOS fails allocate pcbr (deepak) > > v3: Fix PCBR condition check during CHV RC6 Enable flag set > > Signed-off-by: Deepak S <deepak.s@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++- > 2 files changed, 99 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index b951d61..7090b42 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -5134,6 +5134,7 @@ enum punit_power_well { > #define GEN6_GT_GFX_RC6 0x138108 > #define GEN6_GT_GFX_RC6p 0x13810C > #define GEN6_GT_GFX_RC6pp 0x138110 > +#define VLV_PCBR_ADDR_SHIFT 12 > > #define GEN6_PCODE_MAILBOX 0x138124 > #define GEN6_PCODE_READY (1<<31) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index f3c5bce..421a4cc 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev) > gen6_disable_rps_interrupts(dev); > } > > +static void cherryview_disable_rps(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + I915_WRITE(GEN6_RC_CONTROL, 0); > + > + if (dev_priv->vlv_pctx) { > + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); > + dev_priv->vlv_pctx = NULL; > + } > +} > + > static void valleyview_disable_rps(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv) > dev_priv->vlv_pctx->stolen->start); > } > > +static void cherryview_setup_pctx(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned long pctx_paddr; > + struct i915_gtt *gtt = &dev_priv->gtt; > + u32 pcbr; > + int pctx_size = 32*1024; > + > + pcbr = I915_READ(VLV_PCBR); > + if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { > + /* > + * From the Gunit register HAS: > + * The Gfx driver is expected to program this register and ensure > + * proper allocation within Gfx stolen memory. For example, this > + * register should be programmed such than the PCBR range does not > + * overlap with other relevant ranges. > + */ > + pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size); This area should be reserved. > + I915_WRITE(VLV_PCBR, pctx_paddr); > + } > +} > + > static void valleyview_setup_pctx(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev) > dev_priv->vlv_pctx = NULL; > } > > +static void cherryview_enable_rps(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_ring_buffer *ring; > + u32 gtfifodbg, rc6_mode = 0, pcbr; > + int i; > + > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > + > + if ((gtfifodbg = I915_READ(GTFIFODBG))) { > + DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n", > + gtfifodbg); > + I915_WRITE(GTFIFODBG, gtfifodbg); > + } > + > + cherryview_setup_pctx(dev); This should be called from intel_init_gt_powersave(). > + > + /* 1a & 1b: Get forcewake during program sequence. Although the driver > + * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ > + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > + > + /* 2a: Program RC6 thresholds.*/ > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16); > + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */ > + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */ > + > + for_each_ring(ring, dev_priv, i) > + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); > + > + I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */ > + > + /* allows RC6 residency counter to work */ > + I915_WRITE(VLV_COUNTER_CONTROL, > + _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH | > + VLV_MEDIA_RC6_COUNT_EN | > + VLV_RENDER_RC6_COUNT_EN)); > + > + /* Todo: If BIOS has not configured PCBR > + * then allocate in BIOS Reserved */ > + > + /* For now we assume BIOS is allocating and populating the PCBR */ > + pcbr = I915_READ(VLV_PCBR); > + > + DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr); > + > + /* 3: Enable RC6 */ > + if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) && > + (pcbr >> VLV_PCBR_ADDR_SHIFT)) This may break runtime PM, if PCBR isn't setup, since we assume that if intel_enable_rc6() returns a non-zero value RC6 will be truly enabled. So the check for PCBR should be moved to sanitize_rc6_option(). Also note that for CHV PCBR setup cannot fail atm. > + rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL; > + > + I915_WRITE(GEN6_RC_CONTROL, rc6_mode); > + > + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > +} > + > static void valleyview_enable_rps(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) > cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); > cancel_work_sync(&dev_priv->rps.work); > mutex_lock(&dev_priv->rps.hw_lock); > - if (IS_VALLEYVIEW(dev)) > + if (IS_CHERRYVIEW(dev)) > + cherryview_disable_rps(dev); > + else if (IS_VALLEYVIEW(dev)) > valleyview_disable_rps(dev); > else > gen6_disable_rps(dev); > @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work) > > mutex_lock(&dev_priv->rps.hw_lock); > > - if (IS_VALLEYVIEW(dev)) { > + if (IS_CHERRYVIEW(dev)) { > + cherryview_enable_rps(dev); > + } else if (IS_VALLEYVIEW(dev)) { > valleyview_enable_rps(dev); > } else if (IS_BROADWELL(dev)) { > gen8_enable_rps(dev); > @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev) > ironlake_enable_rc6(dev); > intel_init_emon(dev); > } else if (IS_GEN6(dev) || IS_GEN7(dev)) { > + if (IS_VALLEYVIEW(dev)) > + valleyview_setup_pctx(dev); Spurious hunk, as Ben pointed out, and the GEN check will be false for CherryView. The corresponding GEN check must be updated in intel_disable_gt_powersave() too. > /* > * PCU communication is slow and this doesn't need to be > * done at any specific time, so do this out of our fast path > @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev) > dev_priv->mem_freq = 1333; > break; > } > + > DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); > > dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv);
On Mon, Apr 28, 2014 at 05:29:46PM +0300, Imre Deak wrote: > > +static void cherryview_setup_pctx(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + unsigned long pctx_paddr; > > + struct i915_gtt *gtt = &dev_priv->gtt; > > + u32 pcbr; > > + int pctx_size = 32*1024; > > + > > + pcbr = I915_READ(VLV_PCBR); > > + if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { > > + /* > > + * From the Gunit register HAS: > > + * The Gfx driver is expected to program this register and ensure > > + * proper allocation within Gfx stolen memory. For example, this > > + * register should be programmed such than the PCBR range does not > > + * overlap with other relevant ranges. > > + */ > > + pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size); > > This area should be reserved. We've had a really lengthy discussion internally about the bios-reserved chunk in stolen. It was stalled due to (imo unjustified) fear to leak information what the bios actually uses this for. If we need to reserve more of stolen than we currently do we need to pick up that approach again instead of adding more bandaids. -Daniel
On Monday 28 April 2014 08:15 PM, Daniel Vetter wrote: > On Mon, Apr 28, 2014 at 05:29:46PM +0300, Imre Deak wrote: >>> +static void cherryview_setup_pctx(struct drm_device *dev) >>> +{ >>> + struct drm_i915_private *dev_priv = dev->dev_private; >>> + unsigned long pctx_paddr; >>> + struct i915_gtt *gtt = &dev_priv->gtt; >>> + u32 pcbr; >>> + int pctx_size = 32*1024; >>> + >>> + pcbr = I915_READ(VLV_PCBR); >>> + if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { >>> + /* >>> + * From the Gunit register HAS: >>> + * The Gfx driver is expected to program this register and ensure >>> + * proper allocation within Gfx stolen memory. For example, this >>> + * register should be programmed such than the PCBR range does not >>> + * overlap with other relevant ranges. >>> + */ >>> + pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size); >> This area should be reserved. > We've had a really lengthy discussion internally about the bios-reserved > chunk in stolen. It was stalled due to (imo unjustified) fear to leak > information what the bios actually uses this for. > > If we need to reserve more of stolen than we currently do we need to pick > up that approach again instead of adding more bandaids. > -Daniel Agreed. Will change accordingly.
Thanks for the review. I will address the comments On Monday 28 April 2014 07:59 PM, Imre Deak wrote: > On Mon, 2014-04-21 at 13:34 +0530, deepak.s@linux.intel.com wrote: >> From: Deepak S <deepak.s@linux.intel.com> >> >> v2: Configure PCBR if BIOS fails allocate pcbr (deepak) >> >> v3: Fix PCBR condition check during CHV RC6 Enable flag set >> >> Signed-off-by: Deepak S <deepak.s@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 99 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index b951d61..7090b42 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -5134,6 +5134,7 @@ enum punit_power_well { >> #define GEN6_GT_GFX_RC6 0x138108 >> #define GEN6_GT_GFX_RC6p 0x13810C >> #define GEN6_GT_GFX_RC6pp 0x138110 >> +#define VLV_PCBR_ADDR_SHIFT 12 >> >> #define GEN6_PCODE_MAILBOX 0x138124 >> #define GEN6_PCODE_READY (1<<31) >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index f3c5bce..421a4cc 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev) >> gen6_disable_rps_interrupts(dev); >> } >> >> +static void cherryview_disable_rps(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + >> + I915_WRITE(GEN6_RC_CONTROL, 0); >> + >> + if (dev_priv->vlv_pctx) { >> + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); >> + dev_priv->vlv_pctx = NULL; >> + } >> +} >> + >> static void valleyview_disable_rps(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv) >> dev_priv->vlv_pctx->stolen->start); >> } >> >> +static void cherryview_setup_pctx(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + unsigned long pctx_paddr; >> + struct i915_gtt *gtt = &dev_priv->gtt; >> + u32 pcbr; >> + int pctx_size = 32*1024; >> + >> + pcbr = I915_READ(VLV_PCBR); >> + if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { >> + /* >> + * From the Gunit register HAS: >> + * The Gfx driver is expected to program this register and ensure >> + * proper allocation within Gfx stolen memory. For example, this >> + * register should be programmed such than the PCBR range does not >> + * overlap with other relevant ranges. >> + */ >> + pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size); > This area should be reserved. > >> + I915_WRITE(VLV_PCBR, pctx_paddr); >> + } >> +} >> + >> static void valleyview_setup_pctx(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev) >> dev_priv->vlv_pctx = NULL; >> } >> >> +static void cherryview_enable_rps(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_ring_buffer *ring; >> + u32 gtfifodbg, rc6_mode = 0, pcbr; >> + int i; >> + >> + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); >> + >> + if ((gtfifodbg = I915_READ(GTFIFODBG))) { >> + DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n", >> + gtfifodbg); >> + I915_WRITE(GTFIFODBG, gtfifodbg); >> + } >> + >> + cherryview_setup_pctx(dev); > This should be called from intel_init_gt_powersave(). > >> + >> + /* 1a & 1b: Get forcewake during program sequence. Although the driver >> + * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ >> + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); >> + >> + /* 2a: Program RC6 thresholds.*/ >> + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16); >> + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */ >> + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */ >> + >> + for_each_ring(ring, dev_priv, i) >> + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); >> + >> + I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */ >> + >> + /* allows RC6 residency counter to work */ >> + I915_WRITE(VLV_COUNTER_CONTROL, >> + _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH | >> + VLV_MEDIA_RC6_COUNT_EN | >> + VLV_RENDER_RC6_COUNT_EN)); >> + >> + /* Todo: If BIOS has not configured PCBR >> + * then allocate in BIOS Reserved */ >> + >> + /* For now we assume BIOS is allocating and populating the PCBR */ >> + pcbr = I915_READ(VLV_PCBR); >> + >> + DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr); >> + >> + /* 3: Enable RC6 */ >> + if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) && >> + (pcbr >> VLV_PCBR_ADDR_SHIFT)) > This may break runtime PM, if PCBR isn't setup, since we assume that if > intel_enable_rc6() returns a non-zero value RC6 will be truly enabled. > So the check for PCBR should be moved to sanitize_rc6_option(). Also > note that for CHV PCBR setup cannot fail atm. > >> + rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL; >> + >> + I915_WRITE(GEN6_RC_CONTROL, rc6_mode); >> + >> + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); >> +} >> + >> static void valleyview_enable_rps(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) >> cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); >> cancel_work_sync(&dev_priv->rps.work); >> mutex_lock(&dev_priv->rps.hw_lock); >> - if (IS_VALLEYVIEW(dev)) >> + if (IS_CHERRYVIEW(dev)) >> + cherryview_disable_rps(dev); >> + else if (IS_VALLEYVIEW(dev)) >> valleyview_disable_rps(dev); >> else >> gen6_disable_rps(dev); >> @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work) >> >> mutex_lock(&dev_priv->rps.hw_lock); >> >> - if (IS_VALLEYVIEW(dev)) { >> + if (IS_CHERRYVIEW(dev)) { >> + cherryview_enable_rps(dev); >> + } else if (IS_VALLEYVIEW(dev)) { >> valleyview_enable_rps(dev); >> } else if (IS_BROADWELL(dev)) { >> gen8_enable_rps(dev); >> @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev) >> ironlake_enable_rc6(dev); >> intel_init_emon(dev); >> } else if (IS_GEN6(dev) || IS_GEN7(dev)) { >> + if (IS_VALLEYVIEW(dev)) >> + valleyview_setup_pctx(dev); > Spurious hunk, as Ben pointed out, and the GEN check will be false for > CherryView. The corresponding GEN check must be updated in > intel_disable_gt_powersave() too. > >> /* >> * PCU communication is slow and this doesn't need to be >> * done at any specific time, so do this out of our fast path >> @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev) >> dev_priv->mem_freq = 1333; >> break; >> } >> + >> DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); >> >> dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv);
Thanks for the review. I will address the comments On Saturday 26 April 2014 03:12 AM, Ben Widawsky wrote: > On Mon, Apr 21, 2014 at 01:34:07PM +0530, deepak.s@linux.intel.com wrote: >> From: Deepak S <deepak.s@linux.intel.com> >> >> v2: Configure PCBR if BIOS fails allocate pcbr (deepak) >> >> v3: Fix PCBR condition check during CHV RC6 Enable flag set >> >> Signed-off-by: Deepak S <deepak.s@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 1 + >> drivers/gpu/drm/i915/intel_pm.c | 100 +++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 99 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index b951d61..7090b42 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -5134,6 +5134,7 @@ enum punit_power_well { >> #define GEN6_GT_GFX_RC6 0x138108 >> #define GEN6_GT_GFX_RC6p 0x13810C >> #define GEN6_GT_GFX_RC6pp 0x138110 >> +#define VLV_PCBR_ADDR_SHIFT 12 >> >> #define GEN6_PCODE_MAILBOX 0x138124 >> #define GEN6_PCODE_READY (1<<31) >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index f3c5bce..421a4cc 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev) >> gen6_disable_rps_interrupts(dev); >> } >> >> +static void cherryview_disable_rps(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + >> + I915_WRITE(GEN6_RC_CONTROL, 0); >> + >> + if (dev_priv->vlv_pctx) { >> + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); >> + dev_priv->vlv_pctx = NULL; >> + } >> +} >> + >> static void valleyview_disable_rps(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv) >> dev_priv->vlv_pctx->stolen->start); >> } >> >> +static void cherryview_setup_pctx(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + unsigned long pctx_paddr; >> + struct i915_gtt *gtt = &dev_priv->gtt; >> + u32 pcbr; >> + int pctx_size = 32*1024; >> + >> + pcbr = I915_READ(VLV_PCBR); >> + if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { >> + /* >> + * From the Gunit register HAS: >> + * The Gfx driver is expected to program this register and ensure >> + * proper allocation within Gfx stolen memory. For example, this >> + * register should be programmed such than the PCBR range does not >> + * overlap with other relevant ranges. >> + */ >> + pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size); >> + I915_WRITE(VLV_PCBR, pctx_paddr); >> + } >> +} >> + > Is there a reason we did not follow the same idioms as Valleyview? > Shouldn't we be building a stolen object like we do there, and then > using that? > > Furthermore, we need to make sure we make the stolen allocator aware for > the case where pcbr is not zero, like we do for valleyview. > > I think the best solution here is to try to combine the valleyview and > cherryview logic for this function. Extract out size, and most of the > rest looks pretty similar. > > For enabling, I am fine with it as is though provided it's hidden by > preliminary flag. > >> static void valleyview_setup_pctx(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev) >> dev_priv->vlv_pctx = NULL; >> } >> >> +static void cherryview_enable_rps(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_ring_buffer *ring; >> + u32 gtfifodbg, rc6_mode = 0, pcbr; >> + int i; >> + >> + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); >> + >> + if ((gtfifodbg = I915_READ(GTFIFODBG))) { >> + DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n", >> + gtfifodbg); >> + I915_WRITE(GTFIFODBG, gtfifodbg); >> + } >> + >> + cherryview_setup_pctx(dev); >> + >> + /* 1a & 1b: Get forcewake during program sequence. Although the driver >> + * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ >> + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); >> + >> + /* 2a: Program RC6 thresholds.*/ >> + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16); >> + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */ >> + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */ >> + >> + for_each_ring(ring, dev_priv, i) >> + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); >> + >> + I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */ >> + >> + /* allows RC6 residency counter to work */ >> + I915_WRITE(VLV_COUNTER_CONTROL, >> + _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH | >> + VLV_MEDIA_RC6_COUNT_EN | >> + VLV_RENDER_RC6_COUNT_EN)); >> + >> + /* Todo: If BIOS has not configured PCBR >> + * then allocate in BIOS Reserved */ >> + >> + /* For now we assume BIOS is allocating and populating the PCBR */ >> + pcbr = I915_READ(VLV_PCBR); >> + >> + DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr); >> + >> + /* 3: Enable RC6 */ >> + if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) && >> + (pcbr >> VLV_PCBR_ADDR_SHIFT)) >> + rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL; >> + >> + I915_WRITE(GEN6_RC_CONTROL, rc6_mode); >> + >> + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); >> +} >> + > Can you send me a link to the doc for this off-list? I am having trouble > tracking it down. > >> static void valleyview_enable_rps(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) >> cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); >> cancel_work_sync(&dev_priv->rps.work); >> mutex_lock(&dev_priv->rps.hw_lock); >> - if (IS_VALLEYVIEW(dev)) >> + if (IS_CHERRYVIEW(dev)) >> + cherryview_disable_rps(dev); >> + else if (IS_VALLEYVIEW(dev)) >> valleyview_disable_rps(dev); >> else >> gen6_disable_rps(dev); >> @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work) >> >> mutex_lock(&dev_priv->rps.hw_lock); >> >> - if (IS_VALLEYVIEW(dev)) { >> + if (IS_CHERRYVIEW(dev)) { >> + cherryview_enable_rps(dev); >> + } else if (IS_VALLEYVIEW(dev)) { >> valleyview_enable_rps(dev); >> } else if (IS_BROADWELL(dev)) { >> gen8_enable_rps(dev); >> @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev) >> ironlake_enable_rc6(dev); >> intel_init_emon(dev); >> } else if (IS_GEN6(dev) || IS_GEN7(dev)) { >> + if (IS_VALLEYVIEW(dev)) >> + valleyview_setup_pctx(dev); > Spurious hunk? > >> /* >> * PCU communication is slow and this doesn't need to be >> * done at any specific time, so do this out of our fast path >> @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev) >> dev_priv->mem_freq = 1333; >> break; >> } >> + > Spurious hunk? > >> DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); >> >> dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv); > I think for enabling this patch is fine as is. It does need some of the > fixes I mentioned about. So I'd suggest adding FIXMes with what I > request, and calling this > Acked-by: Ben Widawsky <ben@bwidawsk.net> > > To move it up to a reviewed by, I need to find the PM doc (which I > probably have, just don't know it), and I'd like the setup function > fixed. > > >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b951d61..7090b42 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5134,6 +5134,7 @@ enum punit_power_well { #define GEN6_GT_GFX_RC6 0x138108 #define GEN6_GT_GFX_RC6p 0x13810C #define GEN6_GT_GFX_RC6pp 0x138110 +#define VLV_PCBR_ADDR_SHIFT 12 #define GEN6_PCODE_MAILBOX 0x138124 #define GEN6_PCODE_READY (1<<31) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index f3c5bce..421a4cc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3264,6 +3264,18 @@ static void gen6_disable_rps(struct drm_device *dev) gen6_disable_rps_interrupts(dev); } +static void cherryview_disable_rps(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + I915_WRITE(GEN6_RC_CONTROL, 0); + + if (dev_priv->vlv_pctx) { + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); + dev_priv->vlv_pctx = NULL; + } +} + static void valleyview_disable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -3642,6 +3654,28 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv) dev_priv->vlv_pctx->stolen->start); } +static void cherryview_setup_pctx(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + unsigned long pctx_paddr; + struct i915_gtt *gtt = &dev_priv->gtt; + u32 pcbr; + int pctx_size = 32*1024; + + pcbr = I915_READ(VLV_PCBR); + if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { + /* + * From the Gunit register HAS: + * The Gfx driver is expected to program this register and ensure + * proper allocation within Gfx stolen memory. For example, this + * register should be programmed such than the PCBR range does not + * overlap with other relevant ranges. + */ + pctx_paddr = (dev_priv->mm.stolen_base + gtt->stolen_size - pctx_size); + I915_WRITE(VLV_PCBR, pctx_paddr); + } +} + static void valleyview_setup_pctx(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -3697,6 +3731,61 @@ static void valleyview_cleanup_pctx(struct drm_device *dev) dev_priv->vlv_pctx = NULL; } +static void cherryview_enable_rps(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_ring_buffer *ring; + u32 gtfifodbg, rc6_mode = 0, pcbr; + int i; + + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); + + if ((gtfifodbg = I915_READ(GTFIFODBG))) { + DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n", + gtfifodbg); + I915_WRITE(GTFIFODBG, gtfifodbg); + } + + cherryview_setup_pctx(dev); + + /* 1a & 1b: Get forcewake during program sequence. Although the driver + * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); + + /* 2a: Program RC6 thresholds.*/ + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16); + I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */ + I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */ + + for_each_ring(ring, dev_priv, i) + I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10); + + I915_WRITE(GEN6_RC6_THRESHOLD, 50000); /* 50/125ms per EI */ + + /* allows RC6 residency counter to work */ + I915_WRITE(VLV_COUNTER_CONTROL, + _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH | + VLV_MEDIA_RC6_COUNT_EN | + VLV_RENDER_RC6_COUNT_EN)); + + /* Todo: If BIOS has not configured PCBR + * then allocate in BIOS Reserved */ + + /* For now we assume BIOS is allocating and populating the PCBR */ + pcbr = I915_READ(VLV_PCBR); + + DRM_DEBUG_DRIVER("PCBR offset : 0x%x\n", pcbr); + + /* 3: Enable RC6 */ + if ((intel_enable_rc6(dev) & INTEL_RC6_ENABLE) && + (pcbr >> VLV_PCBR_ADDR_SHIFT)) + rc6_mode = GEN6_RC_CTL_EI_MODE(1) | VLV_RC_CTL_CTX_RST_PARALLEL; + + I915_WRITE(GEN6_RC_CONTROL, rc6_mode); + + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); +} + static void valleyview_enable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -4550,7 +4639,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work); cancel_work_sync(&dev_priv->rps.work); mutex_lock(&dev_priv->rps.hw_lock); - if (IS_VALLEYVIEW(dev)) + if (IS_CHERRYVIEW(dev)) + cherryview_disable_rps(dev); + else if (IS_VALLEYVIEW(dev)) valleyview_disable_rps(dev); else gen6_disable_rps(dev); @@ -4568,7 +4659,9 @@ static void intel_gen6_powersave_work(struct work_struct *work) mutex_lock(&dev_priv->rps.hw_lock); - if (IS_VALLEYVIEW(dev)) { + if (IS_CHERRYVIEW(dev)) { + cherryview_enable_rps(dev); + } else if (IS_VALLEYVIEW(dev)) { valleyview_enable_rps(dev); } else if (IS_BROADWELL(dev)) { gen8_enable_rps(dev); @@ -4590,6 +4683,8 @@ void intel_enable_gt_powersave(struct drm_device *dev) ironlake_enable_rc6(dev); intel_init_emon(dev); } else if (IS_GEN6(dev) || IS_GEN7(dev)) { + if (IS_VALLEYVIEW(dev)) + valleyview_setup_pctx(dev); /* * PCU communication is slow and this doesn't need to be * done at any specific time, so do this out of our fast path @@ -5175,6 +5270,7 @@ static void valleyview_init_clock_gating(struct drm_device *dev) dev_priv->mem_freq = 1333; break; } + DRM_DEBUG_DRIVER("DDR speed: %d MHz", dev_priv->mem_freq); dev_priv->vlv_cdclk_freq = valleyview_cur_cdclk(dev_priv);