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

[Daniele Ceraolo Spurio]

On 3/5/20 10:02 AM, Andi Shyti wrote:
> Hi Daniele,
> 
>>> 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.
> 
> Have I seen it wrong or there are new files in this patch?

No, no new debugfs files, only the old ones moved across.

> In any case, maybe we need to have the new structure.
> 
>>>> +#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.
> 
> well that was supposed to be a library for all the gem/debugfs
> files and avoid duplicated code, I don't see anything wrong with
> including the file.
> 
>>>> +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?
> 
> I have a patch for it, can you please hold a little, unless, of
> course, yours is already ready.
> 

Sure, I'll wait for your patch to land first.

Daniele

> Obvously, the naming you propose makes sense.
> 
> Andi
>