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

2022-01-27 Thread Christian König




Am 25.01.22 um 19:18 schrieb Tom St Denis:

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: 0c00 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: 01c0 02409000
DBGU0{0} v3.0.0: 0180 02409800
DBGU1{0} v3.0.0: 01a0 02409c00
DF{0} v3.0.0: 7000 0240b800
DFX{0} v4.1.0: 0580 02409400
DFX_DAP{0} v2.0.0: 05a0 00b8 0240c400
DMU{0} v2.0.2: 0012 00c0 34c0 9000 02403c00
FUSE{0} v11.0.0: 00017400 02401400
GC{0} v10.1.10: 1260 a000 02402c00


Mhm, I'm also leaning more into Alex direction that this should probably 
better represented as directory structure in sysfs.


We could expose the IP discovery table as binary, but parsing it first 
and then not building proper sysfs structures is pretty clearly against 
the documented rules.


Regards,
Christian.



(v2): Use a macro for buffer size and fix alignment in amdgpu.h

Signed-off-by: Tom St Denis 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |  1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 79 ++-
  2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 3bc76759c143..43caeb4bdc07 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_tharvest_ip_mask;
int num_ip_blocks;
+   char*ip_discovery_text;
struct mutexmn_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 07623634fdc2..d036977dab8a 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(>dev->kobj, 
_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(>dev->kobj, _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,22 @@ static int amdgpu_discovery_validate_ip(const struct ip 
*ip)
return 0;
  }
  
+#define IP_DISCOVERY_BLOCK_SIZE 4096

+
+static int add_string(char **dst, unsigned *size, char *src)
+{
+   if (strlen(src) + strlen(*dst) >= *size) {
+   void *tmp = krealloc(*dst, *size + IP_DISCOVERY_BLOCK_SIZE, 
GFP_KERNEL);
+   if (!tmp) {
+   return -1;
+   }
+   *dst = tmp;
+   *size = *size + IP_DISCOVERY_BLOCK_SIZE;
+   }
+   strcat(*dst, src);
+   return 0;
+}
+
  int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
  {
struct binary_header *bhdr;
@@ -396,6 +434,8 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device 
*adev)
int hw_ip;
int i, j, k;
int r;
+   unsigned txt_size = IP_DISCOVERY_BLOCK_SIZE;
+   char *linebuf;
  
  	r = amdgpu_discovery_init(adev);

if (r) {
@@ -410,6 +450,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(IP_DISCOVERY_BLOCK_SIZE, GFP_KERNEL);
+   

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

2022-01-25 Thread Luben Tuikov
Inlined:

On 2022-01-25 13:18, Tom St Denis 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: 0c00 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: 01c0 02409000
> DBGU0{0} v3.0.0: 0180 02409800
> DBGU1{0} v3.0.0: 01a0 02409c00
> DF{0} v3.0.0: 7000 0240b800
> DFX{0} v4.1.0: 0580 02409400
> DFX_DAP{0} v2.0.0: 05a0 00b8 0240c400
> DMU{0} v2.0.2: 0012 00c0 34c0 9000 02403c00
> FUSE{0} v11.0.0: 00017400 02401400
> GC{0} v10.1.10: 1260 a000 02402c00
>
> (v2): Use a macro for buffer size and fix alignment in amdgpu.h
>
> Signed-off-by: Tom St Denis 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 79 ++-
>  2 files changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 3bc76759c143..43caeb4bdc07 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_tharvest_ip_mask;
>   int num_ip_blocks;
> + char*ip_discovery_text;
>   struct mutexmn_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 07623634fdc2..d036977dab8a 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

C++ comments are discouraged in the kernel.

Run your patch through scripts/checkpatch.pl.

> + r = sysfs_create_file(>dev->kobj, 
> _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(>dev->kobj, _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,22 @@ static int amdgpu_discovery_validate_ip(const struct ip 
> *ip)
>   return 0;
>  }
>  
> +#define IP_DISCOVERY_BLOCK_SIZE 4096
> +
> +static int add_string(char **dst, unsigned *size, char *src)
> +{
> + if (strlen(src) + strlen(*dst) >= *size) {
> + void *tmp = krealloc(*dst, *size + IP_DISCOVERY_BLOCK_SIZE, 
> GFP_KERNEL);
> + if (!tmp) {
> + return -1;
> + }
> + *dst = tmp;
> + *size = *size + IP_DISCOVERY_BLOCK_SIZE;
> + }
> + strcat(*dst, src);
> + return 0;
> +}
> +
>  int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
>  {
>   struct binary_header *bhdr;
> @@ -396,6 +434,8 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device 
> *adev)
>   int hw_ip;
>   int i, j, k;
>   int r;
> + unsigned txt_size = IP_DISCOVERY_BLOCK_SIZE;
> + char *linebuf;
>  
>   r = amdgpu_discovery_init(adev);
>   if (r) {
> @@ -410,6 +450,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(IP_DISCOVERY_BLOCK_SIZE, GFP_KERNEL);
> + if (!adev->ip_discovery_text || !linebuf) {
> +   

[PATCH] drm/amd/amdgpu: Add ip_discovery_text sysfs entry (v2)

2022-01-25 Thread Tom St Denis
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: 0c00 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: 01c0 02409000
DBGU0{0} v3.0.0: 0180 02409800
DBGU1{0} v3.0.0: 01a0 02409c00
DF{0} v3.0.0: 7000 0240b800
DFX{0} v4.1.0: 0580 02409400
DFX_DAP{0} v2.0.0: 05a0 00b8 0240c400
DMU{0} v2.0.2: 0012 00c0 34c0 9000 02403c00
FUSE{0} v11.0.0: 00017400 02401400
GC{0} v10.1.10: 1260 a000 02402c00

(v2): Use a macro for buffer size and fix alignment in amdgpu.h

Signed-off-by: Tom St Denis 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h   |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 79 ++-
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 3bc76759c143..43caeb4bdc07 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_tharvest_ip_mask;
int num_ip_blocks;
+   char*ip_discovery_text;
struct mutexmn_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 07623634fdc2..d036977dab8a 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(>dev->kobj, 
_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(>dev->kobj, _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,22 @@ static int amdgpu_discovery_validate_ip(const struct ip 
*ip)
return 0;
 }
 
+#define IP_DISCOVERY_BLOCK_SIZE 4096
+
+static int add_string(char **dst, unsigned *size, char *src)
+{
+   if (strlen(src) + strlen(*dst) >= *size) {
+   void *tmp = krealloc(*dst, *size + IP_DISCOVERY_BLOCK_SIZE, 
GFP_KERNEL);
+   if (!tmp) {
+   return -1;
+   }
+   *dst = tmp;
+   *size = *size + IP_DISCOVERY_BLOCK_SIZE;
+   }
+   strcat(*dst, src);
+   return 0;
+}
+
 int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
 {
struct binary_header *bhdr;
@@ -396,6 +434,8 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device 
*adev)
int hw_ip;
int i, j, k;
int r;
+   unsigned txt_size = IP_DISCOVERY_BLOCK_SIZE;
+   char *linebuf;
 
r = amdgpu_discovery_init(adev);
if (r) {
@@ -410,6 +450,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(IP_DISCOVERY_BLOCK_SIZE, 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