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

[Andi Shyti]

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.

> +
>  	/* 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.

> +	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?

> +	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.

Thanks,
Andi

> +	if (err)
> +		return err;
> +
> +	return 0;
> +}