[v6] drm/xe: Add vram frequency sysfs attributes
Sundaresan, Sujaritha
sujaritha.sundaresan at intel.com
Wed Dec 27 09:31:18 UTC 2023
On 12/26/2023 7:58 PM, Gupta, Anshuman wrote:
>
>> -----Original Message-----
>> From: Sundaresan, Sujaritha <sujaritha.sundaresan at intel.com>
>> Sent: Tuesday, December 26, 2023 10:02 AM
>> To: Gupta, Anshuman <anshuman.gupta at intel.com>; intel-
>> xe at lists.freedesktop.org
>> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>
>> Subject: Re: [v6] drm/xe: Add vram frequency sysfs attributes
>>
>>
>> On 12/22/2023 7:34 PM, Gupta, Anshuman wrote:
>>>> -----Original Message-----
>>>> From: Sundaresan, Sujaritha <sujaritha.sundaresan at intel.com>
>>>> Sent: Friday, December 22, 2023 4:37 PM
>>>> To: intel-xe at lists.freedesktop.org
>>>> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>; Gupta, Anshuman
>>>> <anshuman.gupta at intel.com>; Sundaresan, Sujaritha
>>>> <sujaritha.sundaresan at intel.com>
>>>> Subject: [v6] 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)
>>>>
>>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/xe_pcode_api.h | 7 +++
>>>> drivers/gpu/drm/xe/xe_tile_sysfs.c | 86
>>>> ++++++++++++++++++++++++++++++
>>>> 2 files changed, 93 insertions(+)
>>>>
>>>> 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..cdc9dbbc97b0 100644
>>>> --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>>> +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>>> @@ -7,9 +7,21 @@
>>>> #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"
>>>>
>>>> +/**
>>>> + * DOC: Xe Tile sysfs
>>>> + *
>>>> + * Provides sysfs entries for frequency in tile
>>>> + *
>>>> + * device/tile#/memory/freq0/max_freq - Maximum Frequency, not a
>>>> configuration and read-only.
>>> Let's increase verbosity of doc something explaining it is a fixed fuse point not a
>> configuration.
>> Sure
>>>> + * device/tile#/memory/freq0/min_freq - Minimum Frequency, not a
>>>> configuration and read-only.
>>>> + */
>>>> +
>>>> static void xe_tile_sysfs_kobj_release(struct kobject *kobj) {
>>>> kfree(kobj);
>>>> @@ -20,6 +32,65 @@ static const struct kobj_type
>>>> xe_tile_sysfs_kobj_type = {
>>>> .sysfs_ops = &kobj_sysfs_ops,
>>>> };
>>>>
>>>> +static ssize_t max_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,
>>>> 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 *kdev, struct
>>>> +device_attribute
>>>> *attr,
>>>> + char *buf)
>>>> +{
>>>> + struct kobject *kobj = &kdev->kobj;
>>>> + struct xe_tile *tile = kobj_to_tile(kobj->parent);
>>> If you are missing to create a kobject for freq0 , then this should be
>>> kobj->parent->parent.
>> I don't think a kobject is needed for freq0, since it we are only using
>> attribute_group for it.
>>
>> Similar to throttle_reasons.
> Sure, thanks for explanation.
>>>> + 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 tile_sysfs_fini(struct drm_device *drm, void *arg) {
>>>> struct xe_tile *tile = arg;
>>>> @@ -32,6 +103,7 @@ void xe_tile_sysfs_init(struct xe_tile *tile)
>>>> struct xe_device *xe = tile_to_xe(tile);
>>>> struct device *dev = xe->drm.dev;
>>>> struct kobj_tile *kt;
>>>> + struct kobject *kobj;
>>>> int err;
>>>>
>>>> kt = kzalloc(sizeof(*kt), GFP_KERNEL); @@ -50,6 +122,20 @@ void
>>>> xe_tile_sysfs_init(struct xe_tile *tile)
>>>>
>>>> tile->sysfs = &kt->base;
>>>>
>>>> + if (xe->info.platform == XE_PVC) {
>>>> + kobj = kobject_create_and_add("memory", tile->sysfs);
>>> How freq0 is getting added, I am unable to see the freq0 kobject as per the path
>> "device/tile#/memory/freq0/"
>> freq0 is being added as an attribute group. It is similar to the throttle_reasons
>> implementation.
>>>> + 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) {
>>>> + drm_warn(&xe->drm, "failed to register vram freq
>>>> sysfs, err: %d\n", err);
>>>> + return;
>>>> + }
>>>> + }
>>> Don't we need sysfs cleanup and kobject_put in tile_sysfs_fini() ?
>>> Have you made sure kmemleak won't complain here on memory leak ?
> Check the kobject_release() , this will only get call , when all ref count of that kobj is being put.
> Here you are creating a object and it will never be released.
> Also we need to call the sysfs_remove_group() as well in fini function ?
>
> Thanks,
> Anshuman.
This is a bit of unique case. In this file we have two issues that keeps
us from cleaning up like others.
One, we need to cleanup the base tile directory here. And second, we are
creating the kobject only for PVC. If we add the "memory" kobject
cleanup to fini, we will be defining tile using kobj. This is causing an
error on unload, despite adding platform conditions on fini.
After testing multiple iterations of the fini function, this was the
cleanest way with no errors that worked across platforms.
If needed, the only way to accommodate the kobject_put(kobj) and the
sysfs_remove_group is to move the vram sysfs creation to a separate
file, similar to throttle_reasons and gt_freq.
Thanks,
Suja
>
>>> Thanks,
>>> Anshuman Gupta.
>> I have already checked for mem leaks during the cleanup. The kobject_put is not
>> needed.
>>
>> Thanks,
>>
>> Suja
>>
>>>> +
>>>> 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
More information about the Intel-xe
mailing list