[Intel-gfx] [PATCH v3 2/3] drm/i915/vlv: WA for Turbo and RC6 to work together.
S, Deepak
deepak.s at intel.com
Thu Mar 13 19:40:45 CET 2014
On 3/13/2014 11:47 PM, Ville Syrjälä wrote:
> On Thu, Mar 13, 2014 at 09:30:17PM +0530, deepak.s at linux.intel.com wrote:
>> From: Deepak S <deepak.s at intel.com>
>>
>> With RC6 enabled, BYT has an HW issue in determining the right
>> Gfx busyness.
>> WA for Turbo + RC6: Use SW based Gfx busy-ness detection to decide
>> on increasing/decreasing the freq. This logic will monitor C0
>> counters of render/media power-wells over EI period and takes
>> necessary action based on these values
>>
>> v2: Refactor duplicate code. (Ville)
>>
>> v3: Reformat the comments. (Ville)
>>
>> Signed-off-by: Deepak S <deepak.s at linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 17 +++++
>> drivers/gpu/drm/i915/i915_irq.c | 133 ++++++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/i915_reg.h | 13 +++-
>> drivers/gpu/drm/i915/intel_pm.c | 19 +++---
>> 4 files changed, 173 insertions(+), 9 deletions(-)
>>
> <snip>
>> @@ -1564,6 +1683,16 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>> queue_work(dev_priv->wq, &dev_priv->rps.work);
>> }
>>
>> + if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) {
>> + spin_lock(&dev_priv->irq_lock);
>> + dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RP_UP_EI_EXPIRED;
>> + snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RP_UP_EI_EXPIRED);
>> + spin_unlock(&dev_priv->irq_lock);
>> + DRM_DEBUG_DRIVER("\nQueueing RPS Work - RC6 WA Turbo");
>
> This debug message still seems rather pointless. Just drop it.
>
> Oh actually isn't this entire block of code useless now that
> pm_rps_events is used? The previous if block already checked
> pm_rps_events which will include GEN6_PM_RP_UP_EI_EXPIRED on
> VLV, so this code here will just repeat the work already done.
hmmm. I think i missed this in internal review. with pm_rps_events i
think this is redundant.
>> +
>> + queue_work(dev_priv->wq, &dev_priv->rps.work);
>> + }
>> +
>> if (HAS_VEBOX(dev_priv->dev)) {
>> if (pm_iir & PM_VEBOX_USER_INTERRUPT)
>> notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);
>> @@ -2989,6 +3118,10 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
>> pm_irqs |= PM_VEBOX_USER_INTERRUPT;
>>
>> dev_priv->pm_irq_mask = 0xffffffff;
>> +
>> + dev_priv->pm_irq_mask &= ~dev_priv->pm_rps_events;
>> + pm_irqs |= dev_priv->pm_rps_events;
>
> What's this stuff doing here?
>> +
>> I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
>> I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask);
>> I915_WRITE(GEN6_PMIER, pm_irqs);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 6174fda..d978b46 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -419,6 +419,7 @@ enum punit_power_well {
>> #define PUNIT_REG_GPU_FREQ_STS 0xd8
>> #define GENFREQSTATUS (1<<0)
>> #define PUNIT_REG_MEDIA_TURBO_FREQ_REQ 0xdc
>> +#define PUNIT_REG_CZ_TIMESTAMP 0xce
>>
>> #define PUNIT_FUSE_BUS2 0xf6 /* bits 47:40 */
>> #define PUNIT_FUSE_BUS1 0xf5 /* bits 55:48 */
>> @@ -434,6 +435,11 @@ enum punit_power_well {
>> #define FB_FMAX_VMIN_FREQ_LO_SHIFT 27
>> #define FB_FMAX_VMIN_FREQ_LO_MASK 0xf8000000
>>
>> +#define VLV_CZ_CLOCK_TO_MILLI_SEC 100000
>> +#define VLV_RP_UP_EI_THRESHOLD 90
>> +#define VLV_RP_DOWN_EI_THRESHOLD 70
>> +#define VLV_INT_COUNT_FOR_DOWN_EI 5
>> +
>> /* vlv2 north clock has */
>> #define CCK_FUSE_REG 0x8
>> #define CCK_FUSE_HPLL_FREQ_MASK 0x3
>> @@ -4892,6 +4898,7 @@ enum punit_power_well {
>> #define VLV_GTLC_PW_STATUS 0x130094
>> #define VLV_GTLC_PW_RENDER_STATUS_MASK 0x80
>> #define VLV_GTLC_PW_MEDIA_STATUS_MASK 0x20
>> +#define VLV_GTLC_SURVIVABILITY_REG 0x130098
>> #define FORCEWAKE_MT 0xa188 /* multi-threaded */
>> #define FORCEWAKE_KERNEL 0x1
>> #define FORCEWAKE_USER 0x2
>> @@ -5019,13 +5026,17 @@ enum punit_power_well {
>>
>> #define GEN6_GT_GFX_RC6_LOCKED 0x138104
>> #define VLV_COUNTER_CONTROL 0x138104
>> +#define VLV_RC_COUNTER_CONTROL 0xFFFF00FF
>
> I'd still like to see names for all the bits we frob, and I'd
> still like to have some kind of an answer to the question whether
> we really need to enable them all when the w/a is only interested
> in the rc0 counters.
I did try with enabling only the rc0 counters, but the busyness
calculation was not right. Let me do some more investigation and get
back to you on this.
>> #define VLV_COUNT_RANGE_HIGH (1<<15)
>> +#define VLV_MEDIA_RC0_COUNT_EN (1<<5)
>> +#define VLV_RENDER_RC0_COUNT_EN (1<<4)
>> #define VLV_MEDIA_RC6_COUNT_EN (1<<1)
>> #define VLV_RENDER_RC6_COUNT_EN (1<<0)
>> #define GEN6_GT_GFX_RC6 0x138108
>> #define GEN6_GT_GFX_RC6p 0x13810C
>> #define GEN6_GT_GFX_RC6pp 0x138110
>> -
>> +#define VLV_RENDER_C0_COUNT_REG 0x138118
>> +#define VLV_MEDIA_C0_COUNT_REG 0x13811C
>> #define GEN6_PCODE_MAILBOX 0x138124
>> #define GEN6_PCODE_READY (1<<31)
>> #define GEN6_READ_OC_PARAMS 0xc
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index bf6baa6..8a791b7 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3096,10 +3096,14 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>> I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
>> ~VLV_GFX_CLK_FORCE_ON_BIT);
>>
>> - /* Unmask Up interrupts */
>> - dev_priv->rps.rp_up_masked = true;
>> - gen6_set_pm_mask(dev_priv, GEN6_PM_RP_DOWN_THRESHOLD,
>> + /* Unmask Turbo interrupts */
>> + if (dev_priv->pm_rps_events & GEN6_PM_RP_UP_EI_EXPIRED)
>> + I915_WRITE(GEN6_PMINTRMSK, ~dev_priv->pm_rps_events);
>> + else {
>> + dev_priv->rps.rp_up_masked = true;
>> + gen6_set_pm_mask(dev_priv, GEN6_PM_RP_DOWN_THRESHOLD,
>> dev_priv->rps.min_delay);
>> + }
>> }
>>
>> void gen6_rps_idle(struct drm_i915_private *dev_priv)
>> @@ -3620,6 +3624,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
>> I915_WRITE(GEN6_RP_DOWN_EI, 350000);
>>
>> I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
>> + I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 0xf4240);
>>
>> I915_WRITE(GEN6_RP_CONTROL,
>> GEN6_RP_MEDIA_TURBO |
>> @@ -3639,10 +3644,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
>> I915_WRITE(GEN6_RC6_THRESHOLD, 0x557);
>>
>> /* 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));
>> + I915_WRITE(VLV_COUNTER_CONTROL, VLV_RC_COUNTER_CONTROL);
>> if (intel_enable_rc6(dev) & INTEL_RC6_ENABLE)
>> rc6_mode = GEN7_RC_CTL_TO_MODE | VLV_RC_CTL_CTX_RST_PARALLEL;
>>
>> @@ -3691,7 +3693,8 @@ static void valleyview_enable_rps(struct drm_device *dev)
>> dev_priv->rps.rp_up_masked = false;
>> dev_priv->rps.rp_down_masked = false;
>>
>> - dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
>> + /* WAUseRC0ResidenncyTurbo:VLV */
>> + dev_priv->pm_rps_events = GEN6_PM_RP_UP_EI_EXPIRED;
>
> I'm still wondering if we should have the option of using the old
> fashioned method...
I think we can have a if turbo_wa, This will help us to switch by
disabling a flag.
>> gen6_enable_rps_interrupts(dev);
>>
>> gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>> --
>> 1.8.4.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list