Re: [Intel-xe] [v2 2/2] drm/xe: Add vram frequency sysfs attributes

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

On 12/7/2023 11:32 AM, Riana Tauro wrote:
> Hi Suja
>
> On 12/6/2023 10:17 AM, Sujaritha Sundaresan wrote:
>> Add vram frequency sysfs attributes under the below hierarchy;
>>
>> /device/tile<n>/memory/freq
>>             |-rp0_freq
>>             |-rpn_freq
>>
>> v2: Drop "vram" from attribute names (Rodrigo)
>>
>> 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 | 72 ++++++++++++++++++++++++++++++
>>   2 files changed, 80 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h 
>> b/drivers/gpu/drm/xe/xe_pcode_api.h
>> index 5935cfe30204..edde5335bdb1 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   XEHP_PCODE_FREQUENCY_CONFIG 0x6e    /* xehp, pvc */
> Can we change it to PCODE_FREQUENCY_CONFIG ? there's no usage of 
> prefix in other places
Sure couldn't hurt to generalize it I guess.
>> +/* XEHP_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 */
>> +/* XEHP_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 e8ce4d9270e6..38d334833594 100644
>> --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>> +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>> @@ -7,9 +7,14 @@
>>   #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
> Could you add documentation to this file as there are multiple sysfs 
> added.
>
Sure will add.

>> +
>>   static void xe_tile_sysfs_kobj_release(struct kobject *kobj)
>>   {
>>       kfree(kobj);
>> @@ -35,6 +40,65 @@ static DEVICE_ATTR_RO(physical_vram_size_bytes);
>>   static const struct attribute *physical_memsize_attr =
>>       &dev_attr_physical_vram_size_bytes.attr;
>>   +static ssize_t rp0_freq_show(struct device *kdev, struct 
>> device_attribute *attr,
>> +                  char *buf)
>> +{
>> +    struct kobject *kobj = &kdev->kobj;
>> +    struct xe_tile *tile = kobj_to_tile(kobj->parent);
>> +    struct xe_gt *gt = tile->primary_gt;
>> +    u32 val, mbox;
>> +    int err;
>> +
>> +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, 
>> XEHP_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)
> alignment to the start of bracket
>
> Thanks
> Riana Tauro

Yeah I had already responded about this on one of the reviews from Anshuman.

Will fix.

Thanks,

Suja

>> +{
>> +    struct kobject *kobj = &kdev->kobj;
>> +    struct xe_tile *tile = kobj_to_tile(kobj->parent);
>> +    struct xe_gt *gt = tile->primary_gt;
>> +    u32 val, mbox;
>> +    int err;
>> +
>> +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, 
>> XEHP_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 = "freq",
>> +    .attrs = freq_attrs,
>> +};
>> +
>>   static void tile_sysfs_fini(struct drm_device *drm, void *arg)
>>   {
>>       struct xe_tile *tile = arg;
>> @@ -78,6 +142,14 @@ void xe_tile_sysfs_init(struct xe_tile *tile)
>>           drm_warn(&xe->drm,
>>                "Sysfs creation to read addr_range per tile failed\n");
>>   +    if (xe->info.platform == XE_PVC) {
>> +        err = sysfs_create_group(kobj, &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",