Re: [PATCH v2 3/3] drm/i915: New offset for reading frequencies on CHV.

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

On Friday 16 January 2015 10:39 PM, Ville Syrjälä wrote:
> On Fri, Jan 16, 2015 at 08:42:18PM +0530, deepak.s@linux.intel.com wrote:
>> From: Deepak S <deepak.s@linux.intel.com>
>>
>> Use new Sideband offset to read max/min/gaur freq based on the SKU it
>> is running on. Based on the Number of EU, we read different bits to
>> identify the max frequencies at which system can run.
>>
>> v2: reuse mask definitions & INTEL_INFO() to get device info (Ville)
>>
>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h |  9 +++++++
>>   drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++-------
>>   2 files changed, 54 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index d9692f9..2dcb1b3 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -605,6 +605,15 @@ enum punit_power_well {
>>   #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
>>   #define PUNIT_FUSE_BUS1				0xf5 /* bits 55:48 */
>>   
>> +#define FB_GFX_FMAX_AT_VMAX_FUSE		0x136
>> +#define FB_GFX_FREQ_FUSE_MASK			0xff
>> +#define FB_GFX_FMAX_AT_VMAX_2SS4EU_FUSE_SHIFT	24
>> +#define FB_GFX_FMAX_AT_VMAX_2SS6EU_FUSE_SHIFT	16
>> +#define FB_GFX_FMAX_AT_VMAX_2SS8EU_FUSE_SHIFT	8
>> +
>> +#define FB_GFX_FMIN_AT_VMIN_FUSE		0x137
>> +#define FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT		8
>> +
>>   #define PUNIT_GPU_STATUS_REG			0xdb
>>   #define PUNIT_GPU_STATUS_MAX_FREQ_SHIFT	16
>>   #define PUNIT_GPU_STATUS_MAX_FREQ_MASK		0xff
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 03fc7f2..c010d5c 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4338,11 +4338,32 @@ void gen6_update_ring_freq(struct drm_device *dev)
>>   
>>   static int cherryview_rps_max_freq(struct drm_i915_private *dev_priv)
>>   {
>> +	struct drm_device *dev = dev_priv->dev;
>>   	u32 val, rp0;
>>   
>> -	val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
>> -	rp0 = (val >> PUNIT_GPU_STATUS_MAX_FREQ_SHIFT) & PUNIT_GPU_STATUS_MAX_FREQ_MASK;
>> -
>> +	if (dev->pdev->revision >= 0x20) {
>> +		val = vlv_punit_read(dev_priv, FB_GFX_FMAX_AT_VMAX_FUSE);
>> +
>> +		switch (INTEL_INFO(dev)->eu_total) {
>> +		case 8:
>> +				/* (2 * 4) config */
>> +				rp0 = (val >> FB_GFX_FMAX_AT_VMAX_2SS4EU_FUSE_SHIFT);
> break;
>
>> +		case 12:
>> +				/* (2 * 6) config */
>> +				rp0 = (val >> FB_GFX_FMAX_AT_VMAX_2SS6EU_FUSE_SHIFT);
> break;
>
>> +		case 16:
>> +				/* (2 * 8) config */
>> +		default:
>> +				/* Setting (2 * 8) Min RP0 for any other combination */
>> +				rp0 = (val >> FB_GFX_FMAX_AT_VMAX_2SS8EU_FUSE_SHIFT);
> Maybe break; here too if you feel like it :)

:) Thanks for the review


> Hmm. Now that I started to think about it, might we be expecting EUs to
> be fused off in some other configurations? In that case the switch
> statement might be not be the best idea, or we'd need to use the gnu
> case range extension. But we can maybe worry about that later since it
> might require more investigative work, or I might be totally off base here
> anyway, and we should get this patch in ASAP.
>
> So if you add the missing break statements this patch can have:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>> +		}
>> +		rp0 = (rp0 & FB_GFX_FREQ_FUSE_MASK);
>> +	} else {
>> +		/* For pre-production hardware */
>> +		val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
>> +		rp0 = (val >> PUNIT_GPU_STATUS_MAX_FREQ_SHIFT) &
>> +		       PUNIT_GPU_STATUS_MAX_FREQ_MASK;
>> +	}
>>   	return rp0;
>>   }
>>   
>> @@ -4358,20 +4379,36 @@ static int cherryview_rps_rpe_freq(struct drm_i915_private *dev_priv)
>>   
>>   static int cherryview_rps_guar_freq(struct drm_i915_private *dev_priv)
>>   {
>> +	struct drm_device *dev = dev_priv->dev;
>>   	u32 val, rp1;
>>   
>> -	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
>> -	rp1 = (val >> PUNIT_GPU_STATUS_MAX_FREQ_SHIFT) & PUNIT_GPU_STATUS_MAX_FREQ_MASK;
>> -
>> +	if (dev->pdev->revision >= 0x20) {
>> +		val = vlv_punit_read(dev_priv, FB_GFX_FMAX_AT_VMAX_FUSE);
>> +		rp1 = (val & FB_GFX_FREQ_FUSE_MASK);
>> +	} else {
>> +		/* For pre-production hardware */
>> +		val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
>> +		rp1 = ((val >> PUNIT_GPU_STATUS_MAX_FREQ_SHIFT) &
>> +		       PUNIT_GPU_STATUS_MAX_FREQ_MASK);
>> +	}
>>   	return rp1;
>>   }
>>   
>>   static int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
>>   {
>> +	struct drm_device *dev = dev_priv->dev;
>>   	u32 val, rpn;
>>   
>> -	val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
>> -	rpn = (val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) & PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK;
>> +	if (dev->pdev->revision >= 0x20) {
>> +		val = vlv_punit_read(dev_priv, FB_GFX_FMIN_AT_VMIN_FUSE);
>> +		rpn = ((val >> FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT) &
>> +		       FB_GFX_FREQ_FUSE_MASK);
>> +	} else { /* For pre-production hardware */
>> +		val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
>> +		rpn = ((val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) &
>> +		       PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK);
>> +	}
>> +
>>   	return rpn;
>>   }
>>   
>> -- 
>> 1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx