Re: [Intel-gfx] [PATCH dii-client 6/9] drm/i915/debugfs: Multiplex upper layer interfaces to act on all gt's

[Matt Roper <matthew.d.roper@intel.com>]

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
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation