[Intel-gfx] [PATCH v2 09/10] drm/i915: add uncore flags
Paulo Zanoni
paulo.r.zanoni at intel.com
Wed Mar 20 00:56:45 UTC 2019
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.
Since this is still the init function, maybe we don't even need this
change.
> + 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.
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.
> 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.
> 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.
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.
-
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).
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