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

[Daniele Ceraolo Spurio]

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
>