Re: [v4] drm/xe: Add vram frequency sysfs attributes

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Dec 14, 2023 at 04:41:59PM +0530, Sujaritha Sundaresan wrote:
> Add vram frequency sysfs attributes under the below hierarchy;
> 
> /device/tile#/freq0
> 		|-rp0_freq
> 		|-rpn_freq

hmm...
you have a good point of not leaving an empty memory dir on platforms
where we don't have the freq interfaces.
But then we should only create the mem dir when we are going to create
the freq.

In the way this patch is proposing it looks like the frequency is about
the tile. like a tile basedie frequency or something like that.

So, we do need the memory or vram dir to make it really clear that
the freq is about the memory.

> 
> 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
> 
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pcode_api.h  |  8 +++
>  drivers/gpu/drm/xe/xe_tile_sysfs.c | 80 ++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
> index 5935cfe30204..26ceb8c0c010 100644
> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
> @@ -42,6 +42,14 @@
>  #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
> +/* PCODE_FREQUENCY_CONFIG sub-commands (param1) */
> +#define     PCODE_MBOX_FC_SC_READ_FUSED_P0	0x0
> +#define     PCODE_MBOX_FC_SC_READ_FUSED_PN	0x1
> +/* PCODE_MBOX_DOMAIN_* - mailbox domain IDs */
> +/* PCODE_FREQUENCY_CONFIG 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..45518442ed15 100644
> --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
> +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
> @@ -7,9 +7,23 @@
>  #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"
>  
> +#define GT_FREQUENCY_MULTIPLIER	50

hmmm... it is kind of strange to define a frequency multiplier in
the tile_sysfs component.

This is probably an indication that we need a specific component for
this memory freq?

perhaps xe_vram_freq?

> +
> +/**
> + * DOC: Xe Tile sysfs
> + *
> + * Provides sysfs entries for memory related frequency in tile
> + *
> + * device/tile#/freq0/rp0_freq - The Render Performance 0 level, which is the maximum one.
> + * device/tile#/freq0/rpn_freq - The Render Performance N level, which is the minimal one.
> + */
> +
>  static void xe_tile_sysfs_kobj_release(struct kobject *kobj)
>  {
>  	kfree(kobj);
> @@ -20,10 +34,68 @@ static const struct kobj_type xe_tile_sysfs_kobj_type = {
>  	.sysfs_ops = &kobj_sysfs_ops,
>  };
>  
> +static ssize_t rp0_freq_show(struct device *kdev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct xe_tile *tile = kobj_to_tile(&kdev->kobj);
> +	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 *= GT_FREQUENCY_MULTIPLIER;
> +
> +	return sysfs_emit(buf, "%u\n", val);
> +}
> +static DEVICE_ATTR_RO(rp0_freq);
> +
> +static ssize_t rpn_freq_show(struct device *kdev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct xe_tile *tile = kobj_to_tile(&kdev->kobj);
> +	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 *= GT_FREQUENCY_MULTIPLIER;
> +
> +	return sysfs_emit(buf, "%u\n", val);
> +}
> +static DEVICE_ATTR_RO(rpn_freq);
> +
> +static struct attribute *freq_attrs[] = {
> +	&dev_attr_rp0_freq.attr,
> +	&dev_attr_rpn_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;
>  
> +	sysfs_remove_group(tile->sysfs, &freq_group_attrs);

if you didn't create the group you shouldn't remove it, so same
conditions for create should apply here.

>  	kobject_put(tile->sysfs);
>  }
>  
> @@ -50,6 +122,14 @@ void xe_tile_sysfs_init(struct xe_tile *tile)
>  
>  	tile->sysfs = &kt->base;
>  
> +	if (xe->info.platform == XE_PVC) {
> +		err = sysfs_create_group(tile->sysfs, &freq_group_attrs);
> +		if (err) {
> +			drm_warn(&xe->drm, "failed to register vram freq sysfs, err: %d\n", err);
> +			return;
> +		}
> +	}
> +
>  	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
>