| Message ID | 20230111235531.3353815-7-radhakrishna.sripada@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc Meteorlake patches | expand |
On Wed, Jan 11, 2023 at 03:55:28PM -0800, Radhakrishna Sripada wrote: > From: Andi Shyti <andi.shyti@linux.intel.com> > > Commit 82a149a62b6b5 ('drm/i915/gt: move remaining debugfs > interfaces into gt') moves gt related debugfs files in the gtX/ > directories to act on specific gt's individually. There's another thread somewhere on the list where these top-level entries are being discussed (maybe that one is sysfs rather than debugfs, but the idea is the same). Having entries in the per-gt directories and another copy outside the GT hierarchy is just confusing since the semantics aren't clear. It would be better if we just clean up the interface and stop exposing those top-level nodes on any platform that's still under force_probe protection. > > The original files are kept intact in the same location in order > to not break userspace users. But they were performing only on > the root tile (tile 0). When starting to work on a new platform we have the opportunity to change API's (even stuff that's ABI) and sunset legacy interfaces that no longer make sense as long as the platform isn't fully enabled with force_probe protection lifted. There's no requirement that userspace for platform n-1 must work without any changes on platform n. That's especially true for debugfs which isn't even considered ABI. > > Add a multiplexing functionality to the higher directories files > so that they can perform the operations on all the tiles in a > with a single write. This sounds unnecessary. > > In the read case they provide an or'ed value amongst the tiles. And this sounds like it could be harmful on a platform like MTL with heterogeneous GTs that are responsible for very different things. And if some debugfs interfaces return information other than booleans, using different semantics to combine the information from multiple GTs could lead to even more confusion and problems. I think it's much better to keep things simple, eliminate the ambiguous entries that are outside the gt directories, and keep things simple. Matt > > Cc: Maciej Patelczyk <maciej.patelczyk@intel.com> > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 38 ++++++++++++++++++++++++++--- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index a356ca490159..d64e9e3a439d 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -575,14 +575,36 @@ static int i915_wa_registers(struct seq_file *m, void *unused) > static int i915_wedged_get(void *data, u64 *val) > { > struct drm_i915_private *i915 = data; > + struct intel_gt *gt; > + unsigned int i; > > - return intel_gt_debugfs_reset_show(to_gt(i915), val); > + *val = 0; > + > + for_each_gt(gt, i915, i) { > + int ret; > + u64 v; > + > + ret = intel_gt_debugfs_reset_show(gt, &v); > + if (ret) > + return ret; > + > + /* at least one tile should be wedged */ > + *val |= !!v; > + if (*val) > + break; > + } > + > + return 0; > } > > static int i915_wedged_set(void *data, u64 val) > { > struct drm_i915_private *i915 = data; > - intel_gt_debugfs_reset_store(to_gt(i915), val); > + struct intel_gt *gt; > + unsigned int i; > + > + for_each_gt(gt, i915, i) > + intel_gt_debugfs_reset_store(gt, val); > > return 0; > } > @@ -732,7 +754,11 @@ static int i915_sseu_status(struct seq_file *m, void *unused) > static int i915_forcewake_open(struct inode *inode, struct file *file) > { > struct drm_i915_private *i915 = inode->i_private; > - intel_gt_pm_debugfs_forcewake_user_open(to_gt(i915)); > + struct intel_gt *gt; > + unsigned int i; > + > + for_each_gt(gt, i915, i) > + intel_gt_pm_debugfs_forcewake_user_open(gt); > > return 0; > } > @@ -740,7 +766,11 @@ static int i915_forcewake_open(struct inode *inode, struct file *file) > static int i915_forcewake_release(struct inode *inode, struct file *file) > { > struct drm_i915_private *i915 = inode->i_private; > - intel_gt_pm_debugfs_forcewake_user_release(to_gt(i915)); > + struct intel_gt *gt; > + unsigned int i; > + > + for_each_gt(gt, i915, i) > + intel_gt_pm_debugfs_forcewake_user_release(gt); > > return 0; > } > -- > 2.34.1 >
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index a356ca490159..d64e9e3a439d 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -575,14 +575,36 @@ static int i915_wa_registers(struct seq_file *m, void *unused) static int i915_wedged_get(void *data, u64 *val) { struct drm_i915_private *i915 = data; + struct intel_gt *gt; + unsigned int i; - return intel_gt_debugfs_reset_show(to_gt(i915), val); + *val = 0; + + for_each_gt(gt, i915, i) { + int ret; + u64 v; + + ret = intel_gt_debugfs_reset_show(gt, &v); + if (ret) + return ret; + + /* at least one tile should be wedged */ + *val |= !!v; + if (*val) + break; + } + + return 0; } static int i915_wedged_set(void *data, u64 val) { struct drm_i915_private *i915 = data; - intel_gt_debugfs_reset_store(to_gt(i915), val); + struct intel_gt *gt; + unsigned int i; + + for_each_gt(gt, i915, i) + intel_gt_debugfs_reset_store(gt, val); return 0; } @@ -732,7 +754,11 @@ static int i915_sseu_status(struct seq_file *m, void *unused) static int i915_forcewake_open(struct inode *inode, struct file *file) { struct drm_i915_private *i915 = inode->i_private; - intel_gt_pm_debugfs_forcewake_user_open(to_gt(i915)); + struct intel_gt *gt; + unsigned int i; + + for_each_gt(gt, i915, i) + intel_gt_pm_debugfs_forcewake_user_open(gt); return 0; } @@ -740,7 +766,11 @@ static int i915_forcewake_open(struct inode *inode, struct file *file) static int i915_forcewake_release(struct inode *inode, struct file *file) { struct drm_i915_private *i915 = inode->i_private; - intel_gt_pm_debugfs_forcewake_user_release(to_gt(i915)); + struct intel_gt *gt; + unsigned int i; + + for_each_gt(gt, i915, i) + intel_gt_pm_debugfs_forcewake_user_release(gt); return 0; }