[v6] drm/xe: Add vram frequency sysfs attributes
Sundaresan, Sujaritha
sujaritha.sundaresan at intel.com
Wed Dec 27 12:18:20 UTC 2023
On 12/27/2023 4:26 PM, Gupta, Anshuman wrote:
>
>> -----Original Message-----
>> From: Sundaresan, Sujaritha <sujaritha.sundaresan at intel.com>
>> Sent: Wednesday, December 27, 2023 3:01 PM
>> 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/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.
> Current patch looks broken to me, this will leak the kobject on module unload.
> Please check kmemleak after module unload.
>> 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.
> You may try by changing the type of void *arg to "memory" kob to fini function ?
>> 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.
> I think it is needed for a functional working patch, if it can not be handled by using
> "void *arg" in fini function.
>
> Thanks,
> Anshuman Gupta.
Since this is bit contentious even though the patch is functional, I
will move these sysfs out of xe_tile_sysfs.c to it own new file.
This should clear up everything.
Thanks,
Suja
>
>
>> 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