[v6] drm/xe: Add vram frequency sysfs attributes
Sundaresan, Sujaritha
sujaritha.sundaresan at intel.com
Thu Dec 28 10:04:47 UTC 2023
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.
>>
>> 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