drm/amd/amdgpu: Add ip_discovery_text sysfs entry (v2)
Tom St Denis
tstdenis82 at gmail.com
Wed Jan 26 19:14:09 UTC 2022
Thanks, if we don't end up dropping this patchset I'll incorporate your
suggestions into a v3.
Tom
On Wed, Jan 26, 2022 at 12:36 AM Limonciello, Mario <
mario.limonciello at amd.com> wrote:
> A few suggestion ideas inline.
>
> On 1/25/2022 12: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: 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
> >
> > (v2): Use a macro for buffer size and fix alignment in amdgpu.h
> >
> > Signed-off-by: Tom St Denis <tom.stdenis at amd.com>
> > ---
> > 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_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 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(&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,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;
>
> If you take my other suggestion on cleanup, maybe you can also return
> -ENOMEM here.
>
> > + }
> > + *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);
>
> You've got a variety of new codepaths that do this freeing of the
> memory. Have you considered to add a "goto cleanup" instead at the end
> of the function?
>
> Then each of these turns into
> ret = -ENOMEM;
> goto cleanup;
>
> cleanup:
> DRM_ERROR("Out of memory");
> kfree(..)
> kfree(..)
> return ret;
>
> > + 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 +468,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 +509,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, IP_DISCOVERY_BLOCK_SIZE-1,
> "%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)) {
>
> If you take my other suggestion, you could instead do something like
> ret = add_string(..)
> if (ret)
> goto cleanup;
>
> Here and the other places you use add_string.
>
> > + 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 +529,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,
> IP_DISCOVERY_BLOCK_SIZE-1, "%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 +568,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;
> > }
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20220126/3bc402e2/attachment-0001.htm>
More information about the amd-gfx
mailing list