[Intel-gfx] [PATCH 3/4] drm/i915: New offset for reading frequencies on CHV.
Deepak S
deepak.s at linux.intel.com
Tue Dec 16 04:11:54 PST 2014
On Monday 15 December 2014 12:21 PM, Jani Nikula wrote:
> 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.
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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list