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@xxxxxxxxx> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> --- 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.cindex 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;
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
