On 3/7/20 2:19 PM, Andi Shyti wrote:
Hi Chris,
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. 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);
}
}
Daniele
The main thing of course is not to overengineer and do the minimal
necessary for the immediate users we have. We can always extend and
refactor for a third user, etc, etc.
So if this works for gt + children, go for it and worry about tomorrow,
tomorrow. Trusting our good practice for "a stitch in time saves nine".
this came after Daniele's guc patches where he preferred to
define his own functions instead of using this one that is meant
to be used in that situation.
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
