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

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

On Monday 15 December 2014 12:21 PM, Jani Nikula wrote:
> 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.

Agreed, Better to have a separate patch :)

>>>   {
>>>   	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

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