Re: [PATCH 3/3] drm/xe/pf: Expose SR-IOV policy settings over debugfs

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 22.04.2024 21:39, Rodrigo Vivi wrote:
> On Thu, Apr 18, 2024 at 10:34:42PM +0200, Michal Wajdeczko wrote:
>> We already have functions to configure SR-IOV policies.
>> Allow to tweak those policy settings over debugfs.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c | 50 +++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
>> index 8909bb950a8b..3a83af7aa039 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
>> @@ -17,6 +17,7 @@
>>  #include "xe_gt_sriov_pf_control.h"
>>  #include "xe_gt_sriov_pf_debugfs.h"
>>  #include "xe_gt_sriov_pf_helpers.h"
>> +#include "xe_gt_sriov_pf_policy.h"
>>  #include "xe_pm.h"
>>  
>>  /*
>> @@ -76,6 +77,54 @@ static const struct drm_info_list pf_info[] = {
>>  	},
>>  };
>>  
>> +/*
>> + *      /sys/kernel/debug/dri/0/
>> + *      ├── gt0
>> + *      │   ├── pf
>> + *      │   │   ├── reset_engine
>> + *      │   │   ├── sample_period
>> + *      │   │   ├── sched_if_idle
>> + */
>> +
>> +#define DEFINE_SRIOV_GT_POLICY_DEBUGFS_ATTRIBUTE(POLICY, TYPE, FORMAT)		\
>> +										\
>> +static int POLICY##_set(void *data, u64 val)					\
>> +{										\
>> +	struct xe_gt *gt = extract_gt(data);					\
>> +	struct xe_device *xe = gt_to_xe(gt);					\
>> +	int err;								\
>> +										\
>> +	xe_pm_runtime_get(xe);							\
>> +	err = xe_gt_sriov_pf_policy_set_##POLICY(gt, val);			\
>> +	xe_pm_runtime_put(xe);							\
> 
> I like that this takes care of the runtime_pm in a 'global' way without duplication...
> 
>> +										\
>> +	return err;								\
>> +}										\
>> +										\
>> +static int POLICY##_get(void *data, u64 *val)					\
>> +{										\
>> +	struct xe_gt *gt = extract_gt(data);					\
>> +										\
>> +	*val = xe_gt_sriov_pf_policy_get_##POLICY(gt);				\
>> +	return 0;								\
>> +}										\
>> +										\
>> +DEFINE_DEBUGFS_ATTRIBUTE(POLICY##_fops, POLICY##_get, POLICY##_set, FORMAT)
>> +
>> +DEFINE_SRIOV_GT_POLICY_DEBUGFS_ATTRIBUTE(reset_engine, bool, "%llu\n");
>> +DEFINE_SRIOV_GT_POLICY_DEBUGFS_ATTRIBUTE(sched_if_idle, bool, "%llu\n");
>> +DEFINE_SRIOV_GT_POLICY_DEBUGFS_ATTRIBUTE(sample_period, u32, "%llu\n");
> 
> ... but, is there any other way we could accomplish this without this style
> that hides function... I hate when I have to understand anything in i915-guc
> code because you cannot navigate or grep for the functions that are declared
> with magic macros like this.

but such template macros are quite widely used (at least for sysfs)

$ cgrep ssize_t.*\\#.*show.*\\\\ | wc -l
189

and IMO it's better to have one template than replicate very similar
code N times with just one line difference

besides this are static functions, no need to grep anywhere else

> 
> but this is my only complain and no big suggestions. I would like to hear
> from the xe maintainers here.
> 
> for the debugfs entries and rpm handling and everything I like that.
> 
>> +
>> +static void pf_add_policy_attrs(struct xe_gt *gt, struct dentry *parent)
>> +{
>> +	xe_gt_assert(gt, gt == extract_gt(parent));
>> +	xe_gt_assert(gt, PFID == extract_vfid(parent));
>> +
>> +	debugfs_create_file_unsafe("reset_engine", 0644, parent, parent, &reset_engine_fops);
>> +	debugfs_create_file_unsafe("sched_if_idle", 0644, parent, parent, &sched_if_idle_fops);
>> +	debugfs_create_file_unsafe("sample_period_ms", 0644, parent, parent, &sample_period_fops);
>> +}
>> +
>>  /*
>>   *      /sys/kernel/debug/dri/0/
>>   *      ├── gt0
>> @@ -261,6 +310,7 @@ void xe_gt_sriov_pf_debugfs_register(struct xe_gt *gt, struct dentry *root)
>>  	pfdentry->d_inode->i_private = gt;
>>  
>>  	drm_debugfs_create_files(pf_info, ARRAY_SIZE(pf_info), pfdentry, minor);
>> +	pf_add_policy_attrs(gt, pfdentry);
>>  	pf_add_config_attrs(gt, pfdentry, PFID);
>>  
>>  	for (n = 1; n <= totalvfs; n++) {
>> -- 
>> 2.43.0
>>