[v6] drm/xe: Add vram frequency sysfs attributes
Gupta, Anshuman
anshuman.gupta at intel.com
Wed Dec 27 10:56:42 UTC 2023
> -----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.
>
> 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