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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 12 Dec 2014, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Dec 12, 2014 at 02:18:15PM +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.
>> 
>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       |  4 +--
>>  drivers/gpu/drm/i915/i915_reg.h       | 12 ++++++++
>>  drivers/gpu/drm/i915/intel_pm.c       | 52 ++++++++++++++++++++++++++++++-----
>>  drivers/gpu/drm/i915/intel_sideband.c |  4 +--
>>  4 files changed, 61 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index b58bad4..0690dff 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3016,8 +3016,8 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
>>  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
>>  
>>  /* intel_sideband.c */
>> -u32 vlv_punit_read(struct drm_i915_private *dev_priv, u8 addr);
>> -void vlv_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val);
>> +u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr);
>> +void vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val);
>>  u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr);
>>  u32 vlv_gpio_nc_read(struct drm_i915_private *dev_priv, u32 reg);
>>  void vlv_gpio_nc_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index b57cba3..f41290c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -602,6 +602,18 @@ 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_FMAX_AT_VMAX_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
>> +
>
> This blank line makes me think FB_GFX_GUAR_FREQ_FUSE_MASK isn't part of
> this register. So best not leave such blank line here.
>
> I have 0x3c3c3c28 in this register, which matches what I get using the
> old method.
>
>> +#define FB_GFX_GUAR_FREQ_FUSE_MASK		0xff
>> +
>> +#define FB_GFX_FMIN_AT_VMIN_FUSE		0x137
>> +#define FB_GFX_FMIN_AT_VMIN_FUSE_MASK		0xff
>> +#define FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT		8
>
> I have 0x69841428 here. The low 8 bits look like another freq value.
> What is it?
>
> I have no docs for this stuff so can't really review apart from looking
> at what my hardware reports.
>
>
> Actually, since all the values are 8 bits maybe it would be neater to
> just
> #define FB_GFX_FREQ_FUSE_MASK 0xff
> and use that everywhere instead of having three different definitions
> for the same 0xff value.
>
>> +
>>  #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 2acb3de..71b8e2f 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4346,11 +4346,29 @@ 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;
>> +	struct intel_device_info *info;
>>  	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;
>> -
>> +	info = (struct intel_device_info *)&dev_priv->info;
>
> Pointless cast. Also the assignment could be done when declaring info,
> and we usually use INTEL_INFO() to get at it.
>
>> +
>> +	if (dev->pdev->revision >= 0x20) {
>
> Do we really need this check? I would think it would be up to the
> Punit firmware version rather the stepping. My BSW has PCI rev 0x15,
> but with the latest BIOS both fuse registers already contain correct
> looking information.
>
>> +		val = vlv_punit_read(dev_priv, FB_GFX_FMAX_AT_VMAX_FUSE);
>> +
>> +		if (info->eu_total == 8) /* (2 * 4) config */
>> +			rp0 = (val >> FB_GFX_FMAX_AT_VMAX_2SS4EU_FUSE_SHIFT);
>> +		else if (info->eu_total == 12) /* (2 * 6) config */
>> +			rp0 = (val >> FB_GFX_FMAX_AT_VMAX_2SS6EU_FUSE_SHIFT);
>> +		else if (info->eu_total == 16) /* (2 * 8) config */
>> +			rp0 = (val >> FB_GFX_FMAX_AT_VMAX_2SS8EU_FUSE_SHIFT);
>> +		else /* Setting (2 * 8) Min RP0 for any other combination */
>> +			rp0 = (val >> FB_GFX_FMAX_AT_VMAX_2SS8EU_FUSE_SHIFT);
>
> 'switch (INTEL_INFO(dev)->eu_total)' perhaps?
>
>> +		rp0 = (rp0 & FB_GFX_FMAX_AT_VMAX_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;
>>  }
>>  
>> @@ -4366,20 +4384,40 @@ 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;
>> +	struct intel_device_info *info;
>>  	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;
>> +	info = (struct intel_device_info *)&dev_priv->info;
>
> Unused here.
>
>>  
>> +	if (dev->pdev->revision >= 0x20) {
>
>> +		val = vlv_punit_read(dev_priv, FB_GFX_FMAX_AT_VMAX_FUSE);
>> +		rp1 = (val & FB_GFX_GUAR_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;
>> +	struct intel_device_info *info;
>>  	u32 val, rpn;
>> +	info = (struct intel_device_info *)&dev_priv->info;
>
> Unused.
>
>> +
>> +	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_FMIN_AT_VMIN_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);
>> +	}
>>  
>> -	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;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
>> index 01d841e..3c42eef 100644
>> --- a/drivers/gpu/drm/i915/intel_sideband.c
>> +++ b/drivers/gpu/drm/i915/intel_sideband.c
>> @@ -75,7 +75,7 @@ static int vlv_sideband_rw(struct drm_i915_private *dev_priv, u32 devfn,
>>  	return 0;
>>  }
>>  
>> -u32 vlv_punit_read(struct drm_i915_private *dev_priv, u8 addr)
>> +u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
>
> Good thing you had to pass a constant. Otherwise this could have caused
> quite a bit of head scratching for you ;)

This is the kind of thing that really should be a separate fix. If, for
any reason, we need to revert the rest, we'll still want this.

BR,
Jani.

>
>>  {
>>  	u32 val = 0;
>>  
>> @@ -89,7 +89,7 @@ u32 vlv_punit_read(struct drm_i915_private *dev_priv, u8 addr)
>>  	return val;
>>  }
>>  
>> -void vlv_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val)
>> +void vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val)
>>  {
>>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>>  
>> -- 
>> 1.9.1
>
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx