On Thu, Jan 26, 2023 at 10:46:01AM +0000, Tvrtko Ursulin wrote: > > On 25/01/2023 19:04, Matt Roper wrote: > > On Wed, Jan 25, 2023 at 10:51:53AM +0000, Tvrtko Ursulin wrote: > > > > > > On 24/01/2023 20:54, john.c.harri...@intel.com wrote: > > > > From: John Harrison <john.c.harri...@intel.com> > > > > > > > > Uncore is really part of the GT. So use the GT specific debug/error > > > > That's not really true; uncore should be outside the GT since it's used > > for all kinds of non-GT stuff as well (sgunit, display, etc.). I > > believe "uncore" is just an old-fashioned name for what modern docs > > refer to as "system agent" these days. > > > > Granted, our i915 design does stretch the truth quite a bit today by > > rolling some of the GT-specific concepts into the uncore code (GT > > forcewake/shadowing, GSI offset, etc.). Having two intel_uncore > > structures on a platform like MTL doesn't really match the hardware > > reality at the lowest levels, but allows us to update the software for > > these new platforms without major, intrusive changes for all platforms. > > > > I feel like including 'gt' information in log messages unrelated to GT > > might be confusing. For display stuff it's probably obvious that the GT > > information is bogus, but when stuff is related to the sgunit it won't > > always be so obvious. > > Level of confusing vs absence of debugability and a suggested way forward? > Just do nothing and accept any forcewake related errors will not include the > originating GT?
I guess it's probably fine to keep it on all messages; people will just learn to ignore the bogus GT stuff on things that aren't actually related to the GT. It would still be good to change the commit message though so that people doing git archaeology in the future don't get an incorrect understanding of the relationship. Matt > > Regards, > > Tvrtko > > > > > > > Matt > > > > > > message helpers so as to get the GT index in the prints. > > > > > > Conversion looks good to me and on balance it's better to include the > > > origin > > > in logs even for messages which strictly are not GT related, than not to > > > have the origin at all (intel_de_... helpers, I *think*). > > > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com> > > > > > > I'll just add Jani and Matt in case they have a different opinion. > > > > > > Regards, > > > > > > Tvrtko > > > > > > > Signed-off-by: John Harrison <john.c.harri...@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_uncore.c | 133 > > > > +++++++++++++--------------- > > > > 1 file changed, 63 insertions(+), 70 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c > > > > b/drivers/gpu/drm/i915/intel_uncore.c > > > > index 8dee9e62a73ee..4e357477c6592 100644 > > > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > > > @@ -25,6 +25,7 @@ > > > > #include <linux/pm_runtime.h> > > > > #include "gt/intel_engine_regs.h" > > > > +#include "gt/intel_gt_print.h" > > > > #include "gt/intel_gt_regs.h" > > > > #include "i915_drv.h" > > > > @@ -83,8 +84,7 @@ static void mmio_debug_resume(struct intel_uncore > > > > *uncore) > > > > uncore->debug->unclaimed_mmio_check = > > > > uncore->debug->saved_mmio_check; > > > > if (check_for_unclaimed_mmio(uncore)) > > > > - drm_info(&uncore->i915->drm, > > > > - "Invalid mmio detected during user access\n"); > > > > + gt_info(uncore->gt, "Invalid mmio detected during user > > > > access\n"); > > > > spin_unlock(&uncore->debug->lock); > > > > } > > > > @@ -179,9 +179,9 @@ static inline void > > > > fw_domain_wait_ack_clear(const struct intel_uncore_forcewake_domain > > > > *d) > > > > { > > > > if (wait_ack_clear(d, FORCEWAKE_KERNEL)) { > > > > - drm_err(&d->uncore->i915->drm, > > > > - "%s: timed out waiting for forcewake ack to > > > > clear.\n", > > > > - intel_uncore_forcewake_domain_to_str(d->id)); > > > > + gt_err(d->uncore->gt, > > > > + "%s: timed out waiting for forcewake ack to > > > > clear.\n", > > > > + intel_uncore_forcewake_domain_to_str(d->id)); > > > > add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI > > > > now unreliable */ > > > > } > > > > } > > > > @@ -228,12 +228,11 @@ fw_domain_wait_ack_with_fallback(const struct > > > > intel_uncore_forcewake_domain *d, > > > > fw_clear(d, FORCEWAKE_KERNEL_FALLBACK); > > > > } while (!ack_detected && pass++ < 10); > > > > - drm_dbg(&d->uncore->i915->drm, > > > > - "%s had to use fallback to %s ack, 0x%x (passes %u)\n", > > > > - intel_uncore_forcewake_domain_to_str(d->id), > > > > - type == ACK_SET ? "set" : "clear", > > > > - fw_ack(d), > > > > - pass); > > > > + gt_dbg(d->uncore->gt, "%s had to use fallback to %s ack, 0x%x > > > > (passes %u)\n", > > > > + intel_uncore_forcewake_domain_to_str(d->id), > > > > + type == ACK_SET ? "set" : "clear", > > > > + fw_ack(d), > > > > + pass); > > > > return ack_detected ? 0 : -ETIMEDOUT; > > > > } > > > > @@ -258,9 +257,8 @@ static inline void > > > > fw_domain_wait_ack_set(const struct intel_uncore_forcewake_domain *d) > > > > { > > > > if (wait_ack_set(d, FORCEWAKE_KERNEL)) { > > > > - drm_err(&d->uncore->i915->drm, > > > > - "%s: timed out waiting for forcewake ack > > > > request.\n", > > > > - intel_uncore_forcewake_domain_to_str(d->id)); > > > > + gt_err(d->uncore->gt, "%s: timed out waiting for > > > > forcewake ack request.\n", > > > > + intel_uncore_forcewake_domain_to_str(d->id)); > > > > add_taint_for_CI(d->uncore->i915, TAINT_WARN); /* CI > > > > now unreliable */ > > > > } > > > > } > > > > @@ -366,9 +364,9 @@ static void __gen6_gt_wait_for_thread_c0(struct > > > > intel_uncore *uncore) > > > > * w/a for a sporadic read returning 0 by waiting for the GT > > > > * thread to wake up. > > > > */ > > > > - drm_WARN_ONCE(&uncore->i915->drm, > > > > - wait_for_atomic_us(gt_thread_status(uncore) == 0, > > > > 5000), > > > > - "GT thread status wait timed out\n"); > > > > + gt_WARN_ONCE(uncore->gt, > > > > + wait_for_atomic_us(gt_thread_status(uncore) == 0, > > > > 5000), > > > > + "GT thread status wait timed out\n"); > > > > } > > > > static void fw_domains_get_with_thread_status(struct intel_uncore > > > > *uncore, > > > > @@ -402,8 +400,7 @@ static void __gen6_gt_wait_for_fifo(struct > > > > intel_uncore *uncore) > > > > if (wait_for_atomic((n = fifo_free_entries(uncore)) > > > > > GT_FIFO_NUM_RESERVED_ENTRIES, > > > > GT_FIFO_TIMEOUT_MS)) { > > > > - drm_dbg(&uncore->i915->drm, > > > > - "GT_FIFO timeout, entries: %u\n", n); > > > > + gt_dbg(uncore->gt, "GT_FIFO timeout, entries: > > > > %u\n", n); > > > > return; > > > > } > > > > } > > > > @@ -476,7 +473,7 @@ intel_uncore_forcewake_reset(struct intel_uncore > > > > *uncore) > > > > break; > > > > if (--retry_count == 0) { > > > > - drm_err(&uncore->i915->drm, "Timed out waiting > > > > for forcewake timers to finish\n"); > > > > + gt_err(uncore->gt, "Timed out waiting for > > > > forcewake timers to finish\n"); > > > > break; > > > > } > > > > @@ -484,7 +481,7 @@ intel_uncore_forcewake_reset(struct intel_uncore > > > > *uncore) > > > > cond_resched(); > > > > } > > > > - drm_WARN_ON(&uncore->i915->drm, active_domains); > > > > + gt_WARN_ON(uncore->gt, active_domains); > > > > fw = uncore->fw_domains_active; > > > > if (fw) > > > > @@ -520,8 +517,7 @@ fpga_check_for_unclaimed_mmio(struct intel_uncore > > > > *uncore) > > > > * to recognize when MMIO accesses are just busted. > > > > */ > > > > if (unlikely(dbg == ~0)) > > > > - drm_err(&uncore->i915->drm, > > > > - "Lost access to MMIO BAR; all registers now > > > > read back as 0xFFFFFFFF!\n"); > > > > + gt_err(uncore->gt, "Lost access to MMIO BAR; all > > > > registers now read back as 0xFFFFFFFF!\n"); > > > > __raw_uncore_write32(uncore, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); > > > > @@ -550,7 +546,7 @@ gen6_check_for_fifo_debug(struct intel_uncore > > > > *uncore) > > > > fifodbg = __raw_uncore_read32(uncore, GTFIFODBG); > > > > if (unlikely(fifodbg)) { > > > > - drm_dbg(&uncore->i915->drm, "GTFIFODBG = 0x08%x\n", > > > > fifodbg); > > > > + gt_dbg(uncore->gt, "GTFIFODBG = 0x08%x\n", fifodbg); > > > > __raw_uncore_write32(uncore, GTFIFODBG, fifodbg); > > > > } > > > > @@ -622,7 +618,7 @@ void intel_uncore_resume_early(struct intel_uncore > > > > *uncore) > > > > unsigned int restore_forcewake; > > > > if (intel_uncore_unclaimed_mmio(uncore)) > > > > - drm_dbg(&uncore->i915->drm, "unclaimed mmio detected on > > > > resume, clearing\n"); > > > > + gt_dbg(uncore->gt, "unclaimed mmio detected on resume, > > > > clearing\n"); > > > > if (!intel_uncore_has_forcewake(uncore)) > > > > return; > > > > @@ -847,9 +843,9 @@ void assert_forcewakes_inactive(struct intel_uncore > > > > *uncore) > > > > if (!uncore->fw_get_funcs) > > > > return; > > > > - drm_WARN(&uncore->i915->drm, uncore->fw_domains_active, > > > > - "Expected all fw_domains to be inactive, but %08x are > > > > still on\n", > > > > - uncore->fw_domains_active); > > > > + gt_WARN(uncore->gt, uncore->fw_domains_active, > > > > + "Expected all fw_domains to be inactive, but %08x are > > > > still on\n", > > > > + uncore->fw_domains_active); > > > > } > > > > void assert_forcewakes_active(struct intel_uncore *uncore, > > > > @@ -869,9 +865,9 @@ void assert_forcewakes_active(struct intel_uncore > > > > *uncore, > > > > assert_rpm_wakelock_held(uncore->rpm); > > > > fw_domains &= uncore->fw_domains; > > > > - drm_WARN(&uncore->i915->drm, fw_domains & > > > > ~uncore->fw_domains_active, > > > > - "Expected %08x fw_domains to be active, but %08x are > > > > off\n", > > > > - fw_domains, fw_domains & ~uncore->fw_domains_active); > > > > + gt_WARN(uncore->gt, fw_domains & ~uncore->fw_domains_active, > > > > + "Expected %08x fw_domains to be active, but %08x are > > > > off\n", > > > > + fw_domains, fw_domains & ~uncore->fw_domains_active); > > > > /* > > > > * Check that the caller has an explicit wakeref and we don't > > > > mistake > > > > @@ -884,9 +880,9 @@ void assert_forcewakes_active(struct intel_uncore > > > > *uncore, > > > > if (uncore->fw_domains_timer & domain->mask) > > > > expect++; /* pending automatic release */ > > > > - if (drm_WARN(&uncore->i915->drm, actual < expect, > > > > - "Expected domain %d to be held awake by > > > > caller, count=%d\n", > > > > - domain->id, actual)) > > > > + if (gt_WARN(uncore->gt, actual < expect, > > > > + "Expected domain %d to be held awake by > > > > caller, count=%d\n", > > > > + domain->id, actual)) > > > > break; > > > > } > > > > @@ -955,9 +951,9 @@ find_fw_domain(struct intel_uncore *uncore, u32 > > > > offset) > > > > if (entry->domains == FORCEWAKE_ALL) > > > > return uncore->fw_domains; > > > > - drm_WARN(&uncore->i915->drm, entry->domains & > > > > ~uncore->fw_domains, > > > > - "Uninitialized forcewake domain(s) 0x%x accessed at > > > > 0x%x\n", > > > > - entry->domains & ~uncore->fw_domains, offset); > > > > + gt_WARN(uncore->gt, entry->domains & ~uncore->fw_domains, > > > > + "Uninitialized forcewake domain(s) 0x%x accessed at > > > > 0x%x\n", > > > > + entry->domains & ~uncore->fw_domains, offset); > > > > return entry->domains; > > > > } > > > > @@ -1190,7 +1186,7 @@ static int mmio_range_cmp(u32 key, const struct > > > > i915_range *range) > > > > static bool is_shadowed(struct intel_uncore *uncore, u32 offset) > > > > { > > > > - if (drm_WARN_ON(&uncore->i915->drm, > > > > !uncore->shadowed_reg_table)) > > > > + if (gt_WARN_ON(uncore->gt, !uncore->shadowed_reg_table)) > > > > return false; > > > > if (IS_GSI_REG(offset)) > > > > @@ -1898,11 +1894,11 @@ __unclaimed_reg_debug(struct intel_uncore > > > > *uncore, > > > > const i915_reg_t reg, > > > > const bool read) > > > > { > > > > - if (drm_WARN(&uncore->i915->drm, > > > > - check_for_unclaimed_mmio(uncore), > > > > - "Unclaimed %s register 0x%x\n", > > > > - read ? "read from" : "write to", > > > > - i915_mmio_reg_offset(reg))) > > > > + if (gt_WARN(uncore->gt, > > > > + check_for_unclaimed_mmio(uncore), > > > > + "Unclaimed %s register 0x%x\n", > > > > + read ? "read from" : "write to", > > > > + i915_mmio_reg_offset(reg))) > > > > /* Only report the first N failures */ > > > > uncore->i915->params.mmio_debug--; > > > > } > > > > @@ -1913,10 +1909,10 @@ __unclaimed_previous_reg_debug(struct > > > > intel_uncore *uncore, > > > > const bool read) > > > > { > > > > if (check_for_unclaimed_mmio(uncore)) > > > > - drm_dbg(&uncore->i915->drm, > > > > - "Unclaimed access detected before %s register > > > > 0x%x\n", > > > > - read ? "read from" : "write to", > > > > - i915_mmio_reg_offset(reg)); > > > > + gt_dbg(uncore->gt, > > > > + "Unclaimed access detected before %s register > > > > 0x%x\n", > > > > + read ? "read from" : "write to", > > > > + i915_mmio_reg_offset(reg)); > > > > } > > > > static inline void > > > > @@ -2201,8 +2197,8 @@ static int __fw_domain_init(struct intel_uncore > > > > *uncore, > > > > if (!d) > > > > return -ENOMEM; > > > > - drm_WARN_ON(&uncore->i915->drm, !i915_mmio_reg_valid(reg_set)); > > > > - drm_WARN_ON(&uncore->i915->drm, !i915_mmio_reg_valid(reg_ack)); > > > > + gt_WARN_ON(uncore->gt, !i915_mmio_reg_valid(reg_set)); > > > > + gt_WARN_ON(uncore->gt, !i915_mmio_reg_valid(reg_ack)); > > > > d->uncore = uncore; > > > > d->wake_count = 0; > > > > @@ -2254,8 +2250,8 @@ static void fw_domain_fini(struct intel_uncore > > > > *uncore, > > > > return; > > > > uncore->fw_domains &= ~BIT(domain_id); > > > > - drm_WARN_ON(&uncore->i915->drm, d->wake_count); > > > > - drm_WARN_ON(&uncore->i915->drm, hrtimer_cancel(&d->timer)); > > > > + gt_WARN_ON(uncore->gt, d->wake_count); > > > > + gt_WARN_ON(uncore->gt, hrtimer_cancel(&d->timer)); > > > > kfree(d); > > > > } > > > > @@ -2388,8 +2384,8 @@ static int intel_uncore_fw_domains_init(struct > > > > intel_uncore *uncore) > > > > spin_unlock_irq(&uncore->lock); > > > > if (!(ecobus & FORCEWAKE_MT_ENABLE)) { > > > > - drm_info(&i915->drm, "No MT forcewake available > > > > on Ivybridge, this can result in issues\n"); > > > > - drm_info(&i915->drm, "when using vblank-synced > > > > partial screen updates.\n"); > > > > + gt_info(uncore->gt, "No MT forcewake available > > > > on Ivybridge, this can result in issues\n"); > > > > + gt_info(uncore->gt, "when using vblank-synced > > > > partial screen updates.\n"); > > > > fw_domain_fini(uncore, FW_DOMAIN_ID_RENDER); > > > > fw_domain_init(uncore, FW_DOMAIN_ID_RENDER, > > > > FORCEWAKE, FORCEWAKE_ACK); > > > > @@ -2403,7 +2399,7 @@ static int intel_uncore_fw_domains_init(struct > > > > intel_uncore *uncore) > > > > #undef fw_domain_init > > > > /* All future platforms are expected to require complex power > > > > gating */ > > > > - drm_WARN_ON(&i915->drm, !ret && uncore->fw_domains == 0); > > > > + gt_WARN_ON(uncore->gt, !ret && uncore->fw_domains == 0); > > > > out: > > > > if (ret) > > > > @@ -2487,7 +2483,7 @@ int intel_uncore_setup_mmio(struct intel_uncore > > > > *uncore, phys_addr_t phys_addr) > > > > uncore->regs = ioremap(phys_addr, mmio_size); > > > > if (uncore->regs == NULL) { > > > > - drm_err(&i915->drm, "failed to map registers\n"); > > > > + gt_err(uncore->gt, "failed to map registers\n"); > > > > return -EIO; > > > > } > > > > @@ -2615,7 +2611,7 @@ int intel_uncore_init_mmio(struct intel_uncore > > > > *uncore) > > > > */ > > > > if (IS_DGFX(i915) && > > > > !(__raw_uncore_read32(uncore, GU_CNTL) & LMEM_INIT)) { > > > > - drm_err(&i915->drm, "LMEM not initialized by > > > > firmware\n"); > > > > + gt_err(uncore->gt, "LMEM not initialized by > > > > firmware\n"); > > > > return -ENODEV; > > > > } > > > > @@ -2646,7 +2642,7 @@ int intel_uncore_init_mmio(struct intel_uncore > > > > *uncore) > > > > /* clear out unclaimed reg detection bit */ > > > > if (intel_uncore_unclaimed_mmio(uncore)) > > > > - drm_dbg(&i915->drm, "unclaimed mmio detected on uncore > > > > init, clearing\n"); > > > > + gt_dbg(uncore->gt, "unclaimed mmio detected on uncore > > > > init, clearing\n"); > > > > return 0; > > > > } > > > > @@ -2721,11 +2717,10 @@ void > > > > intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore, > > > > */ > > > > static void driver_initiated_flr(struct intel_uncore *uncore) > > > > { > > > > - struct drm_i915_private *i915 = uncore->i915; > > > > const unsigned int flr_timeout_ms = 3000; /* specs recommend a > > > > 3s wait */ > > > > int ret; > > > > - drm_dbg(&i915->drm, "Triggering Driver-FLR\n"); > > > > + gt_dbg(uncore->gt, "Triggering Driver-FLR\n"); > > > > /* > > > > * Make sure any pending FLR requests have cleared by waiting > > > > for the > > > > @@ -2738,9 +2733,7 @@ static void driver_initiated_flr(struct > > > > intel_uncore *uncore) > > > > */ > > > > ret = intel_wait_for_register_fw(uncore, GU_CNTL, DRIVERFLR, 0, > > > > flr_timeout_ms); > > > > if (ret) { > > > > - drm_err(&i915->drm, > > > > - "Failed to wait for Driver-FLR bit to clear! > > > > %d\n", > > > > - ret); > > > > + gt_err(uncore->gt, "Failed to wait for Driver-FLR bit > > > > to clear! %d\n", ret); > > > > return; > > > > } > > > > intel_uncore_write_fw(uncore, GU_DEBUG, DRIVERFLR_STATUS); > > > > @@ -2752,7 +2745,7 @@ static void driver_initiated_flr(struct > > > > intel_uncore *uncore) > > > > DRIVERFLR_STATUS, > > > > DRIVERFLR_STATUS, > > > > flr_timeout_ms); > > > > if (ret) { > > > > - drm_err(&i915->drm, "wait for Driver-FLR completion > > > > failed! %d\n", ret); > > > > + gt_err(uncore->gt, "wait for Driver-FLR completion > > > > failed! %d\n", ret); > > > > return; > > > > } > > > > @@ -2911,7 +2904,7 @@ intel_uncore_arm_unclaimed_mmio_detection(struct > > > > intel_uncore *uncore) > > > > { > > > > bool ret = false; > > > > - if (drm_WARN_ON(&uncore->i915->drm, !uncore->debug)) > > > > + if (gt_WARN_ON(uncore->gt, !uncore->debug)) > > > > return false; > > > > spin_lock_irq(&uncore->debug->lock); > > > > @@ -2921,10 +2914,10 @@ > > > > intel_uncore_arm_unclaimed_mmio_detection(struct intel_uncore *uncore) > > > > if (unlikely(check_for_unclaimed_mmio(uncore))) { > > > > if (!uncore->i915->params.mmio_debug) { > > > > - drm_dbg(&uncore->i915->drm, > > > > - "Unclaimed register detected, " > > > > - "enabling oneshot unclaimed register > > > > reporting. " > > > > - "Please use i915.mmio_debug=N for more > > > > information.\n"); > > > > + gt_dbg(uncore->gt, > > > > + "Unclaimed register detected, " > > > > + "enabling oneshot unclaimed register > > > > reporting. " > > > > + "Please use i915.mmio_debug=N for more > > > > information.\n"); > > > > uncore->i915->params.mmio_debug++; > > > > } > > > > uncore->debug->unclaimed_mmio_check--; > > > > @@ -2957,7 +2950,7 @@ intel_uncore_forcewake_for_reg(struct > > > > intel_uncore *uncore, > > > > { > > > > enum forcewake_domains fw_domains = 0; > > > > - drm_WARN_ON(&uncore->i915->drm, !op); > > > > + gt_WARN_ON(uncore->gt, !op); > > > > if (!intel_uncore_has_forcewake(uncore)) > > > > return 0; > > > > @@ -2968,7 +2961,7 @@ intel_uncore_forcewake_for_reg(struct > > > > intel_uncore *uncore, > > > > if (op & FW_REG_WRITE) > > > > fw_domains |= uncore->funcs.write_fw_domains(uncore, > > > > reg); > > > > - drm_WARN_ON(&uncore->i915->drm, fw_domains & > > > > ~uncore->fw_domains); > > > > + gt_WARN_ON(uncore->gt, fw_domains & ~uncore->fw_domains); > > > > return fw_domains; > > > > } > > -- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation