Re: [v4] drm/xe: Add vram frequency sysfs attributes

["Sundaresan, Sujaritha" <sujaritha.sundaresan@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6027 bytes --]


On 12/14/2023 7:10 PM, Rodrigo Vivi wrote:
> On Thu, Dec 14, 2023 at 04:41:59PM +0530, Sujaritha Sundaresan wrote:
>> Add vram frequency sysfs attributes under the below hierarchy;
>>
>> /device/tile#/freq0
>> 		|-rp0_freq
>> 		|-rpn_freq
> hmm...
> you have a good point of not leaving an empty memory dir on platforms
> where we don't have the freq interfaces.
> But then we should only create the mem dir when we are going to create
> the freq.
>
> In the way this patch is proposing it looks like the frequency is about
> the tile. like a tile basedie frequency or something like that.
>
> So, we do need the memory or vram dir to make it really clear that
> the freq is about the memory.
>> v2: Drop "vram" from attribute names (Rodrigo)
>>
>> v3: Add documentation for new sysfs (Riana)
>>      Drop prefix from XEHP_PCODE_FREQUENCY_CONFIG (Riana)
>>
>> v4: Create sysfs under tile#/freq0 after removal of
>>      physical_memsize attrbute
>>
>> Signed-off-by: Sujaritha Sundaresan<sujaritha.sundaresan@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pcode_api.h  |  8 +++
>>   drivers/gpu/drm/xe/xe_tile_sysfs.c | 80 ++++++++++++++++++++++++++++++
>>   2 files changed, 88 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
>> index 5935cfe30204..26ceb8c0c010 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
>> @@ -42,6 +42,14 @@
>>   #define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed point format */
>>   #define	    POWER_SETUP_I1_DATA_MASK		REG_GENMASK(15, 0)
>>   
>> + #define   PCODE_FREQUENCY_CONFIG		0x6e
>> +/* PCODE_FREQUENCY_CONFIG sub-commands (param1) */
>> +#define     PCODE_MBOX_FC_SC_READ_FUSED_P0	0x0
>> +#define     PCODE_MBOX_FC_SC_READ_FUSED_PN	0x1
>> +/* PCODE_MBOX_DOMAIN_* - mailbox domain IDs */
>> +/* PCODE_FREQUENCY_CONFIG param2 */
>> +#define     PCODE_MBOX_DOMAIN_HBM		0x2
>> +
>>   struct pcode_err_decode {
>>   	int errno;
>>   	const char *str;
>> diff --git a/drivers/gpu/drm/xe/xe_tile_sysfs.c b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>> index 0f8d3e7fce46..45518442ed15 100644
>> --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>> +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>> @@ -7,9 +7,23 @@
>>   #include <linux/sysfs.h>
>>   #include <drm/drm_managed.h>
>>   
>> +#include "xe_gt_types.h"
>> +#include "xe_pcode.h"
>> +#include "xe_pcode_api.h"
>>   #include "xe_tile.h"
>>   #include "xe_tile_sysfs.h"
>>   
>> +#define GT_FREQUENCY_MULTIPLIER	50
> hmmm... it is kind of strange to define a frequency multiplier in
> the tile_sysfs component.
>
> This is probably an indication that we need a specific component for
> this memory freq?
>
> perhaps xe_vram_freq?

I would prefer not to over complicate for the sake of two attributes 
that appear only on PVC.

I can revert the path to /tile#/memory/freq0 and have all of it appear 
only for PVC.

Thoughts ?

>
>> +
>> +/**
>> + * DOC: Xe Tile sysfs
>> + *
>> + * Provides sysfs entries for memory related frequency in tile
>> + *
>> + * device/tile#/freq0/rp0_freq - The Render Performance 0 level, which is the maximum one.
>> + * device/tile#/freq0/rpn_freq - The Render Performance N level, which is the minimal one.
>> + */
>> +
>>   static void xe_tile_sysfs_kobj_release(struct kobject *kobj)
>>   {
>>   	kfree(kobj);
>> @@ -20,10 +34,68 @@ static const struct kobj_type xe_tile_sysfs_kobj_type = {
>>   	.sysfs_ops = &kobj_sysfs_ops,
>>   };
>>   
>> +static ssize_t rp0_freq_show(struct device *kdev, struct device_attribute *attr,
>> +			     char *buf)
>> +{
>> +	struct xe_tile *tile = kobj_to_tile(&kdev->kobj);
>> +	struct xe_gt *gt = tile->primary_gt;
>> +	u32 val, mbox;
>> +	int err;
>> +
>> +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, PCODE_FREQUENCY_CONFIG)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM1, PCODE_MBOX_FC_SC_READ_FUSED_P0)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM2, PCODE_MBOX_DOMAIN_HBM);
>> +
>> +	err = xe_pcode_read(gt, mbox, &val, NULL);
>> +	if (err)
>> +		return err;
>> +
>> +	/* data_out - Fused P0 for domain ID in units of 50 MHz */
>> +	val *= GT_FREQUENCY_MULTIPLIER;
>> +
>> +	return sysfs_emit(buf, "%u\n", val);
>> +}
>> +static DEVICE_ATTR_RO(rp0_freq);
>> +
>> +static ssize_t rpn_freq_show(struct device *kdev, struct device_attribute *attr,
>> +			     char *buf)
>> +{
>> +	struct xe_tile *tile = kobj_to_tile(&kdev->kobj);
>> +	struct xe_gt *gt = tile->primary_gt;
>> +	u32 val, mbox;
>> +	int err;
>> +
>> +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, PCODE_FREQUENCY_CONFIG)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM1, PCODE_MBOX_FC_SC_READ_FUSED_PN)
>> +		| REG_FIELD_PREP(PCODE_MB_PARAM2, PCODE_MBOX_DOMAIN_HBM);
>> +
>> +	err = xe_pcode_read(gt, mbox, &val, NULL);
>> +	if (err)
>> +		return err;
>> +
>> +	/* data_out - Fused Pn for domain ID in units of 50 MHz */
>> +	val *= GT_FREQUENCY_MULTIPLIER;
>> +
>> +	return sysfs_emit(buf, "%u\n", val);
>> +}
>> +static DEVICE_ATTR_RO(rpn_freq);
>> +
>> +static struct attribute *freq_attrs[] = {
>> +	&dev_attr_rp0_freq.attr,
>> +	&dev_attr_rpn_freq.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group freq_group_attrs = {
>> +	.name = "freq0",
>> +	.attrs = freq_attrs,
>> +};
>> +
>>   static void tile_sysfs_fini(struct drm_device *drm, void *arg)
>>   {
>>   	struct xe_tile *tile = arg;
>>   
>> +	sysfs_remove_group(tile->sysfs, &freq_group_attrs);
> if you didn't create the group you shouldn't remove it, so same
> conditions for create should apply here.
>
>>   	kobject_put(tile->sysfs);
>>   }
>>   
>> @@ -50,6 +122,14 @@ void xe_tile_sysfs_init(struct xe_tile *tile)
>>   
>>   	tile->sysfs = &kt->base;
>>   
>> +	if (xe->info.platform == XE_PVC) {
>> +		err = sysfs_create_group(tile->sysfs, &freq_group_attrs);
>> +		if (err) {
>> +			drm_warn(&xe->drm, "failed to register vram freq sysfs, err: %d\n", err);
>> +			return;
>> +		}
>> +	}
>> +
>>   	err = drmm_add_action_or_reset(&xe->drm, tile_sysfs_fini, tile);
>>   	if (err)
>>   		drm_warn(&xe->drm, "%s: drmm_add_action_or_reset failed, err: %d\n",
>> -- 
>> 2.25.1
>>

[-- Attachment #2: Type: text/html, Size: 7003 bytes --]