[Intel-xe] [PATCH 1/2] drm/xe: add a new sysfs directory for gtidle properties

[Riana Tauro]

Hi Andi

Thanks for the review

On 6/14/2023 3:32 PM, Andi Shyti wrote:
> Hi Riana,
> 
> Thanks for Cc'ing me to this patch, I have a few questions.
> 
> [...]
> 
>> @@ -306,6 +307,10 @@ static int gt_fw_domain_init(struct xe_gt *gt)
>>   	if (err)
>>   		goto err_force_wake;
>>   
>> +	err = xe_gt_idle_sysfs_init(&gt->gtidle);
>> +	if (err)
>> +		goto err_force_wake;
> 
> do we really want to fail here? The driver would still work
> without sysfs.
> 
> In my opinion this should be a warning.
Will make the function void and add warnings in the function
> 
>> +
>>   	/* XXX: Fake that we pull the engine mask from hwconfig blob */
>>   	gt->info.engine_mask = gt->info.__engine_mask;
> 
> [...]
> 
>> +static u64 get_residency_ms(struct xe_gt_idle *gtidle, u64 cur_residency)
>> +{
>> +	u64 delta, overflow_residency, prev_residency;
>> +
>> +	overflow_residency = BIT_ULL(32);
>> +
>> +	/*
>> +	 * Counter wrap handling
>> +	 * Store previous hw counter values for counter wrap-around handling
>> +	 * Relying on sufficient frequency of queries otherwise counters can still wrap.
>> +	 */
>> +	prev_residency = gtidle->prev_residency;
>> +	gtidle->prev_residency = cur_residency;
>> +
>> +	/* delta */
>> +	if (cur_residency >= prev_residency)
>> +		delta = cur_residency - prev_residency;
>> +	else
>> +		delta = cur_residency + (overflow_residency - prev_residency);
>> +
>> +	/* Add delta to extended raw driver copy of idle residency */
>> +	cur_residency = gtidle->cur_residency + delta;
>> +	gtidle->cur_residency = cur_residency;
>> +
>> +	/* residency multiplier in ns, convert to ms */
>> +	cur_residency = mul_u64_u32_div(cur_residency, gtidle->residency_multiplier, 1e6);
> 
> do we need to lock something here? Looks a bit racy to me.
seq_read_iter takes a lock while calling sysfs_show
https://elixir.bootlin.com/linux/latest/source/fs/seq_file.c#L171

This function is used only by idle_residency_show. So didn't add the lock
> 
>> +	return cur_residency;
>> +}
> 
> [...]
> 
>> +static void gt_idle_sysfs_fini(struct drm_device *drm, void *arg)
>> +{
>> +	struct kobject *kobj = arg;
>> +
>> +	sysfs_remove_files(kobj, gt_idle_attrs);
>> +	kobject_put(kobj);
>> +}
>> +
>> +int xe_gt_idle_sysfs_init(struct xe_gt_idle *gtidle)
>> +{
>> +	struct xe_gt *gt = gtidle_to_gt(gtidle);
>> +	struct xe_device *xe = gt_to_xe(gt);
>> +	struct kobject *kobj;
>> +	int err;
>> +
>> +	kobj = kobject_create_and_add("gtidle", gt->sysfs);
>> +	if (!kobj)
>> +		return -ENOMEM;
> 
> I think we shouldn't fail here... although enomem is quite a
> severe error, but still I think i915 can still work without
> sysfs.
> 
>> +	sprintf(gtidle->name, "gt%d-rc\n", gt->info.id);
>> +	/* Multiplier for  RC6 Residency counter in units of 1.28us */
>> +	gtidle->residency_multiplier = 1280;
>> +	gtidle->idle_residency = xe_guc_pc_rc6_residency;
>> +	gtidle->idle_status = xe_guc_pc_rc_status;
> 
> why are we saving "xe_guc_pc_rc_status" and
> "xe_guc_pc_rc6_residency" inside gtidle? What are the
> alternatives? Why can't we call them directly?

 From previous review comments, function pointers were suggested to 
abstract low level guc_pc calls and make the gtidle a control and report
infra

https://patchwork.freedesktop.org/series/117222/#rev1
> 
>> +	err = sysfs_create_files(kobj, gt_idle_attrs);
>> +	if (err) {
>> +		kobject_put(kobj);
>> +		return err;
>> +	}
>> +
>> +	err = drmm_add_action_or_reset(&xe->drm, gt_idle_sysfs_fini, kobj);
> 
> Just
> 
> 	return drmm_add_action_or_reset(&xe->drm, gt_idle_sysfs_fini, kobj);
> 
> In any case I would make this function "void" and print warnings
> in case of failures.

Will fix all the above review comments.

Thanks
Riana
> 
> Thanks,
> Andi
> 
>> +	if (err)
>> +		return err;
>> +
>> +	return 0;
>> +}