On 3/11/20 1:24 AM, Andi Shyti wrote:
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?
Yes, but by adding the mode and making the naming generic we can re-use it
outside the GT code, e.g. in i915_debugfs_connector_add() and to replace the
loop in i915_debugfs_register(). I was reconnecting to Chris' proposal of
having a common function in i915_debugfs_utils.c (or even just in
i915_debugfs.c ?).
that's where we are coming from, we just moved this in :)
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.
If I'm understanding correctly, you're proposing to pass both struct
intel_gt *gt and struct dentry *root, with the latter being set only if we
want a folder different that gt/ ? What would that gain us compared to just
passing the desired root every time like we currently do?
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;
This seems overkill, also because you'd have to save all the roots inside of
the gt struct to allow accessing them from within the register_files
function.
}
and the gt handles everything.
Maybe I'm misunderstanding your proposal, but it feels like you're trying to
find a use for a gt variable we don't really need just to keep this as a gt
routine.
I'm just thinking aloud here also to avoid moving things back and
forth so easily.
As I've mentioned above I wasn't suggesting moving this around now, just
pointing out that there are other parts of the driver that open-code
what we do in the gt and therefore there is space for code re-use in the
future. I'm ok with merging this as-is in the meantime.
I don't want to overdo things, but the way I see it, and the way
it was thought originally, is to have a hierarchical organization
of the debugfs, so taht "uc" and other gt systems cannot create
files above gt.
But this hierarchy is currently implemented by passing the correct root
pointer in and not via the gt pointer, so from the function POV you
nothing stops you from passing in a root above gt/. The only thing the
gt pointer is used for are data and eval().
This patch is not just becoming gt independent, but it's i915
independent and, at this point, we can send it directly under
inode.c.
Fair point :)
I'm going to send my patches rebased on top of this one to better show
what I mean when I say I don't use the gt variable locally at all. If we
decide we still want it I'll throw in a guc/huc_to_gt to take it out.
Daniele
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
