<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
</head>
<body>
<div>I literally brought this up in our initial discussion....</div>
<div><br>
</div>
<div>Frankly from umrs point of view a single file is easier.</div>
<div><br>
</div>
<div>But I can't code anything until it's in the tree...</div>
<div><br>
</div>
<div><br>
</div>
<div>Tom</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div>
<div style="font-family:Tahoma; font-size:13px"><br>
</div>
</div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Alex Deucher <alexdeucher@gmail.com><br>
<b>Sent:</b> Tuesday, January 25, 2022 11:39<br>
<b>To:</b> StDenis, Tom <Tom.StDenis@amd.com><br>
<b>Cc:</b> amd-gfx list <amd-gfx@lists.freedesktop.org><br>
<b>Subject:</b> Re: [PATCH] drm/amd/amdgpu: Add ip_discovery_text sysfs entry</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">On Mon, Jan 24, 2022 at 1:07 PM Tom St Denis <tom.stdenis@amd.com> wrote:<br>
><br>
> Newer hardware has a discovery table in hardware that the kernel will<br>
> rely on instead of header files for things like IP offsets.  This<br>
> sysfs entry adds a simple to parse table of IP instances and segment<br>
> offsets.<br>
><br>
> Produces output that looks like:<br>
><br>
> $ cat ip_discovery_text<br>
> ATHUB{0} v2.0.0: 00000c00 02408c00<br>
> CLKA{0} v11.0.0: 00016c00 02401800<br>
> CLKA{1} v11.0.0: 00016e00 02401c00<br>
> CLKA{2} v11.0.0: 00017000 02402000<br>
> CLKA{3} v11.0.0: 00017200 02402400<br>
> CLKA{4} v11.0.0: 0001b000 0242d800<br>
> CLKB{0} v11.0.0: 00017e00 0240bc00<br>
> DBGU_NBIO{0} v3.0.0: 000001c0 02409000<br>
> DBGU0{0} v3.0.0: 00000180 02409800<br>
> DBGU1{0} v3.0.0: 000001a0 02409c00<br>
> DF{0} v3.0.0: 00007000 0240b800<br>
> DFX{0} v4.1.0: 00000580 02409400<br>
> DFX_DAP{0} v2.0.0: 000005a0 00b80000 0240c400<br>
> DMU{0} v2.0.2: 00000012 000000c0 000034c0 00009000 02403c00<br>
> FUSE{0} v11.0.0: 00017400 02401400<br>
> GC{0} v10.1.10: 00001260 0000a000 02402c00<br>
<br>
I'm not opposed to this, but the general rule is one value per file<br>
with sysfs.  Maybe something like:<br>
<br>
$ ls ip_discovery<br>
ATHUB CLKA CLKB DBGU_NBIO DBGU0 DBGU1<br>
$ ls ip_discovery/CLKA<br>
0 1 2 3 4<br>
$ ls ip_discovery/CLKA/0<br>
version offset0 offset1<br>
<br>
Alex<br>
<br>
><br>
> Signed-off-by: Tom St Denis <tom.stdenis@amd.com><br>
> ---<br>
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  1 +<br>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 77 ++++++++++++++++++-<br>
>  2 files changed, 77 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h<br>
> index 3bc76759c143..6920f73bbe73 100644<br>
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h<br>
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h<br>
> @@ -1019,6 +1019,7 @@ struct amdgpu_device {<br>
>         struct amdgpu_ip_block          ip_blocks[AMDGPU_MAX_IP_NUM];<br>
>         uint32_t                        harvest_ip_mask;<br>
>         int                             num_ip_blocks;<br>
> +       char                    *ip_discovery_text;<br>
>         struct mutex    mn_lock;<br>
>         DECLARE_HASHTABLE(mn_hash, 7);<br>
><br>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c<br>
> index 285811509d94..c64cab0ad18e 100644<br>
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c<br>
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c<br>
> @@ -267,6 +267,19 @@ static void amdgpu_discovery_harvest_config_quirk(struct amdgpu_device *adev)<br>
>         }<br>
>  }<br>
><br>
> +static ssize_t ip_discovery_text_show(struct device *dev,<br>
> +               struct device_attribute *attr, char *buf)<br>
> +{<br>
> +       struct drm_device *ddev = dev_get_drvdata(dev);<br>
> +       struct amdgpu_device *adev = drm_to_adev(ddev);<br>
> +<br>
> +       return sysfs_emit(buf, "%s", adev->ip_discovery_text);<br>
> +}<br>
> +<br>
> +static DEVICE_ATTR(ip_discovery_text, S_IRUGO,<br>
> +                               ip_discovery_text_show, NULL);<br>
> +<br>
> +<br>
>  static int amdgpu_discovery_init(struct amdgpu_device *adev)<br>
>  {<br>
>         struct table_info *info;<br>
> @@ -351,6 +364,11 @@ static int amdgpu_discovery_init(struct amdgpu_device *adev)<br>
>                 goto out;<br>
>         }<br>
><br>
> +       // init sysfs for ip_discovery<br>
> +       r = sysfs_create_file(&adev->dev->kobj, &dev_attr_ip_discovery_text.attr);<br>
> +       if (r)<br>
> +               dev_err(adev->dev, "Could not create amdgpu device attr\n");<br>
> +<br>
>         return 0;<br>
><br>
>  out:<br>
> @@ -363,7 +381,11 @@ static int amdgpu_discovery_init(struct amdgpu_device *adev)<br>
>  void amdgpu_discovery_fini(struct amdgpu_device *adev)<br>
>  {<br>
>         kfree(adev->mman.discovery_bin);<br>
> +       kfree(adev->ip_discovery_text);<br>
> +       sysfs_remove_file(&adev->dev->kobj, &dev_attr_ip_discovery_text.attr);<br>
> +<br>
>         adev->mman.discovery_bin = NULL;<br>
> +       adev->ip_discovery_text = NULL;<br>
>  }<br>
><br>
>  static int amdgpu_discovery_validate_ip(const struct ip *ip)<br>
> @@ -382,6 +404,20 @@ static int amdgpu_discovery_validate_ip(const struct ip *ip)<br>
>         return 0;<br>
>  }<br>
><br>
> +static int add_string(char **dst, unsigned *size, char *src)<br>
> +{<br>
> +       if (strlen(src) + strlen(*dst) >= *size) {<br>
> +               void *tmp = krealloc(*dst, *size + 4096, GFP_KERNEL);<br>
> +               if (!tmp) {<br>
> +                       return -1;<br>
> +               }<br>
> +               *dst = tmp;<br>
> +               *size = *size + 4096;<br>
> +       }<br>
> +       strcat(*dst, src);<br>
> +       return 0;<br>
> +}<br>
> +<br>
>  int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)<br>
>  {<br>
>         struct binary_header *bhdr;<br>
> @@ -396,6 +432,8 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)<br>
>         int hw_ip;<br>
>         int i, j, k;<br>
>         int r;<br>
> +       unsigned txt_size = 4096;<br>
> +       char *linebuf;<br>
><br>
>         r = amdgpu_discovery_init(adev);<br>
>         if (r) {<br>
> @@ -410,6 +448,15 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)<br>
><br>
>         DRM_DEBUG("number of dies: %d\n", num_dies);<br>
><br>
> +       adev->ip_discovery_text = kzalloc(txt_size, GFP_KERNEL);<br>
> +       linebuf = kzalloc(4096, GFP_KERNEL);<br>
> +       if (!adev->ip_discovery_text || !linebuf) {<br>
> +               DRM_ERROR("Out of memory\n");<br>
> +               kfree(linebuf);<br>
> +               kfree(adev->ip_discovery_text);<br>
> +               return -ENOMEM;<br>
> +       }<br>
> +<br>
>         for (i = 0; i < num_dies; i++) {<br>
>                 die_offset = le16_to_cpu(ihdr->die_info[i].die_offset);<br>
>                 dhdr = (struct die_header *)(adev->mman.discovery_bin + die_offset);<br>
> @@ -419,6 +466,8 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)<br>
>                 if (le16_to_cpu(dhdr->die_id) != i) {<br>
>                         DRM_ERROR("invalid die id %d, expected %d\n",<br>
>                                         le16_to_cpu(dhdr->die_id), i);<br>
> +                       kfree(linebuf);<br>
> +                       kfree(adev->ip_discovery_text);<br>
>                         return -EINVAL;<br>
>                 }<br>
><br>
> @@ -458,6 +507,19 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)<br>
>                             le16_to_cpu(ip->hw_id) == SDMA3_HWID)<br>
>                                 adev->sdma.num_instances++;<br>
><br>
> +                       snprintf(linebuf, 4096, "%s{%d} v%d.%d.%d: ",<br>
> +                                 hw_id_names[le16_to_cpu(ip->hw_id)],<br>
> +                                 ip->number_instance,<br>
> +                                 ip->major, ip->minor,<br>
> +                                 ip->revision);<br>
> +                       if (add_string(&adev->ip_discovery_text, &txt_size, linebuf)) {<br>
> +                               DRM_ERROR("Out of memory\n");<br>
> +                               kfree(linebuf);<br>
> +                               kfree(adev->ip_discovery_text);<br>
> +                               return -ENOMEM;<br>
> +                       }<br>
> +<br>
> +<br>
>                         for (k = 0; k < num_base_address; k++) {<br>
>                                 /*<br>
>                                  * convert the endianness of base addresses in place,<br>
> @@ -465,6 +527,19 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)<br>
>                                  */<br>
>                                 ip->base_address[k] = le32_to_cpu(ip->base_address[k]);<br>
>                                 DRM_DEBUG("\t0x%08x\n", ip->base_address[k]);<br>
> +                               snprintf(linebuf, 4096, "%08lx ", (unsigned long)ip->base_address[k]);<br>
> +                               if (add_string(&adev->ip_discovery_text, &txt_size, linebuf)) {<br>
> +                                       DRM_ERROR("Out of memory\n");<br>
> +                                       kfree(linebuf);<br>
> +                                       kfree(adev->ip_discovery_text);<br>
> +                                       return -ENOMEM;<br>
> +                               }<br>
> +                       }<br>
> +                       if (add_string(&adev->ip_discovery_text, &txt_size, "\n")) {<br>
> +                               DRM_ERROR("Out of memory\n");<br>
> +                               kfree(linebuf);<br>
> +                               kfree(adev->ip_discovery_text);<br>
> +                               return -ENOMEM;<br>
>                         }<br>
><br>
>                         for (hw_ip = 0; hw_ip < MAX_HWIP; hw_ip++) {<br>
> @@ -491,7 +566,7 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)<br>
>                         ip_offset += sizeof(*ip) + 4 * (ip->num_base_address - 1);<br>
>                 }<br>
>         }<br>
> -<br>
> +       kfree(linebuf);<br>
>         return 0;<br>
>  }<br>
><br>
> --<br>
> 2.32.0<br>
><br>
</div>
</span></font></div>
</body>
</html>