Re: [Intel-gfx] [PATCH v4] drm/i915/gt: allow setting generic data pointer

[Andi Shyti <andi@etezian.org>]

Hi Daniele,

> > > > > Quoting Andi Shyti (2020-03-06 23:03:44)
> > > > > > -void debugfs_gt_register_files(struct intel_gt *gt,
> > > > > > -                              struct dentry *root,
> > > > > > -                              const struct debugfs_gt_file *files,
> > > > > > -                              unsigned long count)
> > > > > > +void intel_gt_debugfs_register_files(struct dentry *root,
> > > > > > +                                    const struct debugfs_gt_file *files,
> > > > > > +                                    unsigned long count, void *data)
> > > > > >   {
> > > > > >          while (count--) {
> > > > > > -               if (!files->eval || files->eval(gt))
> > > > > > +               if (!files->eval || files->eval(data))
> > > > > >                          debugfs_create_file(files->name,
> > > > > > -                                           0444, root, gt,
> > > > > > +                                           0444, root, data,
> > > > > >                                              files->fops);
> > > > > 
> > > > > And now we are not a intel_gt routine, you'll want to move again :)
> > > > > i915_debugfs_utils.c ? :)
> > > > 
> > > > Actually, this is what it came to and this was the first
> > > > discussion I had with Daniele and that's also why I was loyal to
> > > > th "_gt_" wrappers until the end. But I had to agree that this
> > > > was becoming more a limitation.
> > > > 
> > > > The biggest difference left, which by the way is the real
> > > > distinguishing factor other than the *gt pointer, is that we
> > > > create files under gt directory, instead of having the root
> > > > imposed by the drm (even though the caller can eventually choose
> > > > different roots).
> > > > 
> > > > We could perhaps store the root pointer in the intel_gt
> > > > structure so that this function stays de facto an intel_gt
> > > > routine and the caller doesn't need to care where the files will
> > > > be generated. This is what we planned to do with sysfs as well.
> > > > 
> > > > What do you think?
> > > 
> > > I thought we were passing along the root. If not I think we should, more
> > > of a debugfs constructor context?
> > 
> > What do you mean with debugfs constructor context? Is it a
> > gt->debugfs_root pointer like the gt->sysfs_root?
> > 

> Getting the root pointer internally from gt wouldn't work well for
> subfolders, like the gt/uc/ folder I want to add for GuC/HuC files.

this was not my idea, actually I was thinking the opposite.

When in this case you call "intel_gt_debugfs_register_files", you
would provide "gt" pointer where the funcion extracts and handles
by its own the debugfs_root. The caller doesn't need to care
about it.

Another idea could be to use contexts, e.g. guc or pm or whatever
comes to mind, and the intel_gt_debugfs handles everything
including subdirectories.

> I think extracting this generic helper to a common file, possibly as a follow-up
> step, isn't a bad idea, also considering that there is at least 1 more
> use-case in i915_debugfs_register(). Maybe we can generalize as something
> like:
> 
> struct i915_debugfs_files {
> 	const char *name;
> 	const struct file_operations *fops;
> 	bool (*eval)(void *data);
> }
> 
> void i915_debugfs_register_files(struct dentry *root,
> 				 const struct i915_debugfs_files *files,
> 				 unsigned long count, void *data)
> {
>  	while (count--) {
> 		umode_t mode = files->fops->write ? 0644 : 0444;
> 		if (!files->eval || files->eval(data))
>  			debugfs_create_file(files->name,
> 					    mode, root, data,
>  					    files->fops);
> 	}
> }

apart from the mode, isn't this the same as the latest patch you
actually reviewed?

> void i915_debugfs_register_files(struct dentry *root,

based on my proposal, root would point, in your case, to the
"guc/" directory that will be created under the "gt/". NULL if
you want the file to be created in the main "gt/" directory.

While if we want to go by context, we could do something like:

struct i915_debugfs_files {                                    
      const char *name;                                        
      const struct file_operations *fops;                      
      bool (*eval)(void *data);                                
      enum intel_gt_context context;
}

and the gt handles everything.

Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx