[PATCH] RFC drm/xe/xe_gt_idle: Expose Coarse power gating sysfs entries

Riana Tauro riana.tauro at intel.com
Fri Jul 19 05:46:05 UTC 2024


Hi Matt

Thank you for the review comments.

On 7/18/2024 12:53 AM, Matt Roper wrote:
> On Mon, Jul 15, 2024 at 03:24:55PM +0530, Riana Tauro wrote:
>> Add coarse power gating sysfs interface under gtidle directory to
>> display the powergate properties.
> 
> You might also want to explain what "powergate" really means here.
> I.e., how does this powergating differ from what happens with RC6/MC6?
> When can powergating kick in?
Will add these details in the commit message.
> 
>> powergate sysfs directory will be enumerated for render and media
>> if present.
>>
>> The new entries will have the below layout
>>
>> 	tile<n>/gt<n>/gtidle/
>> 			├── powergate0
>> 			│   ├── enabled
>> 			│   ├── status
>> 			│   └── type
>> 			└── powergate<n>
>> 			    ├── enabled
>> 			    ├── status
>> 		    	    └── type
>>
>> type:    type of powergate [Render or Media]
> 
> What userspace software is expected to be reading this interface and
> what will it be doing with the data?

IGT test is planned for this. Also this is required for debug purposes.

> I'm surprised that they want just
> a single "media" powergate rather than providing the actual powergates
> that the hardware supports (e.g., one entry per individual media slice,
> which would allow userspace to either consolidate them or split them out
> as they see fit). 

i915 actually displays this info in debugfs and does it only for a 
single media slice (ie slice0).

Since there would be multiple directories, combined it as one in this 
RFC. Will split it per media slice


> If we just expose whatever the underlying hardware
> has (which may vary by platform), then userspace is free to consolidate
> some of the values itself if it doesn't need the lower level of detail.
> 
>> enabled: true if powergate is enabled for the type
>> status:  Indicates status of powergate [Up/Down]
>> 	 For type media, status is "Up" if any of the media
>> 	 slices are powered on.
>>
>> Signed-off-by: Riana Tauro <riana.tauro at intel.com>
>> ---
>>   drivers/gpu/drm/xe/regs/xe_gt_regs.h  |   7 +
>>   drivers/gpu/drm/xe/xe_gt_idle.c       | 214 +++++++++++++++++++++++++-
>>   drivers/gpu/drm/xe/xe_gt_idle_types.h |  17 ++
>>   3 files changed, 237 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> index 8a94a94d2267..8586c830bea6 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> @@ -335,6 +335,13 @@
>>   #define   CTC_SHIFT_PARAMETER_MASK		REG_GENMASK(2, 1)
>>   #define   CTC_SOURCE_DIVIDE_LOGIC		REG_BIT(0)
>>   
>> +#define POWERGATE_DOMAIN_STATUS			XE_REG(0xa2a0)
>> +#define   MEDIA_SLICE0_AWAKE_STATUS		REG_BIT(0)
>> +#define   RENDER_AWAKE_STATUS			REG_BIT(1)
>> +#define   MEDIA_SLICE1_AWAKE_STATUS		REG_BIT(2)
>> +#define   MEDIA_SLICE2_AWAKE_STATUS		REG_BIT(3)
>> +#define   MEDIA_SLICE3_AWAKE_STATUS		REG_BIT(4)
> 
> Nitpick:  bits are usually defined in descending order instead of
> ascending.
> 
will fix this
>> +
>>   #define FORCEWAKE_RENDER			XE_REG(0xa278)
>>   #define FORCEWAKE_MEDIA_VDBOX(n)		XE_REG(0xa540 + (n) * 4)
>>   #define FORCEWAKE_MEDIA_VEBOX(n)		XE_REG(0xa560 + (n) * 4)
>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle.c b/drivers/gpu/drm/xe/xe_gt_idle.c
>> index 67aba4140510..fca13e645d24 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_idle.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_idle.c
>> @@ -25,13 +25,26 @@
>>    * device/gt#/gtidle/name - name of the state
>>    * device/gt#/gtidle/idle_residency_ms - Provides residency of the idle state in ms
>>    * device/gt#/gtidle/idle_status - Provides current idle state
>> + * device/gt#/gtidle/powergate<n>/
>> + *                              ├── enabled - indicates whether powergate is enabled
>> + *                              ├── status - status of powergate [Up / Down]
>> + *                              └── type - type of powergate
>>    */
>> +static struct xe_gt_idle *kobj_to_gtidle(struct kobject *kobj)
>> +{
>> +	return &kobj_to_gt(kobj->parent)->gtidle;
>> +}
>>   
>>   static struct xe_gt_idle *dev_to_gtidle(struct device *dev)
>>   {
>>   	struct kobject *kobj = &dev->kobj;
>>   
>> -	return &kobj_to_gt(kobj->parent)->gtidle;
>> +	return kobj_to_gtidle(kobj);
>> +}
>> +
>> +static struct xe_gt_powergate *kobj_to_powergate(struct kobject *kobj)
>> +{
>> +	return container_of(kobj, struct xe_gt_powergate, kobj);
>>   }
>>   
>>   static struct xe_gt *gtidle_to_gt(struct xe_gt_idle *gtidle)
>> @@ -129,6 +142,7 @@ void xe_gt_idle_enable_pg(struct xe_gt *gt)
>>   	}
>>   
>>   	xe_mmio_write32(gt, POWERGATE_ENABLE, pg_enable);
>> +
>>   	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
> 
> Unexpected whitespace change?
yeah missed this. will fix this
> 
>>   }
>>   
>> @@ -145,6 +159,70 @@ void xe_gt_idle_disable_pg(struct xe_gt *gt)
>>   	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>>   }
>>   
>> +static void gt_idle_powergate_enabled(struct xe_gt_idle *gtidle,
>> +				      struct xe_gt_powergate *pg)
>> +{
>> +	struct xe_gt *gt = gtidle_to_gt(gtidle);
>> +	u32 enabled;
>> +
>> +	XE_WARN_ON(xe_force_wake_get(gt_to_fw(gt), XE_FW_GT));
>> +	enabled = xe_mmio_read32(gt, POWERGATE_ENABLE);
>> +	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>> +
>> +	if (pg->is_media)
>> +		enabled &= MEDIA_POWERGATE_ENABLE;
>> +	else
>> +		enabled &= RENDER_POWERGATE_ENABLE;
>> +
>> +	pg->enabled = !!enabled;
>> +}
>> +
>> +static void gt_idle_powergate_status(struct xe_gt_idle *gtidle,
>> +				     struct xe_gt_powergate *pg)
>> +{
>> +	struct xe_gt *gt = gtidle_to_gt(gtidle);
>> +	u32 status, reg;
>> +
>> +	XE_WARN_ON(xe_force_wake_get(gt_to_fw(gt), XE_FW_GT));
>> +	reg = xe_mmio_read32(gt, POWERGATE_DOMAIN_STATUS);
>> +	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>> +
>> +	if (pg->is_media) {
>> +		/*
>> +		 * Media Slices
>> +		 *
>> +		 * Slice 0: VCS0, VCS1, VECS0
>> +		 * Slice 1: VCS2, VCS3, VECS1
>> +		 * Slice 2: VCS4, VCS5, VECS2
>> +		 * Slice 3: VCS6, VCS7, VECS3
>> +		 */
>> +		if (gt->info.engine_mask & (BIT(XE_HW_ENGINE_VCS0)
>> +					    | BIT(XE_HW_ENGINE_VCS1)
>> +					    | BIT(XE_HW_ENGINE_VECS0)))
> 
> If this is false...

Won't media slice 0 be always present if media is available?

> 
>> +			status = (reg & MEDIA_SLICE0_AWAKE_STATUS);
>> +
>> +		if (gt->info.engine_mask & (BIT(XE_HW_ENGINE_VCS2)
>> +					    | BIT(XE_HW_ENGINE_VCS3)
>> +					    | BIT(XE_HW_ENGINE_VECS1)))
> 
> ...and this is true...
> 
>> +			status |= (reg & MEDIA_SLICE1_AWAKE_STATUS);
> 
> Then status still has an undefined value here.  Ditto for the other two
> conditions below.
> 
>> +
>> +		if (gt->info.engine_mask & (BIT(XE_HW_ENGINE_VCS4)
>> +					    | BIT(XE_HW_ENGINE_VCS5)
>> +					    | BIT(XE_HW_ENGINE_VECS2)))
>> +			status |= (reg & MEDIA_SLICE2_AWAKE_STATUS);
>> +
>> +		if (gt->info.engine_mask & (BIT(XE_HW_ENGINE_VCS6)
>> +					    | BIT(XE_HW_ENGINE_VCS7)
>> +					    | BIT(XE_HW_ENGINE_VECS3)))
>> +			status |= (reg & MEDIA_SLICE3_AWAKE_STATUS);
>> +
>> +	} else {
>> +		status = reg & RENDER_AWAKE_STATUS;
>> +	}
>> +
>> +	pg->status = !!status;
>> +}
>> +
>>   static ssize_t name_show(struct device *dev,
>>   			 struct device_attribute *attr, char *buff)
>>   {
>> @@ -197,10 +275,130 @@ static const struct attribute *gt_idle_attrs[] = {
>>   	NULL,
>>   };
>>   
>> +static ssize_t status_show(struct device *dev,
>> +			   struct device_attribute *attr, char *buff)
>> +{
>> +	struct kobject *kobj = &dev->kobj;
>> +	struct xe_gt_powergate *pg = kobj_to_powergate(kobj);
>> +	struct xe_gt_idle *gtidle = kobj_to_gtidle(kobj->parent);
>> +	struct xe_gt *gt = gtidle_to_gt(gtidle);
>> +
>> +	xe_pm_runtime_get(gt_to_xe(gt));
>> +	gt_idle_powergate_status(gtidle, pg);
>> +	xe_pm_runtime_put(gt_to_xe(gt));
>> +
>> +	return sysfs_emit(buff, "%s\n", pg->status ? "Up" : "Down");
>> +}
>> +static DEVICE_ATTR_RO(status);
>> +
>> +static ssize_t enabled_show(struct device *dev,
>> +			    struct device_attribute *attr, char *buff)
>> +{
>> +	struct kobject *kobj = &dev->kobj;
>> +	struct xe_gt_powergate *pg = kobj_to_powergate(kobj);
>> +	struct xe_gt_idle *gtidle = kobj_to_gtidle(kobj->parent);
>> +	struct xe_gt *gt = gtidle_to_gt(gtidle);
>> +
>> +	xe_pm_runtime_get(gt_to_xe(gt));
>> +	gt_idle_powergate_enabled(gtidle, pg);
>> +	xe_pm_runtime_put(gt_to_xe(gt));
>> +
>> +	return sysfs_emit(buff, "%u\n", pg->enabled);
>> +}
>> +static DEVICE_ATTR_RO(enabled);
>> +
>> +static ssize_t type_show(struct device *dev,
>> +			 struct device_attribute *attr, char *buff)
>> +{
>> +	struct xe_gt_powergate *pg = kobj_to_powergate(&dev->kobj);
>> +
>> +	return sysfs_emit(buff, "%s\n", pg->is_media ? "Media" : "Render");
>> +}
>> +
>> +static DEVICE_ATTR_RO(type);
>> +
>> +static const struct attribute *powergate_attrs[] = {
>> +	&dev_attr_type.attr,
>> +	&dev_attr_enabled.attr,
>> +	&dev_attr_status.attr,
>> +	NULL,
>> +};
>> +
>> +static void powergate_sysfs_release(struct kobject *kobj) {};
>> +
>> +static const struct kobj_type powergate_sysfs_kobj_type = {
>> +	.release = powergate_sysfs_release,
>> +	.sysfs_ops = &kobj_sysfs_ops,
>> +};
>> +
>> +static int register_powergate_sysfs(struct kobject *gtidle_kobj, int idx,
>> +				    bool is_media)
>> +{
>> +	struct xe_gt *gt = kobj_to_gt(gtidle_kobj->parent);
>> +	struct xe_device *xe = gt_to_xe(gt);
>> +	struct xe_gt_idle *gtidle = &gt->gtidle;
>> +	struct xe_gt_powergate *pg = &gtidle->powergate[idx];
>> +	int ret;
>> +
>> +	kobject_init(&pg->kobj, &powergate_sysfs_kobj_type);
>> +
>> +	ret = kobject_add(&pg->kobj, gtidle_kobj, "powergate%d", idx);
>> +
>> +	if (ret) {
>> +		kobject_put(&pg->kobj);
>> +		drm_warn(&xe->drm, "Failed to create powergate%d sysfs directory\n", idx);
>> +		return ret;
>> +	}
>> +
>> +	ret = sysfs_create_files(&pg->kobj, powergate_attrs);
>> +	if (ret) {
>> +		drm_warn(&xe->drm, "Failed to create powergate%d attrs\n", idx);
>> +		kobject_put(&pg->kobj);
>> +		return ret;
>> +	}
>> +
>> +	pg->is_media = is_media;
>> +
>> +	return ret;
>> +}
>> +
>> +static int gt_idle_create_pg_attrs(struct kobject *gtidle_kobj)
>> +{
>> +	struct xe_gt *gt = kobj_to_gt(gtidle_kobj->parent);
>> +	struct xe_device *xe  = gt_to_xe(gt);
>> +	int ret, idx = 0;
>> +
>> +	/* CPG is disabled for PVC */
>> +	if (xe->info.platform == XE_PVC)
>> +		return 0;
>> +
>> +	if (!xe_gt_is_media_type(gt)) {
>> +		ret = register_powergate_sysfs(gtidle_kobj, idx, false);
>> +		if (!ret)
>> +			idx++;
>> +	}
>> +
>> +	/*
>> +	 * Do not create media pg status if the entire first media slice
>> +	 * is absent (i.e., no VCS0 or VECS0).
> 
> Is this an attempt to work around the bug above?  Otherwise I don't see
> why we'd want to tie this to media slice 0 specifically and not report
> powergating when only the other slice(s) are available.Won't media slice 0 be always present if media is available? I saw some 
code in xe checking the presence of slice0 to check if media is available

Thanks,
Riana
> 
> 
> Matt
> 
>> +	 */
>> +	if (gt->info.engine_mask & (BIT(XE_HW_ENGINE_VCS0) | BIT(XE_HW_ENGINE_VECS0))) {
>> +		ret = register_powergate_sysfs(gtidle_kobj, idx, true);
>> +		if (!ret)
>> +			idx++;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static void gt_idle_fini(void *arg)
>>   {
>>   	struct kobject *kobj = arg;
>>   	struct xe_gt *gt = kobj_to_gt(kobj->parent);
>> +	struct xe_gt_idle *gtidle = &gt->gtidle;
>> +	struct xe_device *xe  = gt_to_xe(gt);
>> +	struct xe_gt_powergate *pg;
>> +	int n;
>>   
>>   	xe_gt_idle_disable_pg(gt);
>>   
>> @@ -210,6 +408,18 @@ static void gt_idle_fini(void *arg)
>>   		xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
>>   	}
>>   
>> +	for (n = 0 ; n < 2; n++) {
>> +		if (xe->info.platform == XE_PVC)
>> +			break;
>> +
>> +		pg = &gtidle->powergate[n];
>> +
>> +		if (pg->kobj.state_initialized) {
>> +			sysfs_remove_files(&pg->kobj, powergate_attrs);
>> +			kobject_put(&pg->kobj);
>> +		}
>> +	}
>> +
>>   	sysfs_remove_files(kobj, gt_idle_attrs);
>>   	kobject_put(kobj);
>>   }
>> @@ -248,6 +458,8 @@ int xe_gt_idle_init(struct xe_gt_idle *gtidle)
>>   
>>   	xe_gt_idle_enable_pg(gt);
>>   
>> +	gt_idle_create_pg_attrs(kobj);
>> +
>>   	return devm_add_action_or_reset(xe->drm.dev, gt_idle_fini, kobj);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle_types.h b/drivers/gpu/drm/xe/xe_gt_idle_types.h
>> index f99b447534f3..603f7f6b509d 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_idle_types.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_idle_types.h
>> @@ -7,6 +7,7 @@
>>   #define _XE_GT_IDLE_SYSFS_TYPES_H_
>>   
>>   #include <linux/types.h>
>> +#include <linux/kobject.h>
>>   
>>   struct xe_guc_pc;
>>   
>> @@ -17,6 +18,20 @@ enum xe_gt_idle_state {
>>   	GT_IDLE_UNKNOWN,
>>   };
>>   
>> +/**
>> + * struct xe_gt_powergate - Powergate properties
>> + */
>> +struct xe_gt_powergate {
>> +	/** @enabled: powergate enable */
>> +	bool enabled;
>> +	/** @status: status of powerwell*/
>> +	bool status;
>> +	/** @is_media: indicates media powergate */
>> +	bool is_media;
>> +	/** @kobj: kobject for powergate sysfs */
>> +	struct kobject kobj;
>> +};
>> +
>>   /**
>>    * struct xe_gt_idle - A struct that contains idle properties based of gt
>>    */
>> @@ -29,6 +44,8 @@ struct xe_gt_idle {
>>   	u64 cur_residency;
>>   	/** @prev_residency: previous residency counter */
>>   	u64 prev_residency;
>> +	/** @powergate: powergate properties */
>> +	struct xe_gt_powergate powergate[2];
>>   	/** @idle_status: get the current idle state */
>>   	enum xe_gt_idle_state (*idle_status)(struct xe_guc_pc *pc);
>>   	/** @idle_residency: get idle residency counter */
>> -- 
>> 2.40.0
>>
> 


More information about the Intel-xe mailing list