Re: [PATCH v3 2/3] drm/i915/vlv: WA for Turbo and RC6 to work together.

["S, Deepak" <deepak.s@intel.com>]

On 3/13/2014 11:47 PM, Ville Syrjälä wrote:
> On Thu, Mar 13, 2014 at 09:30:17PM +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@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@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>