Deepak S <deepak.s@xxxxxxxxxxxxxxx> writes:
> On Monday 26 May 2014 07:00 PM, Mika Kuoppala wrote:
>> Hi Deepak,
>>
>> deepak.s@xxxxxxxxxxxxxxx writes:
>>
>>> From: Deepak S <deepak.s@xxxxxxxxxxxxxxx>
>>>
>>> v2: Disable media turbo and Add DOWN_IDLE_AVG support (Ville)
>>>
>>> v3: Mass rename of the dev_priv->rps variables in upstream.
>>>
>>> v4: Rebase against latest code. (Deepak)
>>>
>>> v5: Rebase against latest nightly code. (Deepak)
>>>
>>> Signed-off-by: Deepak S <deepak.s@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>>> drivers/gpu/drm/i915/i915_reg.h | 10 ++++
>>> drivers/gpu/drm/i915/intel_pm.c | 95 ++++++++++++++++++++++++++++++++++-
>>> drivers/gpu/drm/i915/intel_sideband.c | 14 ++++++
>>> 4 files changed, 119 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 0412b12..5f0e338 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2621,6 +2621,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
>>> 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_nc_read(struct drm_i915_private *dev_priv, u8 addr);
>>> +u32 chv_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);
>>> u32 vlv_cck_read(struct drm_i915_private *dev_priv, u32 reg);
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index c1f36a5..37f4b12 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -487,6 +487,7 @@
>>> #define VLV_IOSF_DATA (VLV_DISPLAY_BASE + 0x2104)
>>> #define VLV_IOSF_ADDR (VLV_DISPLAY_BASE + 0x2108)
>>>
>>> +#define CHV_IOSF_PORT_NC 0x04
>> Use IOSF_PORT_PUNIT instead of defining this?
>
> Yes, Agreed, I will address this
>
>>> /* See configdb bunit SB addr map */
>>> #define BUNIT_REG_BISOC 0x11
>>>
>>> @@ -529,6 +530,14 @@ enum punit_power_well {
>>> #define PUNIT_FUSE_BUS2 0xf6 /* bits 47:40 */
>>> #define PUNIT_FUSE_BUS1 0xf5 /* bits 55:48 */
>>>
>>> +#define CHV_IOSF_NC_FB_GFX_FREQ_FUSE 0xdb
>>> +#define CHV_FB_GFX_MAX_FREQ_FUSE_SHIFT 16
>>> +#define CHV_FB_GFX_MAX_FREQ_FUSE_MASK 0xff
>>> +
>>> +#define CHV_IOSF_NC_FB_GFX_RPE_FUSE 0xdf
>>> +#define CHV_FB_RPE_FREQ_SHIFT 8
>>> +#define CHV_FB_RPE_FREQ_MASK 0xff
>>> +
>> These seem to be also part of punit space so I would prefer:
>> PUNIT_REG_GPU_STATUS 0xdb
>> PUNIT_GPU_STATUS_MAX_FREQ_SHIFT 16
>> PUNIT_GPU_STATUS_MAX_FREQ_MASK 0xff
>> PUNIT_REG_GPU_DUTYCYCLE 0xdf
>>
>> etc...
>
> I can change. Q? don't we want to identify the register with CHV?
If you like, add /* chv */ after those punit regs you add.
I would not globber the namespace more. As in this case only chv
code will use these inside cherryview_* named functions.
>>> #define IOSF_NC_FB_GFX_FREQ_FUSE 0x1c
>>> #define FB_GFX_MAX_FREQ_FUSE_SHIFT 3
>>> #define FB_GFX_MAX_FREQ_FUSE_MASK 0x000007f8
>>> @@ -933,6 +942,7 @@ enum punit_power_well {
>>> #define SANDYBRIDGE_FENCE_PITCH_SHIFT 32
>>> #define GEN7_FENCE_MAX_PITCH_VAL 0x0800
>>>
>>> +
>>> /* control register for cpu gtt access */
>>> #define TILECTL 0x101000
>>> #define TILECTL_SWZCTL (1 << 0)
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 1816c52..08dcdc5 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -3731,6 +3731,38 @@ void gen6_update_ring_freq(struct drm_device *dev)
>>> mutex_unlock(&dev_priv->rps.hw_lock);
>>> }
>>>
>>> +int cherryview_rps_max_freq(struct drm_i915_private *dev_priv)
>>> +{
>>> + u32 val, rp0;
>>> +
>>> + val = chv_nc_read(dev_priv, CHV_IOSF_NC_FB_GFX_FREQ_FUSE);
>>> +
>> I didn't find any reason we couldn't use vlv_punit_read().
>
> I am adding separate function to be inline with VLV. If needed we can modify both VLV and CHV
> I would prefer to keep distinguish between fuse and punit read.
>>> + rp0 = (val >> CHV_FB_GFX_MAX_FREQ_FUSE_SHIFT) &
>>> + CHV_FB_GFX_MAX_FREQ_FUSE_MASK;
>>> +
>>> + return rp0;
>>> +}
>>> +
>>> +static int cherryview_rps_rpe_freq(struct drm_i915_private *dev_priv)
>>> +{
>>> + u32 val, rpe;
>>> +
>>> + val = chv_nc_read(dev_priv, CHV_IOSF_NC_FB_GFX_RPE_FUSE);
>>> + rpe = (val >> CHV_FB_RPE_FREQ_SHIFT) & CHV_FB_RPE_FREQ_MASK;
>>> +
>>> + return rpe;
>>> +}
>>> +
>>> +int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
>>> +{
>>> + u32 val, rpn;
>>> +
>>> + val = chv_nc_read(dev_priv, CHV_IOSF_NC_FB_GFX_FREQ_FUSE);
>>> + rpn = (val >> CHV_FB_RPE_FREQ_SHIFT) & CHV_FB_RPE_FREQ_MASK;
>>> +
>> Please don't reuse mask/shift from other register even tho
>> they happen to be identical. This confuses the reader alot.
>> Define new ones with proper naming.
>>
>>> + return rpn;
>>> +}
>>> +
>>> int valleyview_rps_max_freq(struct drm_i915_private *dev_priv)
>>> {
>>> u32 val, rp0;
>>> @@ -3890,7 +3922,36 @@ static void valleyview_init_gt_powersave(struct drm_device *dev)
>>>
>>> static void cherryview_init_gt_powersave(struct drm_device *dev)
>>> {
>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>> +
>>> cherryview_setup_pctx(dev);
>>> +
>>> + mutex_lock(&dev_priv->rps.hw_lock);
>>> +
>>> + dev_priv->rps.max_freq = cherryview_rps_max_freq(dev_priv);
>>> + dev_priv->rps.rp0_freq = dev_priv->rps.max_freq;
>>> + DRM_DEBUG_DRIVER("max GPU freq: %d MHz (%u)\n",
>>> + vlv_gpu_freq(dev_priv, dev_priv->rps.max_freq),
>>> + dev_priv->rps.max_freq);
>>> +
>>> + dev_priv->rps.efficient_freq = cherryview_rps_rpe_freq(dev_priv);
>>> + DRM_DEBUG_DRIVER("RPe GPU freq: %d MHz (%u)\n",
>>> + vlv_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
>>> + dev_priv->rps.efficient_freq);
>>> +
>>> + dev_priv->rps.min_freq = cherryview_rps_min_freq(dev_priv);
>>> + DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
>>> + vlv_gpu_freq(dev_priv, dev_priv->rps.min_freq),
>>> + dev_priv->rps.min_freq);
>>> +
>>> + /* Preserve min/max settings in case of re-init */
>>> + if (dev_priv->rps.max_freq_softlimit == 0)
>>> + dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq;
>>> +
>>> + if (dev_priv->rps.min_freq_softlimit == 0)
>>> + dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq;
>>> +
>>> + mutex_unlock(&dev_priv->rps.hw_lock);
>>> }
>>>
>>> static void valleyview_cleanup_gt_powersave(struct drm_device *dev)
>>> @@ -3902,7 +3963,7 @@ static void cherryview_enable_rps(struct drm_device *dev)
>>> {
>>> struct drm_i915_private *dev_priv = dev->dev_private;
>>> struct intel_engine_cs *ring;
>>> - u32 gtfifodbg, rc6_mode = 0, pcbr;
>>> + u32 gtfifodbg, val, rc6_mode = 0, pcbr;
>>> int i;
>>>
>>> WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>>> @@ -3949,6 +4010,38 @@ static void cherryview_enable_rps(struct drm_device *dev)
>>>
>>> I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
>>>
>>> + /* 4 Program defaults and thresholds for RPS*/
>>> + I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
>>> + I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000);
>>> + I915_WRITE(GEN6_RP_UP_EI, 66000);
>>> + I915_WRITE(GEN6_RP_DOWN_EI, 350000);
>>> +
>>> + I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
>>> +
>>> + /* 5: Enable RPS */
>>> + I915_WRITE(GEN6_RP_CONTROL,
>>> + GEN6_RP_MEDIA_HW_NORMAL_MODE |
>>> + GEN6_RP_MEDIA_IS_GFX |
>>> + GEN6_RP_ENABLE |
>>> + GEN6_RP_UP_BUSY_AVG |
>>> + GEN6_RP_DOWN_IDLE_AVG);
>>> +
>>> + val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
>>> +
>>> + DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no");
>>> + DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val);
>>> +
>>> + dev_priv->rps.cur_freq = (val >> 8) & 0xff;
>>> + DRM_DEBUG_DRIVER("current GPU freq: %d MHz (%u)\n",
>>> + vlv_gpu_freq(dev_priv, dev_priv->rps.cur_freq),
>>> + dev_priv->rps.cur_freq);
>>> +
>>> + DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
>>> + vlv_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
>>> + dev_priv->rps.efficient_freq);
>>> +
>>> + valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
>>> +
>>> gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
>>> index 01d841e..a74f60b 100644
>>> --- a/drivers/gpu/drm/i915/intel_sideband.c
>>> +++ b/drivers/gpu/drm/i915/intel_sideband.c
>>> @@ -115,6 +115,20 @@ void vlv_bunit_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
>>> SB_CRWRDA_NP, reg, &val);
>>> }
>>>
>>> +u32 chv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
>>> +{
>>> + u32 val = 0;
>>> +
>>> + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>>> +
>>> + mutex_lock(&dev_priv->dpio_lock);
>>> + vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), CHV_IOSF_PORT_NC,
>>> + SB_CRRDDA_NP, addr, &val);
>>> + mutex_unlock(&dev_priv->dpio_lock);
>>> +
>>> + return val;
>>> +}
>>> +
>> Use vlv_punit_read() and you can get rid of this function.
>
> Same as above. I would prefer to keep distinguish between fuse and punit read. If needed i can change
> Let me know you thoughts?
The registers are in the punit space. The sideband target address is
punit target address. For what I know, this is a punit access.
Please explain why this fuse distingtion is needed/comes from.
If it has been fuse read in past then we have opportunity to forget the
ugly past in here and match the code with the documentation. If this is
the case, please change it to punit read and remove references to 'nc'
Thanks,
-Mika
>> -Mika
>>
>>> u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
>>> {
>>> u32 val = 0;
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
