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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
