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

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

On 1/2/2024 4:19 PM, Riana Tauro wrote:
>
> Hi Suja
>
> On 1/2/2024 3:48 PM, Sundaresan, Sujaritha wrote:
>>
>> On 1/2/2024 3:39 PM, Gupta, Anshuman wrote:
>>>
>>>> -----Original Message-----
>>>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>>>> Sent: Tuesday, January 2, 2024 10:58 AM
>>>> To: intel-xe@lists.freedesktop.org
>>>> Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Sundaresan, Sujaritha
>>>> <sujaritha.sundaresan@intel.com>
>>>> Subject: [v7] drm/xe: Add vram frequency sysfs attributes
>>>>
>>>> Add vram frequency sysfs attributes under the below hierarchy;
>>>>
>>>> /device/tile#/memory/freq0
>>>>             |-max_freq
>>>>             |-min_freq
>>>>
>>>> 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
>>>>
>>>> v5: Revert back to creating sysfs under tile#/memory/freq0
>>>>      Remove definition of GT_FREQUENCY_MULTIPLIER (Rodrigo)
>>>>
>>>> v6: Rename attributes to max/min_freq (Anshuman)
>>>>      Fix review comments (Rodrigo)
>>>>
>>>> v7: Make docuemntation more verbose
> typo
>>>>      Move sysfs to separate file (Anshuman)
> Separate file will not be needed if drmm_add_action_or_reset is added
> similar to the file xe_hw_engine_class_sysfs.c
>>>>
>>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/xe/Makefile        |   1 +
>>>>   drivers/gpu/drm/xe/xe_pcode_api.h  |   7 ++
>>>>   drivers/gpu/drm/xe/xe_tile_sysfs.c |   3 +
>>>>   drivers/gpu/drm/xe/xe_vram_freq.c  | 127
>>>> +++++++++++++++++++++++++++++ drivers/gpu/drm/xe/xe_vram_freq.h
>>>> |  16 ++++
>>>>   5 files changed, 154 insertions(+)
>>>>   create mode 100644 drivers/gpu/drm/xe/xe_vram_freq.c create mode
>>>> 100644 drivers/gpu/drm/xe/xe_vram_freq.h
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/Makefile 
>>>> b/drivers/gpu/drm/xe/Makefile index
>>>> df8601d6a59f..17884e422cec 100644
>>>> --- a/drivers/gpu/drm/xe/Makefile
>>>> +++ b/drivers/gpu/drm/xe/Makefile
>>>> @@ -139,6 +139,7 @@ xe-y += xe_bb.o \
>>>>       xe_uc_debugfs.o \
>>>>       xe_uc_fw.o \
>>>>       xe_vm.o \
>>>> +    xe_vram_freq.o \
>>>>       xe_wait_user_fence.o \
>>>>       xe_wa.o \
>>>>       xe_wopcm.o
>>>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h
>>>> b/drivers/gpu/drm/xe/xe_pcode_api.h
>>>> index 5935cfe30204..f153ce96f69a 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
>>>> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
>>>> @@ -42,6 +42,13 @@
>>>>   #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
>>>> +/* Frequency Config Sub Commands (param1) */
>>>> +#define     PCODE_MBOX_FC_SC_READ_FUSED_P0    0x0
>>>> +#define     PCODE_MBOX_FC_SC_READ_FUSED_PN    0x1
>>>> +/* Domain IDs (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..ed60d12f5cf0 100644
>>>> --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>>> +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>>> @@ -9,6 +9,7 @@
>>>>
>>>>   #include "xe_tile.h"
>>>>   #include "xe_tile_sysfs.h"
>>>> +#include "xe_vram_freq.h"
>>>>
>>>>   static void xe_tile_sysfs_kobj_release(struct kobject *kobj)  { 
>>>> @@ -50,6 +51,8
>>>> @@ void xe_tile_sysfs_init(struct xe_tile *tile)
>>>>
>>>>       tile->sysfs = &kt->base;
>>>>
>>>> +    xe_vram_freq_init(tile);
>>>> +
>>>>       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", diff --git a/drivers/gpu/drm/xe/xe_vram_freq.c
>>>> b/drivers/gpu/drm/xe/xe_vram_freq.c
>>>> new file mode 100644
>>>> index 000000000000..166d41a6b222
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/xe/xe_vram_freq.c
>>>> @@ -0,0 +1,127 @@
>>>> +// SPDX-License-Identifier: MIT
>>>> +/*
>>>> + * Copyright © 2023 Intel Corporation >>> + */
>>>> +#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"
> unnecessary header
>>>> +#include "xe_vram_freq.h"
>>>> +
>>>> +/**
>>>> + * DOC: Xe VRAM freq
>>>> + *
>>>> + * Provides sysfs entries for vram frequency in tile
>>>> + *
>>>> + * device/tile#/memory/freq0/max_freq - This is maximum frequency. 
>>>> This
>>>> value is read-only as it
>>>> + *                    is the fixed fuse point P0. It is not the
>>>> system
>>>> + *                    configuration.
>>>> + * device/tile#/memory/freq0/min_freq - This is minimum frequency. 
>>>> This
>>>> value is read-only as it
>>>> + *                    is the fixed fuse point PN. It is not the
>>>> system
>>>> + *                    configuration.
>>>> + */
>>>> +
>>>> +static struct xe_tile *dev_to_tile(struct device *dev) {
>>>> +    return kobj_to_tile(dev->kobj.parent); }
>>>> +
>>>> +static ssize_t max_freq_show(struct device *dev, struct 
>>>> device_attribute
>>>> *attr,
>>>> +                 char *buf)
>>>> +{
>>>> +    struct xe_tile *tile = dev_to_tile(dev);
>>>> +    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 *= 50;
>>>> +
>>>> +    return sysfs_emit(buf, "%u\n", val);
>>>> +}
>>>> +static DEVICE_ATTR_RO(max_freq);
>>>> +
>>>> +static ssize_t min_freq_show(struct device *dev, struct 
>>>> device_attribute *attr,
>>>> +                 char *buf)
>>>> +{
>>>> +    struct xe_tile *tile = dev_to_tile(dev);
>>>> +    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 *= 50;
>>>> +
>>>> +    return sysfs_emit(buf, "%u\n", val);
>>>> +}
>>>> +static DEVICE_ATTR_RO(min_freq);
>>>> +
>>>> +static struct attribute *freq_attrs[] = {
>>>> +    &dev_attr_max_freq.attr,
>>>> +    &dev_attr_min_freq.attr,
>>>> +    NULL
>>>> +};
>>>> +
>>>> +static const struct attribute_group freq_group_attrs = {
>>>> +    .name = "freq0",
>>>> +    .attrs = freq_attrs,
>>>> +};
>>>> +
>>>> +static void vram_freq_fini(struct drm_device *drm, void *arg) {
>>>> +    struct kobject *kobj = arg;
>>>> +
>>>> +    sysfs_remove_group(kobj, &freq_group_attrs);
>>>> +    kobject_put(kobj);
>>>> +}
>>>> +
>>>> +void xe_vram_freq_init(struct xe_tile *tile) {
>>> Please provide a kernel function doc for this exported function.
>>>> +    struct xe_device *xe = tile_to_xe(tile);
>>>> +    struct kobject *kobj;
>>>> +    int err;
>>>> +
>>>> +    if (xe->info.platform == XE_PVC) {
>>> Drop these platform checks(including below ones) , instead use a 
>>> early return at starts of this function.
>>> If (xe->info.platform != XE_PVC)
>>>     Return.
>>> With all of above comment.
>>> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>
>> Will make the changes. Thanks for the r-b.
>>
>> Regards,
>>
>> Suja
>>
>>>> +        kobj = kobject_create_and_add("memory", tile->sysfs);
>>>> +        if (!kobj)
>>>> +            drm_warn(&xe->drm, "failed to add memory
>>>> directory, err: %d\n", -ENOMEM);
>>>> +    }
>>>> +
>>>> +    if (kobj && xe->info.platform == XE_PVC) {
>>>> +        err = sysfs_create_group(kobj, &freq_group_attrs);
>>>> +        if (err) {
> Need kobject_put here?
> If there is failure in creating freq attrs, memory dir will be empty.
> Should be removed.
Sure, can't hurt to have extra cleanup.
>>>> +            drm_warn(&xe->drm, "failed to register vram freq
>>>> sysfs, err: %d\n", err);
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    if (xe->info.platform == XE_PVC) {
>>>> +        err = drmm_add_action_or_reset(&xe->drm, vram_freq_fini,
>>>> kobj);
>>>> +        if (err)
>>>> +            drm_warn(&xe->drm, "%s:
>>>> drmm_add_action_or_reset failed, err: %d\n",
>>>> +                 __func__, err);
>>>> +    }
>>>> +
>>>> +}
>>>> +
>>>> +
>>>> diff --git a/drivers/gpu/drm/xe/xe_vram_freq.h
>>>> b/drivers/gpu/drm/xe/xe_vram_freq.h
>>>> new file mode 100644
>>>> index 000000000000..94b04178a798
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/xe/xe_vram_freq.h
>>>> @@ -0,0 +1,16 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +/*
>>>> + * Copyright © 2023 Intel Corporation
> 2024
>>>> + */
>>>> +
>>>> +#ifndef _XE_VRAM_FREQ_H_
>>>> +#define _XE_VRAM_FREQ_H_
>>>> +
>>>> +#include <drm/drm_managed.h>
>>>> +
>>>> +#include "xe_device.h"
>>>> +#include "xe_tile_sysfs.h"
> unnecessary headers. Use forward declaration of tile
>
> Thanks
> Riana

Sure will fix above comments, before merge.

Thanks,

Suja

>>>> +
>>>> +void xe_vram_freq_init(struct xe_tile *tile);
>>>> +
>>>> +#endif /* _XE_VRAM_FREQ_H_ */
>>>> -- 
>>>> 2.25.1