Re: [PATCH] drm/amd/amdgpu: Add ip_discovery_text sysfs entry

["StDenis, Tom" <Tom.StDenis@xxxxxxx>]

I literally brought this up in our initial discussion....

Frankly from umrs point of view a single file is easier.

But I can't code anything until it's in the tree...

Tom

---

From: Alex Deucher <alexdeucher@xxxxxxxxx>
Sent: Tuesday, January 25, 2022 11:39
To: StDenis, Tom <Tom.StDenis@xxxxxxx>
Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH] drm/amd/amdgpu: Add ip_discovery_text sysfs entry

 

On Mon, Jan 24, 2022 at 1:07 PM Tom St Denis <tom.stdenis@xxxxxxx> wrote:
>
> Newer hardware has a discovery table in hardware that the kernel will
> rely on instead of header files for things like IP offsets.  This
> sysfs entry adds a simple to parse table of IP instances and segment
> offsets.
>
> Produces output that looks like:
>
> $ cat ip_discovery_text
> ATHUB{0} v2.0.0: 00000c00 02408c00
> CLKA{0} v11.0.0: 00016c00 02401800
> CLKA{1} v11.0.0: 00016e00 02401c00
> CLKA{2} v11.0.0: 00017000 02402000
> CLKA{3} v11.0.0: 00017200 02402400
> CLKA{4} v11.0.0: 0001b000 0242d800
> CLKB{0} v11.0.0: 00017e00 0240bc00
> DBGU_NBIO{0} v3.0.0: 000001c0 02409000
> DBGU0{0} v3.0.0: 00000180 02409800
> DBGU1{0} v3.0.0: 000001a0 02409c00
> DF{0} v3.0.0: 00007000 0240b800
> DFX{0} v4.1.0: 00000580 02409400
> DFX_DAP{0} v2.0.0: 000005a0 00b80000 0240c400
> DMU{0} v2.0.2: 00000012 000000c0 000034c0 00009000 02403c00
> FUSE{0} v11.0.0: 00017400 02401400
> GC{0} v10.1.10: 00001260 0000a000 02402c00

I'm not opposed to this, but the general rule is one value per file
with sysfs.  Maybe something like:

$ ls ip_discovery
ATHUB CLKA CLKB DBGU_NBIO DBGU0 DBGU1
$ ls ip_discovery/CLKA
0 1 2 3 4
$ ls ip_discovery/CLKA/0
version offset0 offset1

Alex

>
> Signed-off-by: Tom St Denis <tom.stdenis@xxxxxxx>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 77 ++++++++++++++++++-
>  2 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 3bc76759c143..6920f73bbe73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1019,6 +1019,7 @@ struct amdgpu_device {
>         struct amdgpu_ip_block          ip_blocks[AMDGPU_MAX_IP_NUM];
>         uint32_t                        harvest_ip_mask;
>         int                             num_ip_blocks;
> +       char                    *ip_discovery_text;
>         struct mutex    mn_lock;
>         DECLARE_HASHTABLE(mn_hash, 7);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index 285811509d94..c64cab0ad18e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -267,6 +267,19 @@ static void amdgpu_discovery_harvest_config_quirk(struct amdgpu_device *adev)
>         }
>  }
>
> +static ssize_t ip_discovery_text_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct drm_device *ddev = dev_get_drvdata(dev);
> +       struct amdgpu_device *adev = drm_to_adev(ddev);
> +
> +       return sysfs_emit(buf, "%s", adev->ip_discovery_text);
> +}
> +
> +static DEVICE_ATTR(ip_discovery_text, S_IRUGO,
> +                               ip_discovery_text_show, NULL);
> +
> +
>  static int amdgpu_discovery_init(struct amdgpu_device *adev)
>  {
>         struct table_info *info;
> @@ -351,6 +364,11 @@ static int amdgpu_discovery_init(struct amdgpu_device *adev)
>                 goto out;
>         }
>
> +       // init sysfs for ip_discovery
> +       r = sysfs_create_file(&adev->dev->kobj, &dev_attr_ip_discovery_text.attr);
> +       if (r)
> +               dev_err(adev->dev, "Could not create amdgpu device attr\n");
> +
>         return 0;
>
>  out:
> @@ -363,7 +381,11 @@ static int amdgpu_discovery_init(struct amdgpu_device *adev)
>  void amdgpu_discovery_fini(struct amdgpu_device *adev)
>  {
>         kfree(adev->mman.discovery_bin);
> +       kfree(adev->ip_discovery_text);
> +       sysfs_remove_file(&adev->dev->kobj, &dev_attr_ip_discovery_text.attr);
> +
>         adev->mman.discovery_bin = NULL;
> +       adev->ip_discovery_text = NULL;
>  }
>
>  static int amdgpu_discovery_validate_ip(const struct ip *ip)
> @@ -382,6 +404,20 @@ static int amdgpu_discovery_validate_ip(const struct ip *ip)
>         return 0;
>  }
>
> +static int add_string(char **dst, unsigned *size, char *src)
> +{
> +       if (strlen(src) + strlen(*dst) >= *size) {
> +               void *tmp = krealloc(*dst, *size + 4096, GFP_KERNEL);
> +               if (!tmp) {
> +                       return -1;
> +               }
> +               *dst = tmp;
> +               *size = *size + 4096;
> +       }
> +       strcat(*dst, src);
> +       return 0;
> +}
> +
>  int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
>  {
>         struct binary_header *bhdr;
> @@ -396,6 +432,8 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
>         int hw_ip;
>         int i, j, k;
>         int r;
> +       unsigned txt_size = 4096;
> +       char *linebuf;
>
>         r = amdgpu_discovery_init(adev);
>         if (r) {
> @@ -410,6 +448,15 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
>
>         DRM_DEBUG("number of dies: %d\n", num_dies);
>
> +       adev->ip_discovery_text = kzalloc(txt_size, GFP_KERNEL);
> +       linebuf = kzalloc(4096, GFP_KERNEL);
> +       if (!adev->ip_discovery_text || !linebuf) {
> +               DRM_ERROR("Out of memory\n");
> +               kfree(linebuf);
> +               kfree(adev->ip_discovery_text);
> +               return -ENOMEM;
> +       }
> +
>         for (i = 0; i < num_dies; i++) {
>                 die_offset = le16_to_cpu(ihdr->die_info[i].die_offset);
>                 dhdr = (struct die_header *)(adev->mman.discovery_bin + die_offset);
> @@ -419,6 +466,8 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
>                 if (le16_to_cpu(dhdr->die_id) != i) {
>                         DRM_ERROR("invalid die id %d, expected %d\n",
>                                         le16_to_cpu(dhdr->die_id), i);
> +                       kfree(linebuf);
> +                       kfree(adev->ip_discovery_text);
>                         return -EINVAL;
>                 }
>
> @@ -458,6 +507,19 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
>                             le16_to_cpu(ip->hw_id) == SDMA3_HWID)
>                                 adev->sdma.num_instances++;
>
> +                       snprintf(linebuf, 4096, "%s{%d} v%d.%d.%d: ",
> +                                 hw_id_names[le16_to_cpu(ip->hw_id)],
> +                                 ip->number_instance,
> +                                 ip->major, ip->minor,
> +                                 ip->revision);
> +                       if (add_string(&adev->ip_discovery_text, &txt_size, linebuf)) {
> +                               DRM_ERROR("Out of memory\n");
> +                               kfree(linebuf);
> +                               kfree(adev->ip_discovery_text);
> +                               return -ENOMEM;
> +                       }
> +
> +
>                         for (k = 0; k < num_base_address; k++) {
>                                 /*
>                                  * convert the endianness of base addresses in place,
> @@ -465,6 +527,19 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
>                                  */
>                                 ip->base_address[k] = le32_to_cpu(ip->base_address[k]);
>                                 DRM_DEBUG("\t0x%08x\n", ip->base_address[k]);
> +                               snprintf(linebuf, 4096, "%08lx ", (unsigned long)ip->base_address[k]);
> +                               if (add_string(&adev->ip_discovery_text, &txt_size, linebuf)) {
> +                                       DRM_ERROR("Out of memory\n");
> +                                       kfree(linebuf);
> +                                       kfree(adev->ip_discovery_text);
> +                                       return -ENOMEM;
> +                               }
> +                       }
> +                       if (add_string(&adev->ip_discovery_text, &txt_size, "\n")) {
> +                               DRM_ERROR("Out of memory\n");
> +                               kfree(linebuf);
> +                               kfree(adev->ip_discovery_text);
> +                               return -ENOMEM;
>                         }
>
>                         for (hw_ip = 0; hw_ip < MAX_HWIP; hw_ip++) {
> @@ -491,7 +566,7 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
>                         ip_offset += sizeof(*ip) + 4 * (ip->num_base_address - 1);
>                 }
>         }
> -
> +       kfree(linebuf);
>         return 0;
>  }
>
> --
> 2.32.0
>