[Intel-gfx] [PATCH 5/6] drm/i915/uc: Move uC debugfs to its own folder under GT

[Daniele Ceraolo Spurio]

On 3/2/20 5:52 PM, Andi Shyti wrote:
> Hi Daniele,
> 
> I'm sorry I missed this patch,
> 
> On Thu, Feb 27, 2020 at 06:28:42PM -0800, Daniele Ceraolo Spurio wrote:
>> uC is a component of the GT, so it makes sense for the uC debugfs files
>> to be in the GT folder. A subfolder has been used to keep the same
>> structure we have for the code.
> 
> Can we please document the interface changes. I see there are
> some differences between the original and the new interfaces.
> 

What differences are you referring to? there aren't supposed to be any, 
aside from the path change.

>> +#define DEFINE_UC_DEBUGFS_ATTRIBUTE(__name)				\
>> +	static int __name ## _open(struct inode *inode, struct file *file) \
>> +{									\
>> +	return single_open(file, __name ## _show, inode->i_private);	\
>> +}									\
>> +static const struct file_operations __name ## _fops = {			\
>> +	.owner = THIS_MODULE,						\
>> +	.open = __name ## _open,					\
>> +	.read = seq_read,						\
>> +	.llseek = seq_lseek,						\
>> +	.release = single_release,					\
>> +}
> 
> Why do we need DEFINE_UC_DEBUGFS_ATTRIBUTE()?
> 
> DEFINE_GT_DEBUGFS_ATTRIBUTE() was meant to be common to all gt
> debugfs. I there any reason we need a new one?
> 

Just wanted to avoid including the other header just for this macro.

>> +struct debugfs_uc_file {
>> +	const char *name;
>> +	const struct file_operations *fops;
>> +};
>> +
>> +#define debugfs_uc_register_files(files__, root__, data__) \
>> +do { \
>> +	int i__ = 0; \
>> +	for (i__ = 0; i__ < ARRAY_SIZE(files__); i__++) { \
>> +		debugfs_create_file(files__[i__].name, \
>> +				    0444, root__, data__, \
>> +				    files__[i__].fops); \
>> +	} \
>> +} while (0)
> 
> You want to define your own debugfs_uc_register_files() instead
> of using debugfs_gt_register_files() because you want "data__"
> to be void, right?
> 
> I think we can achieve that by adding a wrapper in debugfs_gt.c,
> perhaps we can do something like:
> 
> void __debugfs_gt_register_files(struct intel_gt *gt,
>                                   struct dentry *root,
>                                   const struct debugfs_gt_file *files,
>                                   void *data,
>                                   unsigned long count)
> {
>        ......
> }
> 
> and
> 
> #define debugfs_gt_register_files(...) __debugfs_gt_register_files(...)
> #define debugfs_uc_register_files(...) __debugfs_gt_register_files(...)
> 
> so that we can keep everything in a library. What do you think.
> 

LGTM. Mind if I rename to:

intel_gt_debugfs_register(...)
intel_uc_debugfs_register(...)

to avoid the debugfs_* prefix, as pointed out by Jani?

Thanks,
Daniele

> Thanks,
> Andi
>