From mboxrd@z Thu Jan 1 00:00:00 1970 From: "S, Deepak" Subject: Re: [PATCH v3 2/3] drm/i915/vlv: WA for Turbo and RC6 to work together. Date: Fri, 14 Mar 2014 00:10:45 +0530 Message-ID: <5321FBAD.9010103@intel.com> References: <531718EF.3060201@intel.com> <1394726418-10831-1-git-send-email-deepak.s@linux.intel.com> <1394726418-10831-3-git-send-email-deepak.s@linux.intel.com> <20140313181730.GQ20292@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (unknown [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id E812DFB36E for ; Thu, 13 Mar 2014 11:40:50 -0700 (PDT) In-Reply-To: <20140313181730.GQ20292@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= , deepak.s@linux.intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 3/13/2014 11:47 PM, Ville Syrj=E4l=E4 wrote: > On Thu, Mar 13, 2014 at 09:30:17PM +0530, deepak.s@linux.intel.com wrote: >> From: Deepak S >> >> 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 >> --- >> 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(-) >> > >> @@ -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 |=3D 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_de= vice *dev) >> pm_irqs |=3D PM_VEBOX_USER_INTERRUPT; >> >> dev_priv->pm_irq_mask =3D 0xffffffff; >> + >> + dev_priv->pm_irq_mask &=3D ~dev_priv->pm_rps_events; >> + pm_irqs |=3D 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/inte= l_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_pri= vate *dev_priv) >> I915_READ(VLV_GTLC_SURVIVABILITY_REG) & >> ~VLV_GFX_CLK_FORCE_ON_BIT); >> >> - /* Unmask Up interrupts */ >> - dev_priv->rps.rp_up_masked =3D 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 =3D 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_devic= e *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_devi= ce *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 =3D GEN7_RC_CTL_TO_MODE | VLV_RC_CTL_CTX_RST_PARALLEL; >> >> @@ -3691,7 +3693,8 @@ static void valleyview_enable_rps(struct drm_devic= e *dev) >> dev_priv->rps.rp_up_masked =3D false; >> dev_priv->rps.rp_down_masked =3D false; >> >> - dev_priv->pm_rps_events =3D GEN6_PM_RPS_EVENTS; >> + /* WAUseRC0ResidenncyTurbo:VLV */ >> + dev_priv->pm_rps_events =3D 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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >