[v6] drm/xe: Add vram frequency sysfs attributes
Riana Tauro
riana.tauro at intel.com
Tue Jan 2 07:09:43 UTC 2024
Hi Suja
On 12/28/2023 3:34 PM, Sundaresan, Sujaritha wrote:
>
> On 12/27/2023 5:48 PM, Sundaresan, Sujaritha wrote:
>>
>> 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.
I think you can try something similar to the below file
xe_hw_engine_class_sysfs.c
Has different functions for different hierarchy of kobjects.
Thanks
Riana
>>>
>>> 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
>
> Hi Anshuman,
>
> I've moved everything to a new file called xe_vram_freq.c/h . All
> kmemleaks and unload/reload checks are clean.
>
> Let me know if the naming works.
>
> The documentation added is as below, let me know if any tweaks are needed;
>
> /**
> * 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 frequency.
> * 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 frequency.
> */
>
> 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