| Message ID | 20211007230903.4084-1-andi@etezian.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/gt: move remaining debugfs interfaces into gt | expand |
On Thu, Oct 7, 2021 at 5:27 PM Andi Shyti <andi@etezian.org> wrote: > > From: Andi Shyti <andi.shyti@intel.com> > > The following interfaces: > > i915_wedged > i915_forcewake_user > i915_gem_interrupt > > are dependent on gt values. Put them inside gt/ and drop the > "i915_" prefix name. This would be the new structure: > > dri/0/gt > | > +-- forcewake_user > | > +-- interrupt_info > | > \-- reset > > For backwards compatibility with existing igt (and the slight > semantic difference between operating on the i915 abi entry > points and the deep gt info): > > dri/0 > | > +-- i915_wedged > | > \-- i915_forcewake_user > > remain at the top level. > > Signed-off-by: Andi Shyti <andi.shyti@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > Hi, > > I am reproposing this patch exactly as it was proposed initially > where the original interfaces are kept where they have been > originally placed. It might generate some duplicated code but, > well, it's debugfs and I don't see any issue. In the future we > can transform the upper interfaces to act upon all the GTs and > provide information from all the GTs. This is, for example, how > the sysfs interfaces will act. NACK. We've made this mistake in the past for other debugfs files. We don't want to do it again just to maintain 2 separate places for one year and then finally realize we want to merge them. > > The reason I removed them in V1 is because igt as only user is > not a strong reason to keep duplicated code, but as Chris > suggested offline: > > "It's debugfs, igt is the primary consumer. CI has to be bridged over > changes to the interfaces it is using in any case, as you want > comparable results before/after the patches land. That doesn't mean you have to copy and paste it. It may mean you do the implementation in one of them and the other calls that implementation. See how I did the deduplication in commit d0c560316d6f ("drm/i915: deduplicate frequency dump on debugfs") Alternative would be to prepare igt already and then add a Test-with: in this patch series.... But I think it makes more sense to support both locations for some time and then later remove the previous one. Thanks Lucas De Marchi
Hi Lucas, > > I am reproposing this patch exactly as it was proposed initially > > where the original interfaces are kept where they have been > > originally placed. It might generate some duplicated code but, > > well, it's debugfs and I don't see any issue. In the future we > > can transform the upper interfaces to act upon all the GTs and > > provide information from all the GTs. This is, for example, how > > the sysfs interfaces will act. > > NACK. We've made this mistake in the past for other debugfs files. > We don't want to do it again just to maintain 2 separate places for > one year and then finally realize we want to merge them. In my opinion it's all about what mistake you like the most because until we will have multi-gt support in upstream all the patches come with the "promise" of a follow-up and maintenance cost. > > The reason I removed them in V1 is because igt as only user is > > not a strong reason to keep duplicated code, but as Chris > > suggested offline: > > > > "It's debugfs, igt is the primary consumer. CI has to be bridged over > > changes to the interfaces it is using in any case, as you want > > comparable results before/after the patches land. > > That doesn't mean you have to copy and paste it. It may mean you > do the implementation in one of them and the other calls that implementation. > See how I did the deduplication in commit d0c560316d6f ("drm/i915: > deduplicate frequency dump on debugfs") In this case, from a user perspective, which gt is the interface affecting? is it affecting all the system? or gt 0, 1...? Does the user know? The maintenance cost is that later you will need to use for_each_gt and make all those interfaces multitile, and this would be your "promise". How are you going to do it then? Will every interface iterate and perform its own action? When you read, whad do you read? all the gt values in 'or'? in 'and'? Is there any common strategy? Or will we have inconsistent behaviors? In sysfs (where we are left with the same questions) some times ago I proposoed a common solution for all the upper level files in order to provide the user with a consistent interface all along the GTs. This is my "promise" and until then it's just a matter of what promise and what mistake you like the most. > Alternative would be to prepare igt already and then add a Test-with: > in this patch > series.... But I think it makes more sense to support both locations > for some time and then later > remove the previous one. Anyway, I can sure do something similar to how you did it, it might look prettier but it doesn't exclude a follow-up improvement. Thanks for the review, Andi
On Fri, Oct 8, 2021 at 3:14 AM Andi Shyti <andi.shyti@intel.com> wrote: > > Hi Lucas, > > > > I am reproposing this patch exactly as it was proposed initially > > > where the original interfaces are kept where they have been > > > originally placed. It might generate some duplicated code but, > > > well, it's debugfs and I don't see any issue. In the future we > > > can transform the upper interfaces to act upon all the GTs and > > > provide information from all the GTs. This is, for example, how > > > the sysfs interfaces will act. > > > > NACK. We've made this mistake in the past for other debugfs files. > > We don't want to do it again just to maintain 2 separate places for > > one year and then finally realize we want to merge them. > > In my opinion it's all about what mistake you like the most > because until we will have multi-gt support in upstream all the > patches come with the "promise" of a follow-up and maintenance > cost. no. If you put the implementation in a single place, later you only have the decision on what to do with the per-device entrypoint: - should we remove it once igt is converted? - should we make it iterate all gts? - should we make it mean root tile? Then you take the action that is needed and decide it per interface. Here you are leaving behind a lot of code that we will need to maintain until there is support for such a thing. It already happened once: we needed to maintain that duplicated code for over a year with multiple patches changing them (or failing to do so). > > > > The reason I removed them in V1 is because igt as only user is > > > not a strong reason to keep duplicated code, but as Chris > > > suggested offline: > > > > > > "It's debugfs, igt is the primary consumer. CI has to be bridged over > > > changes to the interfaces it is using in any case, as you want > > > comparable results before/after the patches land. > > > > That doesn't mean you have to copy and paste it. It may mean you > > do the implementation in one of them and the other calls that implementation. > > See how I did the deduplication in commit d0c560316d6f ("drm/i915: > > deduplicate frequency dump on debugfs") > > In this case, from a user perspective, which gt is the interface > affecting? is it affecting all the system? or gt 0, 1...? Does > the user know? The maintenance cost is that later you will need > to use for_each_gt and make all those interfaces multitile, and > this would be your "promise". multi-gt is irrelevant here. This patch without any "promise" should do what the commit message says: *move*. The only reason to keep the old entrypoint around is because it's missing the igt conversion. If you are going to support a per-device entrypoint and do for_each_gt(), or do a symlink to the root tile, or whatever, it isn't very relevant to this patch. Right now we have just a single directory, gt. Lucas De Marchi
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index cdc244bbbfc1..e92984954ba8 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -98,6 +98,7 @@ gt-y += \ gt/intel_gt_debugfs.o \ gt/intel_gt_engines_debugfs.o \ gt/intel_gt_irq.o \ + gt/intel_gt_irq_debugfs.o \ gt/intel_gt_pm.o \ gt/intel_gt_pm_debugfs.o \ gt/intel_gt_pm_irq.o \ diff --git a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c index 1fe19ccd2794..d3075c138585 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c @@ -8,11 +8,54 @@ #include "i915_drv.h" #include "intel_gt_debugfs.h" #include "intel_gt_engines_debugfs.h" +#include "intel_gt_irq_debugfs.h" +#include "intel_gt_pm.h" #include "intel_gt_pm_debugfs.h" +#include "intel_gt_requests.h" #include "intel_sseu_debugfs.h" #include "pxp/intel_pxp_debugfs.h" #include "uc/intel_uc_debugfs.h" +static int reset_show(void *data, u64 *val) +{ + struct intel_gt *gt = data; + int ret = intel_gt_terminally_wedged(gt); + + switch (ret) { + case -EIO: + *val = 1; + return 0; + case 0: + *val = 0; + return 0; + default: + return ret; + } +} + +static int reset_store(void *data, u64 val) +{ + struct intel_gt *gt = data; + + /* Flush any previous reset before applying for a new one */ + wait_event(gt->reset.queue, + !test_bit(I915_RESET_BACKOFF, >->reset.flags)); + + intel_gt_handle_error(gt, val, I915_ERROR_CAPTURE, + "Manually reset engine mask to %llx", val); + return 0; +} +DEFINE_SIMPLE_ATTRIBUTE(reset_fops, reset_show, reset_store, "%llu\n"); + +static void gt_debugfs_register(struct intel_gt *gt, struct dentry *root) +{ + static const struct intel_gt_debugfs_file files[] = { + { "reset", &reset_fops, NULL }, + }; + + intel_gt_debugfs_register_files(root, files, ARRAY_SIZE(files), gt); +} + void intel_gt_debugfs_register(struct intel_gt *gt) { struct dentry *root; @@ -24,10 +67,12 @@ void intel_gt_debugfs_register(struct intel_gt *gt) if (IS_ERR(root)) return; + gt_debugfs_register(gt, root); + intel_gt_engines_debugfs_register(gt, root); intel_gt_pm_debugfs_register(gt, root); + intel_gt_irq_debugfs_register(gt, root); intel_sseu_debugfs_register(gt, root); - intel_uc_debugfs_register(>->uc, root); intel_pxp_debugfs_register(>->pxp, root); } diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_irq_debugfs.c new file mode 100644 index 000000000000..3cf9ae8437e5 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq_debugfs.c @@ -0,0 +1,178 @@ +// SPDX-License-Identifier: MIT + +/* + * Copyright © 2020 Intel Corporation + */ + +#include "i915_drv.h" +#include "intel_gt_debugfs.h" +#include "intel_gt_irq_debugfs.h" + +static int interrupt_info_show(struct seq_file *m, void *data) +{ + struct intel_gt *gt = m->private; + struct drm_i915_private *i915 = gt->i915; + struct intel_uncore *uncore = gt->uncore; + struct intel_engine_cs *engine; + enum intel_engine_id id; + intel_wakeref_t wakeref; + int i; + + wakeref = intel_runtime_pm_get(uncore->rpm); + + if (IS_CHERRYVIEW(i915)) { + seq_printf(m, "Master Interrupt Control:\t%08x\n", + intel_uncore_read(uncore, GEN8_MASTER_IRQ)); + + for (i = 0; i < 4; i++) { + seq_printf(m, "GT Interrupt IMR %d:\t%08x\n", + i, intel_uncore_read(uncore, + GEN8_GT_IMR(i))); + seq_printf(m, "GT Interrupt IIR %d:\t%08x\n", + i, intel_uncore_read(uncore, + GEN8_GT_IIR(i))); + seq_printf(m, "GT Interrupt IER %d:\t%08x\n", + i, intel_uncore_read(uncore, + GEN8_GT_IER(i))); + } + + } else if (GRAPHICS_VER(i915) >= 11) { + seq_printf(m, "Master Interrupt Control: %08x\n", + intel_uncore_read(uncore, GEN11_GFX_MSTR_IRQ)); + + seq_printf(m, "Render/Copy Intr Enable: %08x\n", + intel_uncore_read(uncore, + GEN11_RENDER_COPY_INTR_ENABLE)); + seq_printf(m, "VCS/VECS Intr Enable: %08x\n", + intel_uncore_read(uncore, + GEN11_VCS_VECS_INTR_ENABLE)); + seq_printf(m, "GUC/SG Intr Enable:\t %08x\n", + intel_uncore_read(uncore, + GEN11_GUC_SG_INTR_ENABLE)); + seq_printf(m, "GPM/WGBOXPERF Intr Enable: %08x\n", + intel_uncore_read(uncore, + GEN11_GPM_WGBOXPERF_INTR_ENABLE)); + seq_printf(m, "Crypto Intr Enable:\t %08x\n", + intel_uncore_read(uncore, + GEN11_CRYPTO_RSVD_INTR_ENABLE)); + seq_printf(m, "GUnit/CSME Intr Enable:\t %08x\n", + intel_uncore_read(uncore, + GEN11_GUNIT_CSME_INTR_ENABLE)); + + } else if (GRAPHICS_VER(i915) >= 8) { + seq_printf(m, "Master Interrupt Control:\t%08x\n", + intel_uncore_read(uncore, GEN8_MASTER_IRQ)); + + for (i = 0; i < 4; i++) { + seq_printf(m, "GT Interrupt IMR %d:\t%08x\n", + i, intel_uncore_read(uncore, + GEN8_GT_IMR(i))); + seq_printf(m, "GT Interrupt IIR %d:\t%08x\n", + i, intel_uncore_read(uncore, + GEN8_GT_IIR(i))); + seq_printf(m, "GT Interrupt IER %d:\t%08x\n", + i, intel_uncore_read(uncore, + GEN8_GT_IER(i))); + } + + } else if (IS_VALLEYVIEW(i915)) { + seq_printf(m, "Master IER:\t%08x\n", + intel_uncore_read(uncore, VLV_MASTER_IER)); + + seq_printf(m, "Render IER:\t%08x\n", + intel_uncore_read(uncore, GTIER)); + seq_printf(m, "Render IIR:\t%08x\n", + intel_uncore_read(uncore, GTIIR)); + seq_printf(m, "Render IMR:\t%08x\n", + intel_uncore_read(uncore, GTIMR)); + + seq_printf(m, "PM IER:\t\t%08x\n", + intel_uncore_read(uncore, GEN6_PMIER)); + seq_printf(m, "PM IIR:\t\t%08x\n", + intel_uncore_read(uncore, GEN6_PMIIR)); + seq_printf(m, "PM IMR:\t\t%08x\n", + intel_uncore_read(uncore, GEN6_PMIMR)); + + } else if (!HAS_PCH_SPLIT(i915)) { + seq_printf(m, "Interrupt enable: %08x\n", + intel_uncore_read(uncore, GEN2_IER)); + seq_printf(m, "Interrupt identity: %08x\n", + intel_uncore_read(uncore, GEN2_IIR)); + seq_printf(m, "Interrupt mask: %08x\n", + intel_uncore_read(uncore, GEN2_IMR)); + } else { + seq_printf(m, "Graphics Interrupt enable: %08x\n", + intel_uncore_read(uncore, GTIER)); + seq_printf(m, "Graphics Interrupt identity: %08x\n", + intel_uncore_read(uncore, GTIIR)); + seq_printf(m, "Graphics Interrupt mask: %08x\n", + intel_uncore_read(uncore, GTIMR)); + } + + if (GRAPHICS_VER(i915) >= 11) { + seq_printf(m, "RCS Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN11_RCS0_RSVD_INTR_MASK)); + seq_printf(m, "BCS Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN11_BCS_RSVD_INTR_MASK)); + seq_printf(m, "VCS0/VCS1 Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN11_VCS0_VCS1_INTR_MASK)); + seq_printf(m, "VCS2/VCS3 Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN11_VCS2_VCS3_INTR_MASK)); + + if (HAS_ENGINE(gt, VCS4) || HAS_ENGINE(gt, VCS5)) + seq_printf(m, "VCS4/VCS5 Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN12_VCS4_VCS5_INTR_MASK)); + if (HAS_ENGINE(gt, VCS6) || HAS_ENGINE(gt, VCS7)) + seq_printf(m, "VCS6/VCS7 Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN12_VCS6_VCS7_INTR_MASK)); + + seq_printf(m, "VECS0/VECS1 Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN11_VECS0_VECS1_INTR_MASK)); + + if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3)) + seq_printf(m, "VECS2/VECS3 Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN12_VECS2_VECS3_INTR_MASK)); + + seq_printf(m, "GUC/SG Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN11_GUC_SG_INTR_MASK)); + seq_printf(m, "GPM/WGBOXPERF Intr Mask: %08x\n", + intel_uncore_read(uncore, + GEN11_GPM_WGBOXPERF_INTR_MASK)); + seq_printf(m, "Crypto Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN11_CRYPTO_RSVD_INTR_MASK)); + seq_printf(m, "Gunit/CSME Intr Mask:\t %08x\n", + intel_uncore_read(uncore, + GEN11_GUNIT_CSME_INTR_MASK)); + + } else if (GRAPHICS_VER(i915) >= 6) { + for_each_engine(engine, gt, id) { + seq_printf(m, + "Graphics Interrupt mask (%s): %08x\n", + engine->name, ENGINE_READ(engine, RING_IMR)); + } + } + + intel_runtime_pm_put(uncore->rpm, wakeref); + + return 0; +} +DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(interrupt_info); + +void intel_gt_irq_debugfs_register(struct intel_gt *gt, struct dentry *root) +{ + static const struct intel_gt_debugfs_file files[] = { + { "interrupt_info", &interrupt_info_fops, NULL }, + }; + + intel_gt_debugfs_register_files(root, files, ARRAY_SIZE(files), gt); +} diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq_debugfs.h b/drivers/gpu/drm/i915/gt/intel_gt_irq_debugfs.h new file mode 100644 index 000000000000..95e519705001 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq_debugfs.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: MIT */ + +/* + * Copyright © 2020 Intel Corporation + */ + +#ifndef INTEL_GT_IRQ_DEBUGFS_H +#define INTEL_GT_IRQ_DEBUGFS_H + +struct intel_gt; +struct dentry; + +void intel_gt_irq_debugfs_register(struct intel_gt *gt, struct dentry *root); + +#endif /* INTEL_GT_IRQ_DEBUGFS_H */ diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c index 5f84ad602642..c75af6f97e7e 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c @@ -19,6 +19,36 @@ #include "intel_sideband.h" #include "intel_uncore.h" +static int forcewake_user_open(struct inode *inode, struct file *file) +{ + struct intel_gt *gt = inode->i_private; + + atomic_inc(>->user_wakeref); + intel_gt_pm_get(gt); + if (GRAPHICS_VER(gt->i915) >= 6) + intel_uncore_forcewake_user_get(gt->uncore); + + return 0; +} + +static int forcewake_user_release(struct inode *inode, struct file *file) +{ + struct intel_gt *gt = inode->i_private; + + if (GRAPHICS_VER(gt->i915) >= 6) + intel_uncore_forcewake_user_put(gt->uncore); + intel_gt_pm_put(gt); + atomic_dec(>->user_wakeref); + + return 0; +} + +static const struct file_operations forcewake_user_fops = { + .owner = THIS_MODULE, + .open = forcewake_user_open, + .release = forcewake_user_release, +}; + static int fw_domains_show(struct seq_file *m, void *data) { struct intel_gt *gt = m->private; @@ -627,6 +657,7 @@ void intel_gt_pm_debugfs_register(struct intel_gt *gt, struct dentry *root) { "drpc", &drpc_fops, NULL }, { "frequency", &frequency_fops, NULL }, { "forcewake", &fw_domains_fops, NULL }, + { "forcewake_user", &forcewake_user_fops, NULL}, { "llc", &llc_fops, llc_eval }, { "rps_boost", &rps_boost_fops, rps_eval }, };