Re: [PATCH 55/89] drm/i915/skl: Gen9 multi-engine forcewake

Mika Kuoppala
2014-09-25T07:32:24Z

Bob Wang <zhe1.wang@intel.com> writes:

> On 09/22/2014 04:11 PM, Mika Kuoppala wrote:
>> Damien Lespiau <damien.lespiau@intel.com> writes:
>>
>>> From: Zhe Wang <zhe1.wang@intel.com>
>>>
>>> Enable multi-engine forcewake for Gen9.
>>>
>>> v2: Rebase on top of nightly
>>> Move the register range definitions to intel_uncore.c
>>> Whitespace fixes
>>> (Damien)
>>>
>>> Signed-off-by: Zhe Wang <zhe1.wang@intel.com>
>>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_uncore.c | 120 ++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 120 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>>> index 7b7fc9e..f289f4f 100644
>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>> @@ -674,6 +674,34 @@ void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
>>> REG_RANGE((reg), 0x14000, 0x14400) || \
>>> REG_RANGE((reg), 0x22000, 0x24000))
>>>
>>> +#define FORCEWAKE_GEN9_UNCORE_RANGE_OFFSET(reg) \
>>> + ((reg) >= 0xC00 && (reg) < 0x2000)
>>> +
>>> +#define FORCEWAKE_GEN9_RENDER_RANGE_OFFSET(reg) \
>>> + (((reg) >= 0x2000 && (reg) < 0x4000) || \
>>> + ((reg) >= 0x5200 && (reg) < 0x8000) || \
>>> + ((reg) >= 0x8300 && (reg) < 0x8500) || \
>>> + ((reg) >= 0x8C00 && (reg) < 0x8D00) || \
>>> + ((reg) >= 0xB000 && (reg) < 0xB480) || \
>>> + ((reg) >= 0xE000 && (reg) < 0xE800))
>>> +
>>> +#define FORCEWAKE_GEN9_MEDIA_RANGE_OFFSET(reg) \
>>> + (((reg) >= 0x8800 && (reg) < 0x8A00) || \
>>> + ((reg) >= 0xD000 && (reg) < 0xD800) || \
>>> + ((reg) >= 0x12000 && (reg) < 0x14000) || \
>>> + ((reg) >= 0x1A000 && (reg) < 0x1EA00) || \
>>> + ((reg) >= 0x30000 && (reg) < 0x40000))
>>> +
>>> +#define FORCEWAKE_GEN9_COMMON_RANGE_OFFSET(reg) \
>>> + ((reg) >= 0x9400 && (reg) < 0x9800)
>> Please use REG_RANGE for all above.
> ok, we can change this for these macros.
>>> +#define FORCEWAKE_GEN9_BLITTER_RANGE_OFFSET(reg) \
>>> + ((reg) < 0x40000 &&\
>>> + !FORCEWAKE_GEN9_UNCORE_RANGE_OFFSET(reg) && \
>>> + !FORCEWAKE_GEN9_RENDER_RANGE_OFFSET(reg) && \
>>> + !FORCEWAKE_GEN9_MEDIA_RANGE_OFFSET(reg) && \
>>> + !FORCEWAKE_GEN9_COMMON_RANGE_OFFSET(reg))
>>> +
>>> static void
>>> ilk_dummy_write(struct drm_i915_private *dev_priv)
>>> {
>>> @@ -804,6 +832,43 @@ chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>>> REG_READ_FOOTER; \
>>> }
>>>
>>> +#define __gen9_read(x) \
>>> +static u##x \
>>> +gen9_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>>> + REG_READ_HEADER(x); \
>>> + if (!NEEDS_FORCE_WAKE((dev_priv), (reg)) || \
>> As gen9 doesn't have FORCEWAKE register, you can save one
>> comparison here if you discard the macro and opencode.
> Hi Mika,
> Excuse me, I didn't really get this. can you explain more on it?
>
> Gen9 has 3 separate forcewake registers.
> we still have to check for 2 ranges to ensure forcewake is not required
> 1. >= 0x40000
> 2. 0xC00-0x1FFF (this range is new and always accessible)

I was a little bit vague on what macro I was talking about.

The macro in question is:

/* We give fast paths for the really cool registers */
#define NEEDS_FORCE_WAKE(dev_priv, reg) \
((reg) < 0x40000 && (reg) != FORCEWAKE)

and as we dont have generic FORCEWAKE (0xA18C) on gen9,
we end up doing one needless != for each register access.

-Mika

> Thanks,
> Bob
>>> + FORCEWAKE_GEN9_UNCORE_RANGE_OFFSET(reg)) { \
>>> + val = __raw_i915_read##x(dev_priv, reg); \
>>> + } else { \
>>> + unsigned fwengine = 0; \
>>> + if (FORCEWAKE_GEN9_RENDER_RANGE_OFFSET(reg)) { \
>>> + if (dev_priv->uncore.fw_rendercount == 0) \
>>> + fwengine = FORCEWAKE_RENDER; \
>>> + } else if (FORCEWAKE_GEN9_MEDIA_RANGE_OFFSET(reg)) { \
>>> + if (dev_priv->uncore.fw_mediacount == 0) \
>>> + fwengine = FORCEWAKE_MEDIA; \
>>> + } else if (FORCEWAKE_GEN9_COMMON_RANGE_OFFSET(reg)) { \
>>> + if (dev_priv->uncore.fw_rendercount == 0) \
>>> + fwengine |= FORCEWAKE_RENDER; \
>>> + if (dev_priv->uncore.fw_mediacount == 0) \
>>> + fwengine |= FORCEWAKE_MEDIA; \
>>> + } else { \
>>> + if (dev_priv->uncore.fw_blittercount == 0) \
>>> + fwengine = FORCEWAKE_BLITTER; \
>>> + } \
>>> + if (fwengine) \
>>> + dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
>>> + val = __raw_i915_read##x(dev_priv, reg); \
>>> + if (fwengine) \
>>> + dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
>>> + } \
>>> + REG_READ_FOOTER; \
>>> +}
>>> +
>>> +__gen9_read(8)
>>> +__gen9_read(16)
>>> +__gen9_read(32)
>>> +__gen9_read(64)
>>> __chv_read(8)
>>> __chv_read(16)
>>> __chv_read(32)
>>> @@ -825,6 +890,7 @@ __gen4_read(16)
>>> __gen4_read(32)
>>> __gen4_read(64)
>>>
>>> +#undef __gen9_read
>>> #undef __chv_read
>>> #undef __vlv_read
>>> #undef __gen6_read
>>> @@ -962,6 +1028,46 @@ chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
>>> REG_WRITE_FOOTER; \
>>> }
>>>
>>> +#define __gen9_write(x) \
>>> +static void \
>>> +gen9_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, \
>>> + bool trace) { \
>>> + REG_WRITE_HEADER; \
>>> + if (!NEEDS_FORCE_WAKE((dev_priv), (reg)) || \
>> ditto
>>
>> -Mika
>>
>>> + FORCEWAKE_GEN9_UNCORE_RANGE_OFFSET(reg)) { \
>>> + __raw_i915_write##x(dev_priv, reg, val); \
>>> + } else { \
>>> + unsigned fwengine = 0; \
>>> + if (FORCEWAKE_GEN9_RENDER_RANGE_OFFSET(reg)) { \
>>> + if (dev_priv->uncore.fw_rendercount == 0) \
>>> + fwengine = FORCEWAKE_RENDER; \
>>> + } else if (FORCEWAKE_GEN9_MEDIA_RANGE_OFFSET(reg)) { \
>>> + if (dev_priv->uncore.fw_mediacount == 0) \
>>> + fwengine = FORCEWAKE_MEDIA; \
>>> + } else if (FORCEWAKE_GEN9_COMMON_RANGE_OFFSET(reg)) { \
>>> + if (dev_priv->uncore.fw_rendercount == 0) \
>>> + fwengine |= FORCEWAKE_RENDER; \
>>> + if (dev_priv->uncore.fw_mediacount == 0) \
>>> + fwengine |= FORCEWAKE_MEDIA; \
>>> + } else { \
>>> + if (dev_priv->uncore.fw_blittercount == 0) \
>>> + fwengine = FORCEWAKE_BLITTER; \
>>> + } \
>>> + if (fwengine) \
>>> + dev_priv->uncore.funcs.force_wake_get(dev_priv, \
>>> + fwengine); \
>>> + __raw_i915_write##x(dev_priv, reg, val); \
>>> + if (fwengine) \
>>> + dev_priv->uncore.funcs.force_wake_put(dev_priv, \
>>> + fwengine); \
>>> + } \
>>> + REG_WRITE_FOOTER; \
>>> +}
>>> +
>>> +__gen9_write(8)
>>> +__gen9_write(16)
>>> +__gen9_write(32)
>>> +__gen9_write(64)
>>> __chv_write(8)
>>> __chv_write(16)
>>> __chv_write(32)
>>> @@ -987,6 +1093,7 @@ __gen4_write(16)
>>> __gen4_write(32)
>>> __gen4_write(64)
>>>
>>> +#undef __gen9_write
>>> #undef __chv_write
>>> #undef __gen8_write
>>> #undef __hsw_write
>>> @@ -1054,6 +1161,19 @@ void intel_uncore_init(struct drm_device *dev)
>>>
>>> switch (INTEL_INFO(dev)->gen) {
>>> default:
>>> + WARN_ON(1);
>>> + return;
>>> + case 9:
>>> + dev_priv->uncore.funcs.mmio_writeb = gen9_write8;
>>> + dev_priv->uncore.funcs.mmio_writew = gen9_write16;
>>> + dev_priv->uncore.funcs.mmio_writel = gen9_write32;
>>> + dev_priv->uncore.funcs.mmio_writeq = gen9_write64;
>>> + dev_priv->uncore.funcs.mmio_readb = gen9_read8;
>>> + dev_priv->uncore.funcs.mmio_readw = gen9_read16;
>>> + dev_priv->uncore.funcs.mmio_readl = gen9_read32;
>>> + dev_priv->uncore.funcs.mmio_readq = gen9_read64;
>>> + break;
>>> + case 8:
>>> if (IS_CHERRYVIEW(dev)) {
>>> dev_priv->uncore.funcs.mmio_writeb = chv_write8;
>>> dev_priv->uncore.funcs.mmio_writew = chv_write16;
>>> --
>>> 1.8.3.1
>>>
>>> _______________________________________________
>>> 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