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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 3/9/20 3:38 PM, 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 ?).

>> 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.

Daniele

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