[Intel-gfx] [PATCH 3/4] drm/i915: New offset for reading frequencies on CHV.
Jani Nikula
jani.nikula at linux.intel.com
Sun Dec 14 22:51:30 PST 2014
On Fri, 12 Dec 2014, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Fri, Dec 12, 2014 at 02:18:15PM +0530, deepak.s at linux.intel.com wrote:
>> From: Deepak S <deepak.s at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list