[Intel-gfx] [PATCH v2 09/10] drm/i915: add uncore flags
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Wed Mar 20 01:28:30 UTC 2019
On 3/19/19 5:56 PM, Paulo Zanoni wrote:
> Em ter, 2019-03-19 às 11:35 -0700, Daniele Ceraolo Spurio escreveu:
>> Save some uncore properties to avoid having to jump back to
>> dev_priv every time
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 4 +-
>> drivers/gpu/drm/i915/intel_display.c | 2 +-
>> drivers/gpu/drm/i915/intel_hangcheck.c | 2 +-
>> drivers/gpu/drm/i915/intel_uncore.c | 82 ++++++++++---------
>> drivers/gpu/drm/i915/intel_uncore.h | 10 ++-
>> drivers/gpu/drm/i915/selftests/intel_uncore.c | 15 ++--
>> 6 files changed, 65 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index ca41a3da1918..c609bcac8577 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -2849,7 +2849,7 @@ static int intel_runtime_suspend(struct device *kdev)
>> enable_rpm_wakeref_asserts(dev_priv);
>> intel_runtime_pm_cleanup(dev_priv);
>>
>> - if (intel_uncore_arm_unclaimed_mmio_detection(dev_priv))
>> + if (intel_uncore_arm_unclaimed_mmio_detection(&dev_priv->uncore))
>> DRM_ERROR("Unclaimed access detected prior to suspending\n");
>>
>> dev_priv->runtime_pm.suspended = true;
>> @@ -2903,7 +2903,7 @@ static int intel_runtime_resume(struct device *kdev)
>>
>> intel_opregion_notify_adapter(dev_priv, PCI_D0);
>> dev_priv->runtime_pm.suspended = false;
>> - if (intel_uncore_unclaimed_mmio(dev_priv))
>> + if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
>> DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
>>
>> if (INTEL_GEN(dev_priv) >= 11) {
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index d4fcad136120..cfe379e938e6 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13517,7 +13517,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>> * so enable debugging for the next modeset - and hope we catch
>> * the culprit.
>> */
>> - intel_uncore_arm_unclaimed_mmio_detection(dev_priv);
>> + intel_uncore_arm_unclaimed_mmio_detection(&dev_priv->uncore);
>> intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET, wakeref);
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hangcheck.c b/drivers/gpu/drm/i915/intel_hangcheck.c
>> index 57ed49dc19c4..125662c64934 100644
>> --- a/drivers/gpu/drm/i915/intel_hangcheck.c
>> +++ b/drivers/gpu/drm/i915/intel_hangcheck.c
>> @@ -270,7 +270,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
>> * periodically arm the mmio checker to see if we are triggering
>> * any invalid access.
>> */
>> - intel_uncore_arm_unclaimed_mmio_detection(dev_priv);
>> + intel_uncore_arm_unclaimed_mmio_detection(&dev_priv->uncore);
>>
>> for_each_engine(engine, dev_priv, id) {
>> struct hangcheck hc;
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 1816eeae3ba9..26b28afb4500 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -509,18 +509,17 @@ gen6_check_for_fifo_debug(struct intel_uncore *uncore)
>> }
>>
>> static bool
>> -check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>> +check_for_unclaimed_mmio(struct intel_uncore *uncore)
>> {
>> - struct intel_uncore *uncore = &dev_priv->uncore;
>> bool ret = false;
>>
>> - if (HAS_FPGA_DBG_UNCLAIMED(dev_priv))
>> + if (uncore->flags & UNCORE_HAS_FPGA_DBG_UNCLAIMED)
>> ret |= fpga_check_for_unclaimed_mmio(uncore);
>>
>> - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> + if (uncore->flags & UNCORE_HAS_DBG_UNCLAIMED)
>> ret |= vlv_check_for_unclaimed_mmio(uncore);
>>
>> - if (IS_GEN_RANGE(dev_priv, 6, 7))
>> + if (uncore->flags & UNCORE_HAS_FIFO)
>> ret |= gen6_check_for_fifo_debug(uncore);
>>
>> return ret;
>> @@ -529,14 +528,12 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>> static void __intel_uncore_early_sanitize(struct intel_uncore *uncore,
>> unsigned int restore_forcewake)
>> {
>> - struct drm_i915_private *i915 = uncore_to_i915(uncore);
>> -
>> /* clear out unclaimed reg detection bit */
>> - if (check_for_unclaimed_mmio(i915))
>> + if (check_for_unclaimed_mmio(uncore))
>> DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
>>
>> /* WaDisableShadowRegForCpd:chv */
>> - if (IS_CHERRYVIEW(i915)) {
>> + if (IS_CHERRYVIEW(uncore_to_i915(uncore))) {
>> __raw_i915_write32(uncore, GTFIFOCTL,
>> __raw_i915_read32(uncore, GTFIFOCTL) |
>> GT_FIFO_CTL_BLOCK_ALL_POLICY_STALL |
>> @@ -549,7 +546,7 @@ static void __intel_uncore_early_sanitize(struct intel_uncore *uncore,
>> spin_lock_irq(&uncore->lock);
>> uncore->funcs.force_wake_get(uncore, restore_forcewake);
>>
>> - if (IS_GEN_RANGE(i915, 6, 7))
>> + if (uncore->flags & UNCORE_HAS_FIFO)
>> uncore->fifo_count = fifo_free_entries(uncore);
>> spin_unlock_irq(&uncore->lock);
>> }
>> @@ -668,12 +665,10 @@ void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
>> */
>> void intel_uncore_forcewake_user_put(struct intel_uncore *uncore)
>> {
>> - struct drm_i915_private *i915 = uncore_to_i915(uncore);
>> -
>> spin_lock_irq(&uncore->lock);
>> if (!--uncore->user_forcewake.count) {
>> - if (intel_uncore_unclaimed_mmio(i915))
>> - dev_info(i915->drm.dev,
>> + if (intel_uncore_unclaimed_mmio(uncore))
>> + dev_info(uncore_to_i915(uncore)->drm.dev,
>> "Invalid mmio detected during user access\n");
>>
>> uncore->unclaimed_mmio_check =
>> @@ -1072,12 +1067,12 @@ ilk_dummy_write(struct intel_uncore *uncore)
>> }
>>
>> static void
>> -__unclaimed_reg_debug(struct drm_i915_private *dev_priv,
>> +__unclaimed_reg_debug(struct intel_uncore *uncore,
>> const i915_reg_t reg,
>> const bool read,
>> const bool before)
>> {
>> - if (WARN(check_for_unclaimed_mmio(dev_priv) && !before,
>> + if (WARN(check_for_unclaimed_mmio(uncore) && !before,
>> "Unclaimed %s register 0x%x\n",
>> read ? "read from" : "write to",
>> i915_mmio_reg_offset(reg)))
>> @@ -1086,7 +1081,7 @@ __unclaimed_reg_debug(struct drm_i915_private *dev_priv,
>> }
>>
>> static inline void
>> -unclaimed_reg_debug(struct drm_i915_private *dev_priv,
>> +unclaimed_reg_debug(struct intel_uncore *uncore,
>> const i915_reg_t reg,
>> const bool read,
>> const bool before)
>> @@ -1094,7 +1089,7 @@ unclaimed_reg_debug(struct drm_i915_private *dev_priv,
>> if (likely(!i915_modparams.mmio_debug))
>> return;
>>
>> - __unclaimed_reg_debug(dev_priv, reg, read, before);
>> + __unclaimed_reg_debug(uncore, reg, read, before);
>> }
>>
>> #define GEN2_READ_HEADER(x) \
>> @@ -1145,10 +1140,10 @@ __gen2_read(64)
>> u##x val = 0; \
>> assert_rpm_wakelock_held(dev_priv); \
>> spin_lock_irqsave(&uncore->lock, irqflags); \
>> - unclaimed_reg_debug(dev_priv, reg, true, true)
>> + unclaimed_reg_debug(uncore, reg, true, true)
>>
>> #define GEN6_READ_FOOTER \
>> - unclaimed_reg_debug(dev_priv, reg, true, false); \
>> + unclaimed_reg_debug(uncore, reg, true, false); \
>> spin_unlock_irqrestore(&uncore->lock, irqflags); \
>> trace_i915_reg_rw(false, reg, val, sizeof(val), trace); \
>> return val
>> @@ -1259,10 +1254,10 @@ __gen2_write(32)
>> trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \
>> assert_rpm_wakelock_held(dev_priv); \
>> spin_lock_irqsave(&uncore->lock, irqflags); \
>> - unclaimed_reg_debug(dev_priv, reg, false, true)
>> + unclaimed_reg_debug(uncore, reg, false, true)
>>
>> #define GEN6_WRITE_FOOTER \
>> - unclaimed_reg_debug(dev_priv, reg, false, false); \
>> + unclaimed_reg_debug(uncore, reg, false, false); \
>> spin_unlock_irqrestore(&uncore->lock, irqflags)
>>
>> #define __gen6_write(x) \
>> @@ -1391,7 +1386,7 @@ static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>> {
>> struct drm_i915_private *i915 = uncore_to_i915(uncore);
>>
>> - if (INTEL_GEN(i915) <= 5 || intel_vgpu_active(i915))
>> + if (!(uncore->flags & UNCORE_HAS_FORCEWAKE))
>> return;
>>
>> if (INTEL_GEN(i915) >= 11) {
>> @@ -1590,6 +1585,9 @@ int intel_uncore_init(struct intel_uncore *uncore)
>>
>> i915_check_vgpu(i915);
>>
>> + if (INTEL_GEN(i915) > 5 && !intel_vgpu_active(i915))
>> + uncore->flags |= UNCORE_HAS_FORCEWAKE;
>> +
>> intel_uncore_edram_detect(i915);
>> intel_uncore_fw_domains_init(uncore);
>> __intel_uncore_early_sanitize(uncore, 0);
>> @@ -1598,12 +1596,14 @@ int intel_uncore_init(struct intel_uncore *uncore)
>> uncore->pmic_bus_access_nb.notifier_call =
>> i915_pmic_bus_access_notifier;
>>
>> - if (IS_GEN_RANGE(i915, 2, 4) || intel_vgpu_active(i915)) {
>> - ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen2);
>> - ASSIGN_READ_MMIO_VFUNCS(uncore, gen2);
>> - } else if (IS_GEN(i915, 5)) {
>> - ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen5);
>> - ASSIGN_READ_MMIO_VFUNCS(uncore, gen5);
>> + if (!(uncore->flags & UNCORE_HAS_FORCEWAKE)) {
>
> This changes the behavior when it's gen5 and vgpu is active. Is this
> even possible? If not, will it ever be? Whether intentional or not,
> perhaps it needs to be in a separate patch.
It shouldn't be possible, GVT is gen8+. Gen5 doesn't have PPGTT so it is
not something you can run virtualization on and that is why I didn't
care of that case.
>
> Since this is still the init function, maybe we don't even need this
> change.
True, I just thought that since I had the flag I could make use of it to
remove some redundant checks :P
>
>
>> + if (IS_GEN(i915, 5)) {
>> + ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen5);
>> + ASSIGN_READ_MMIO_VFUNCS(uncore, gen5);
>> + } else {
>> + ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen2);
>> + ASSIGN_READ_MMIO_VFUNCS(uncore, gen2);
>> + }
>> } else if (IS_GEN_RANGE(i915, 6, 7)) {
>> ASSIGN_WRITE_MMIO_VFUNCS(uncore, gen6);
>>
>> @@ -1633,6 +1633,15 @@ int intel_uncore_init(struct intel_uncore *uncore)
>> ASSIGN_READ_MMIO_VFUNCS(uncore, gen11_fwtable);
>> }
>>
>> + if (HAS_FPGA_DBG_UNCLAIMED(i915))
>> + uncore->flags |= UNCORE_HAS_FPGA_DBG_UNCLAIMED;
>> +
>> + if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
>> + uncore->flags |= UNCORE_HAS_DBG_UNCLAIMED;
>> +
>> + if (IS_GEN_RANGE(i915, 6, 7))
>> + uncore->flags |= UNCORE_HAS_FIFO;
>> +
>> iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
>>
>> return 0;
>> @@ -1864,15 +1873,14 @@ int __intel_wait_for_register(struct drm_i915_private *dev_priv,
>> return ret;
>> }
>>
>> -bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
>> +bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore)
>> {
>> - return check_for_unclaimed_mmio(dev_priv);
>> + return check_for_unclaimed_mmio(uncore);
>> }
>>
>> bool
>> -intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
>> +intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore)
>> {
>> - struct intel_uncore *uncore = &dev_priv->uncore;
>> bool ret = false;
>>
>> spin_lock_irq(&uncore->lock);
>> @@ -1880,7 +1888,7 @@ intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
>> if (unlikely(uncore->unclaimed_mmio_check <= 0))
>> goto out;
>>
>> - if (unlikely(intel_uncore_unclaimed_mmio(dev_priv))) {
>> + if (unlikely(intel_uncore_unclaimed_mmio(uncore))) {
>> if (!i915_modparams.mmio_debug) {
>> DRM_DEBUG("Unclaimed register detected, "
>> "enabling oneshot unclaimed register reporting. "
>> @@ -1912,7 +1920,7 @@ intel_uncore_forcewake_for_read(struct drm_i915_private *dev_priv,
>> } else if (INTEL_GEN(dev_priv) >= 6) {
>> fw_domains = __gen6_reg_read_fw_domains(uncore, offset);
>> } else {
>> - WARN_ON(!IS_GEN_RANGE(dev_priv, 2, 5));
>> + WARN_ON(!(uncore->flags & UNCORE_HAS_FORCEWAKE));
>
> That doesn't look equivalent. Isn't it always gonna WARN now? Gen 2-5
> never has the flag.
ooops, you're right the check is the wrong way around
>
> The whole function is about checking for gens, when we switch to
> checking the flag we make it harder to read, especially since the
> reader now has to think about whether vgpu_active() was true during
> initialization. I'm not sure this is an improvement.
I've read this check as "if we didn't match any of the above cases on a
platform with forcewake then we're missing something, so warn", kind of
like a MISSING_CASE macro. What gen is the platform or whether we're on
a vgpu or not is not really something we care about here IMO.
>
>> fw_domains = 0;
>> }
>>
>> @@ -1938,7 +1946,7 @@ intel_uncore_forcewake_for_write(struct drm_i915_private *dev_priv,
>> } else if (IS_GEN_RANGE(dev_priv, 6, 7)) {
>> fw_domains = FORCEWAKE_RENDER;
>> } else {
>> - WARN_ON(!IS_GEN_RANGE(dev_priv, 2, 5));
>> + WARN_ON(!(uncore->flags & UNCORE_HAS_FORCEWAKE));
>
> Same here.
>
ooops x2 :P
>> fw_domains = 0;
>> }
>>
>> @@ -1969,7 +1977,7 @@ intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv,
>>
>> WARN_ON(!op);
>>
>> - if (intel_vgpu_active(dev_priv))
>> + if (!(dev_priv->uncore.flags & UNCORE_HAS_FORCEWAKE))
>
> This changes the vgpu_active() from something you call all the time to
> a static check that is only ever checked during initialization. A quick
> read at the vgpu code does not tell me whether this is something that
> can change over time (i.e., the result intel_vgpu_active() is always
> the same), but my first guess would be that it could change over time.
AFAIU intel_vgpu_active() tells us whether we're running on a virtual
GPU so it can't change at runtime.
>
> Still, this is the kind of change that should be in its own patch if
> it's a behavior change and not just a rework in function arguments.
>
I'll split it out
> -
>
> Overall, I'm not super sold in the current approach of the patch.
> Perhaps it's fine to actually call uncore_to_i915() in the functions
> that need to do gen or feature checks (e.g.,
> check_for_unclaimed_mmio()). Maybe instead of adding the flags and
> converting everything to intel_uncore we should keep the conversion of
> everything to intel_uncore and then use uncore_to_i915() when needed.
>
> I'm not against the patch, but we at least need to sort out the
> possible behavior changes (or go the easier way to just convert all
> these functions and then use uncore_to_i915() when needed).
I just wanted to reduce the calls to uncore_to_i915 as much as possible
for extra encapsulation. My goal is to have no dev_priv involvment is
the full read/write flow at the end of the rework, but if the general
consensus is to use uncore_to_i915() instead I don't mind switching to that.
Thanks for the feedback!
Daniele
>
> Having more opinions on the design would probably help. Ping @everyone.
>
>> return 0;
>>
>> if (op & FW_REG_READ)
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
>> index d345e5ab04a5..6df6586bb603 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.h
>> +++ b/drivers/gpu/drm/i915/intel_uncore.h
>> @@ -127,6 +127,12 @@ struct intel_uncore {
>> } user_forcewake;
>>
>> int unclaimed_mmio_check;
>> +
>> + u32 flags;
>> +#define UNCORE_HAS_FORCEWAKE BIT(0)
>> +#define UNCORE_HAS_FPGA_DBG_UNCLAIMED BIT(1)
>> +#define UNCORE_HAS_DBG_UNCLAIMED BIT(2)
>> +#define UNCORE_HAS_FIFO BIT(3)
>> };
>>
>> /* Iterate over initialised fw domains */
>> @@ -146,8 +152,8 @@ forcewake_domain_to_uncore(const struct intel_uncore_forcewake_domain *d)
>> void intel_uncore_sanitize(struct drm_i915_private *dev_priv);
>> int intel_uncore_init(struct intel_uncore *uncore);
>> void intel_uncore_prune(struct intel_uncore *uncore);
>> -bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
>> -bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
>> +bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore);
>> +bool intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore);
>> void intel_uncore_fini(struct intel_uncore *uncore);
>> void intel_uncore_suspend(struct intel_uncore *uncore);
>> void intel_uncore_resume_early(struct intel_uncore *uncore);
>> diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
>> index 69aa260b479d..d2c6a03fb29c 100644
>> --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c
>> @@ -122,6 +122,7 @@ int intel_uncore_mock_selftests(void)
>> static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_priv)
>> {
>> #define FW_RANGE 0x40000
>> + struct intel_uncore *uncore = &dev_priv->uncore;
>> unsigned long *valid;
>> u32 offset;
>> int err;
>> @@ -142,31 +143,31 @@ static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_pri
>> if (!valid)
>> return -ENOMEM;
>>
>> - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL);
>> + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
>>
>> - check_for_unclaimed_mmio(dev_priv);
>> + check_for_unclaimed_mmio(uncore);
>> for (offset = 0; offset < FW_RANGE; offset += 4) {
>> i915_reg_t reg = { offset };
>>
>> (void)I915_READ_FW(reg);
>> - if (!check_for_unclaimed_mmio(dev_priv))
>> + if (!check_for_unclaimed_mmio(uncore))
>> set_bit(offset, valid);
>> }
>>
>> - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL);
>> + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
>>
>> err = 0;
>> for_each_set_bit(offset, valid, FW_RANGE) {
>> i915_reg_t reg = { offset };
>>
>> iosf_mbi_punit_acquire();
>> - intel_uncore_forcewake_reset(&dev_priv->uncore);
>> + intel_uncore_forcewake_reset(uncore);
>> iosf_mbi_punit_release();
>>
>> - check_for_unclaimed_mmio(dev_priv);
>> + check_for_unclaimed_mmio(uncore);
>>
>> (void)I915_READ(reg);
>> - if (check_for_unclaimed_mmio(dev_priv)) {
>> + if (check_for_unclaimed_mmio(uncore)) {
>> pr_err("Unclaimed mmio read to register 0x%04x\n",
>> offset);
>> err = -EINVAL;
>
More information about the Intel-gfx
mailing list