* [PATCH] drm/amdgpu: Add sysfs file for VBIOS @ 2017-08-23 18:12 Kent Russell [not found] ` <1503511940-645-1-git-send-email-kent.russell-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Kent Russell @ 2017-08-23 18:12 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Kent Russell This won't change after initialization, so we can add the information when we parse the atombios information. This ensures that we can find out the VBIOS, even when the dmesg buffer fills up, and makes it easier to associate which VBIOS is for which GPU on mGPU configurations. Set the size to 20 characters in case of some weird VBIOS suffix that exceeds the expected 17 character format (3-8-3\0) Signed-off-by: Kent Russell <kent.russell@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- drivers/gpu/drm/amd/amdgpu/atom.h | 1 + 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index a1f9424..f40be71 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) return r; } +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct drm_device *ddev = dev_get_drvdata(dev); + struct amdgpu_device *adev = ddev->dev_private; + struct atom_context *ctx = adev->mode_info.atom_context; + + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); +} + +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, + NULL); + /** * amdgpu_atombios_fini - free the driver info and callbacks for atombios * @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) adev->mode_info.atom_context = NULL; kfree(adev->mode_info.atom_card_info); adev->mode_info.atom_card_info = NULL; + device_remove_file(adev->dev, &dev_attr_vbios_version); } /** @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) { struct card_info *atom_card_info = kzalloc(sizeof(struct card_info), GFP_KERNEL); + int ret; if (!atom_card_info) return -ENOMEM; @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) amdgpu_atombios_scratch_regs_init(adev); amdgpu_atombios_allocate_fb_scratch(adev); } + + ret = device_create_file(adev->dev, &dev_attr_vbios_version); + if (ret) { + DRM_ERROR("Failed to create device file for VBIOS version\n"); + return ret; + } + return 0; } diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/amdgpu/atom.c index d69aa2e..69500a8 100644 --- a/drivers/gpu/drm/amd/amdgpu/atom.c +++ b/drivers/gpu/drm/amd/amdgpu/atom.c @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) idx = 0x80; str = CSTR(idx); - if (*str != '\0') + if (*str != '\0') { pr_info("ATOM BIOS: %s\n", str); + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); + } + return ctx; } diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h b/drivers/gpu/drm/amd/amdgpu/atom.h index ddd8045..a391709 100644 --- a/drivers/gpu/drm/amd/amdgpu/atom.h +++ b/drivers/gpu/drm/amd/amdgpu/atom.h @@ -140,6 +140,7 @@ struct atom_context { int io_mode; uint32_t *scratch; int scratch_size_bytes; + char vbios_version[20]; }; extern int amdgpu_atom_debug; -- 2.7.4 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 29+ messages in thread
[parent not found: <1503511940-645-1-git-send-email-kent.russell-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <1503511940-645-1-git-send-email-kent.russell-5C7GfCeVMHo@public.gmane.org> @ 2017-08-24 1:21 ` Michel Dänzer 2017-08-24 6:21 ` Christian König 1 sibling, 0 replies; 29+ messages in thread From: Michel Dänzer @ 2017-08-24 1:21 UTC (permalink / raw) To: Kent Russell; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 24/08/17 03:12 AM, Kent Russell wrote: > This won't change after initialization, so we can add the information > when we parse the atombios information. This ensures that we can find > out the VBIOS, even when the dmesg buffer fills up, and makes it easier > to associate which VBIOS is for which GPU on mGPU configurations. Set > the size to 20 characters in case of some weird VBIOS suffix that exceeds > the expected 17 character format (3-8-3\0) > > Signed-off-by: Kent Russell <kent.russell@amd.com> Most if not all instances of "VBIOS" in the commit log should probably be replaced by "VBIOS version" for clarity. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <1503511940-645-1-git-send-email-kent.russell-5C7GfCeVMHo@public.gmane.org> 2017-08-24 1:21 ` Michel Dänzer @ 2017-08-24 6:21 ` Christian König [not found] ` <1e64a642-411e-ea58-1639-390e4c3cd75d-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> 1 sibling, 1 reply; 29+ messages in thread From: Christian König @ 2017-08-24 6:21 UTC (permalink / raw) To: Kent Russell, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Am 23.08.2017 um 20:12 schrieb Kent Russell: > This won't change after initialization, so we can add the information > when we parse the atombios information. This ensures that we can find > out the VBIOS, even when the dmesg buffer fills up, and makes it easier > to associate which VBIOS is for which GPU on mGPU configurations. Set > the size to 20 characters in case of some weird VBIOS suffix that exceeds > the expected 17 character format (3-8-3\0) Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. Christian. > > Signed-off-by: Kent Russell <kent.russell@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ > drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- > drivers/gpu/drm/amd/amdgpu/atom.h | 1 + > 3 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index a1f9424..f40be71 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) > return r; > } > > +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct drm_device *ddev = dev_get_drvdata(dev); > + struct amdgpu_device *adev = ddev->dev_private; > + struct atom_context *ctx = adev->mode_info.atom_context; > + > + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); > +} > + > +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, > + NULL); > + > /** > * amdgpu_atombios_fini - free the driver info and callbacks for atombios > * > @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) > adev->mode_info.atom_context = NULL; > kfree(adev->mode_info.atom_card_info); > adev->mode_info.atom_card_info = NULL; > + device_remove_file(adev->dev, &dev_attr_vbios_version); > } > > /** > @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) > { > struct card_info *atom_card_info = > kzalloc(sizeof(struct card_info), GFP_KERNEL); > + int ret; > > if (!atom_card_info) > return -ENOMEM; > @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) > amdgpu_atombios_scratch_regs_init(adev); > amdgpu_atombios_allocate_fb_scratch(adev); > } > + > + ret = device_create_file(adev->dev, &dev_attr_vbios_version); > + if (ret) { > + DRM_ERROR("Failed to create device file for VBIOS version\n"); > + return ret; > + } > + > return 0; > } > > diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/amdgpu/atom.c > index d69aa2e..69500a8 100644 > --- a/drivers/gpu/drm/amd/amdgpu/atom.c > +++ b/drivers/gpu/drm/amd/amdgpu/atom.c > @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) > idx = 0x80; > > str = CSTR(idx); > - if (*str != '\0') > + if (*str != '\0') { > pr_info("ATOM BIOS: %s\n", str); > + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); > + } > + > > return ctx; > } > diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h b/drivers/gpu/drm/amd/amdgpu/atom.h > index ddd8045..a391709 100644 > --- a/drivers/gpu/drm/amd/amdgpu/atom.h > +++ b/drivers/gpu/drm/amd/amdgpu/atom.h > @@ -140,6 +140,7 @@ struct atom_context { > int io_mode; > uint32_t *scratch; > int scratch_size_bytes; > + char vbios_version[20]; > }; > > extern int amdgpu_atom_debug; _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <1e64a642-411e-ea58-1639-390e4c3cd75d-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>]
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <1e64a642-411e-ea58-1639-390e4c3cd75d-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> @ 2017-08-24 9:58 ` Russell, Kent [not found] ` <BN6PR1201MB01801A32C06AE0DA6634F440859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Russell, Kent @ 2017-08-24 9:58 UTC (permalink / raw) To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! Kent -----Original Message----- From: Christian König [mailto:deathsimple@vodafone.de] Sent: Thursday, August 24, 2017 2:22 AM To: Russell, Kent; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS Am 23.08.2017 um 20:12 schrieb Kent Russell: > This won't change after initialization, so we can add the information > when we parse the atombios information. This ensures that we can find > out the VBIOS, even when the dmesg buffer fills up, and makes it > easier to associate which VBIOS is for which GPU on mGPU > configurations. Set the size to 20 characters in case of some weird > VBIOS suffix that exceeds the expected 17 character format (3-8-3\0) Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. Christian. > > Signed-off-by: Kent Russell <kent.russell@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ > drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- > drivers/gpu/drm/amd/amdgpu/atom.h | 1 + > 3 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index a1f9424..f40be71 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) > return r; > } > > +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct drm_device *ddev = dev_get_drvdata(dev); > + struct amdgpu_device *adev = ddev->dev_private; > + struct atom_context *ctx = adev->mode_info.atom_context; > + > + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); } > + > +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, > + NULL); > + > /** > * amdgpu_atombios_fini - free the driver info and callbacks for atombios > * > @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) > adev->mode_info.atom_context = NULL; > kfree(adev->mode_info.atom_card_info); > adev->mode_info.atom_card_info = NULL; > + device_remove_file(adev->dev, &dev_attr_vbios_version); > } > > /** > @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) > { > struct card_info *atom_card_info = > kzalloc(sizeof(struct card_info), GFP_KERNEL); > + int ret; > > if (!atom_card_info) > return -ENOMEM; > @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) > amdgpu_atombios_scratch_regs_init(adev); > amdgpu_atombios_allocate_fb_scratch(adev); > } > + > + ret = device_create_file(adev->dev, &dev_attr_vbios_version); > + if (ret) { > + DRM_ERROR("Failed to create device file for VBIOS version\n"); > + return ret; > + } > + > return 0; > } > > diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c > b/drivers/gpu/drm/amd/amdgpu/atom.c > index d69aa2e..69500a8 100644 > --- a/drivers/gpu/drm/amd/amdgpu/atom.c > +++ b/drivers/gpu/drm/amd/amdgpu/atom.c > @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) > idx = 0x80; > > str = CSTR(idx); > - if (*str != '\0') > + if (*str != '\0') { > pr_info("ATOM BIOS: %s\n", str); > + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); > + } > + > > return ctx; > } > diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h > b/drivers/gpu/drm/amd/amdgpu/atom.h > index ddd8045..a391709 100644 > --- a/drivers/gpu/drm/amd/amdgpu/atom.h > +++ b/drivers/gpu/drm/amd/amdgpu/atom.h > @@ -140,6 +140,7 @@ struct atom_context { > int io_mode; > uint32_t *scratch; > int scratch_size_bytes; > + char vbios_version[20]; > }; > > extern int amdgpu_atom_debug; _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <BN6PR1201MB01801A32C06AE0DA6634F440859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR1201MB01801A32C06AE0DA6634F440859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> @ 2017-08-24 12:55 ` Alex Deucher [not found] ` <CADnq5_PD3s8HHPtr49ZJ3DXPVXgP5xyvFZj_YTzsGCi=cnOUKA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Alex Deucher @ 2017-08-24 12:55 UTC (permalink / raw) To: Russell, Kent Cc: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote: > No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! > While you are at it, can you expose the vbios binary itself via debugfs? That's been on by todo list for a while. Alex > Kent > > -----Original Message----- > From: Christian König [mailto:deathsimple@vodafone.de] > Sent: Thursday, August 24, 2017 2:22 AM > To: Russell, Kent; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > Am 23.08.2017 um 20:12 schrieb Kent Russell: >> This won't change after initialization, so we can add the information >> when we parse the atombios information. This ensures that we can find >> out the VBIOS, even when the dmesg buffer fills up, and makes it >> easier to associate which VBIOS is for which GPU on mGPU >> configurations. Set the size to 20 characters in case of some weird >> VBIOS suffix that exceeds the expected 17 character format (3-8-3\0) > > Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. > > Christian. > >> >> Signed-off-by: Kent Russell <kent.russell@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ >> drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- >> drivers/gpu/drm/amd/amdgpu/atom.h | 1 + >> 3 files changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index a1f9424..f40be71 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) >> return r; >> } >> >> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct drm_device *ddev = dev_get_drvdata(dev); >> + struct amdgpu_device *adev = ddev->dev_private; >> + struct atom_context *ctx = adev->mode_info.atom_context; >> + >> + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); } >> + >> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, >> + NULL); >> + >> /** >> * amdgpu_atombios_fini - free the driver info and callbacks for atombios >> * >> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) >> adev->mode_info.atom_context = NULL; >> kfree(adev->mode_info.atom_card_info); >> adev->mode_info.atom_card_info = NULL; >> + device_remove_file(adev->dev, &dev_attr_vbios_version); >> } >> >> /** >> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >> { >> struct card_info *atom_card_info = >> kzalloc(sizeof(struct card_info), GFP_KERNEL); >> + int ret; >> >> if (!atom_card_info) >> return -ENOMEM; >> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >> amdgpu_atombios_scratch_regs_init(adev); >> amdgpu_atombios_allocate_fb_scratch(adev); >> } >> + >> + ret = device_create_file(adev->dev, &dev_attr_vbios_version); >> + if (ret) { >> + DRM_ERROR("Failed to create device file for VBIOS version\n"); >> + return ret; >> + } >> + >> return 0; >> } >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c >> b/drivers/gpu/drm/amd/amdgpu/atom.c >> index d69aa2e..69500a8 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/atom.c >> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c >> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) >> idx = 0x80; >> >> str = CSTR(idx); >> - if (*str != '\0') >> + if (*str != '\0') { >> pr_info("ATOM BIOS: %s\n", str); >> + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); >> + } >> + >> >> return ctx; >> } >> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h >> b/drivers/gpu/drm/amd/amdgpu/atom.h >> index ddd8045..a391709 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/atom.h >> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h >> @@ -140,6 +140,7 @@ struct atom_context { >> int io_mode; >> uint32_t *scratch; >> int scratch_size_bytes; >> + char vbios_version[20]; >> }; >> >> extern int amdgpu_atom_debug; > > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <CADnq5_PD3s8HHPtr49ZJ3DXPVXgP5xyvFZj_YTzsGCi=cnOUKA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <CADnq5_PD3s8HHPtr49ZJ3DXPVXgP5xyvFZj_YTzsGCi=cnOUKA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-08-24 13:06 ` Russell, Kent [not found] ` <BN6PR1201MB018010027CA78E203F629BA7859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Russell, Kent @ 2017-08-24 13:06 UTC (permalink / raw) To: Alex Deucher Cc: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW I can definitely add that as well. Kent -----Original Message----- From: Alex Deucher [mailto:alexdeucher@gmail.com] Sent: Thursday, August 24, 2017 8:56 AM To: Russell, Kent Cc: Christian König; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote: > No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! > While you are at it, can you expose the vbios binary itself via debugfs? That's been on by todo list for a while. Alex > Kent > > -----Original Message----- > From: Christian König [mailto:deathsimple@vodafone.de] > Sent: Thursday, August 24, 2017 2:22 AM > To: Russell, Kent; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > Am 23.08.2017 um 20:12 schrieb Kent Russell: >> This won't change after initialization, so we can add the information >> when we parse the atombios information. This ensures that we can find >> out the VBIOS, even when the dmesg buffer fills up, and makes it >> easier to associate which VBIOS is for which GPU on mGPU >> configurations. Set the size to 20 characters in case of some weird >> VBIOS suffix that exceeds the expected 17 character format (3-8-3\0) > > Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. > > Christian. > >> >> Signed-off-by: Kent Russell <kent.russell@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ >> drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- >> drivers/gpu/drm/amd/amdgpu/atom.h | 1 + >> 3 files changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index a1f9424..f40be71 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) >> return r; >> } >> >> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) { >> + struct drm_device *ddev = dev_get_drvdata(dev); >> + struct amdgpu_device *adev = ddev->dev_private; >> + struct atom_context *ctx = adev->mode_info.atom_context; >> + >> + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); } >> + >> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, >> + NULL); >> + >> /** >> * amdgpu_atombios_fini - free the driver info and callbacks for atombios >> * >> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) >> adev->mode_info.atom_context = NULL; >> kfree(adev->mode_info.atom_card_info); >> adev->mode_info.atom_card_info = NULL; >> + device_remove_file(adev->dev, &dev_attr_vbios_version); >> } >> >> /** >> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >> { >> struct card_info *atom_card_info = >> kzalloc(sizeof(struct card_info), GFP_KERNEL); >> + int ret; >> >> if (!atom_card_info) >> return -ENOMEM; >> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >> amdgpu_atombios_scratch_regs_init(adev); >> amdgpu_atombios_allocate_fb_scratch(adev); >> } >> + >> + ret = device_create_file(adev->dev, &dev_attr_vbios_version); >> + if (ret) { >> + DRM_ERROR("Failed to create device file for VBIOS version\n"); >> + return ret; >> + } >> + >> return 0; >> } >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c >> b/drivers/gpu/drm/amd/amdgpu/atom.c >> index d69aa2e..69500a8 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/atom.c >> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c >> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) >> idx = 0x80; >> >> str = CSTR(idx); >> - if (*str != '\0') >> + if (*str != '\0') { >> pr_info("ATOM BIOS: %s\n", str); >> + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); >> + } >> + >> >> return ctx; >> } >> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h >> b/drivers/gpu/drm/amd/amdgpu/atom.h >> index ddd8045..a391709 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/atom.h >> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h >> @@ -140,6 +140,7 @@ struct atom_context { >> int io_mode; >> uint32_t *scratch; >> int scratch_size_bytes; >> + char vbios_version[20]; >> }; >> >> extern int amdgpu_atom_debug; > > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <BN6PR1201MB018010027CA78E203F629BA7859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR1201MB018010027CA78E203F629BA7859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> @ 2017-08-24 15:35 ` Kuehling, Felix [not found] ` <DM5PR1201MB023503F29269CEC80DC7DAD8929A0-grEf7a3NxMBd8L2jMOIKKmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Kuehling, Felix @ 2017-08-24 15:35 UTC (permalink / raw) To: Russell, Kent, Alex Deucher Cc: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs? I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi. Regards, Felix ________________________________________ From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Russell, Kent <Kent.Russell@amd.com> Sent: Thursday, August 24, 2017 9:06:22 AM To: Alex Deucher Cc: Christian König; amd-gfx@lists.freedesktop.org Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS I can definitely add that as well. Kent -----Original Message----- From: Alex Deucher [mailto:alexdeucher@gmail.com] Sent: Thursday, August 24, 2017 8:56 AM To: Russell, Kent Cc: Christian König; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote: > No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! > While you are at it, can you expose the vbios binary itself via debugfs? That's been on by todo list for a while. Alex > Kent > > -----Original Message----- > From: Christian König [mailto:deathsimple@vodafone.de] > Sent: Thursday, August 24, 2017 2:22 AM > To: Russell, Kent; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > Am 23.08.2017 um 20:12 schrieb Kent Russell: >> This won't change after initialization, so we can add the information >> when we parse the atombios information. This ensures that we can find >> out the VBIOS, even when the dmesg buffer fills up, and makes it >> easier to associate which VBIOS is for which GPU on mGPU >> configurations. Set the size to 20 characters in case of some weird >> VBIOS suffix that exceeds the expected 17 character format (3-8-3\0) > > Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. > > Christian. > >> >> Signed-off-by: Kent Russell <kent.russell@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ >> drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- >> drivers/gpu/drm/amd/amdgpu/atom.h | 1 + >> 3 files changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index a1f9424..f40be71 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) >> return r; >> } >> >> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) { >> + struct drm_device *ddev = dev_get_drvdata(dev); >> + struct amdgpu_device *adev = ddev->dev_private; >> + struct atom_context *ctx = adev->mode_info.atom_context; >> + >> + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); } >> + >> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, >> + NULL); >> + >> /** >> * amdgpu_atombios_fini - free the driver info and callbacks for atombios >> * >> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) >> adev->mode_info.atom_context = NULL; >> kfree(adev->mode_info.atom_card_info); >> adev->mode_info.atom_card_info = NULL; >> + device_remove_file(adev->dev, &dev_attr_vbios_version); >> } >> >> /** >> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >> { >> struct card_info *atom_card_info = >> kzalloc(sizeof(struct card_info), GFP_KERNEL); >> + int ret; >> >> if (!atom_card_info) >> return -ENOMEM; >> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >> amdgpu_atombios_scratch_regs_init(adev); >> amdgpu_atombios_allocate_fb_scratch(adev); >> } >> + >> + ret = device_create_file(adev->dev, &dev_attr_vbios_version); >> + if (ret) { >> + DRM_ERROR("Failed to create device file for VBIOS version\n"); >> + return ret; >> + } >> + >> return 0; >> } >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c >> b/drivers/gpu/drm/amd/amdgpu/atom.c >> index d69aa2e..69500a8 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/atom.c >> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c >> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) >> idx = 0x80; >> >> str = CSTR(idx); >> - if (*str != '\0') >> + if (*str != '\0') { >> pr_info("ATOM BIOS: %s\n", str); >> + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); >> + } >> + >> >> return ctx; >> } >> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h >> b/drivers/gpu/drm/amd/amdgpu/atom.h >> index ddd8045..a391709 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/atom.h >> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h >> @@ -140,6 +140,7 @@ struct atom_context { >> int io_mode; >> uint32_t *scratch; >> int scratch_size_bytes; >> + char vbios_version[20]; >> }; >> >> extern int amdgpu_atom_debug; > > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <DM5PR1201MB023503F29269CEC80DC7DAD8929A0-grEf7a3NxMBd8L2jMOIKKmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <DM5PR1201MB023503F29269CEC80DC7DAD8929A0-grEf7a3NxMBd8L2jMOIKKmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> @ 2017-08-24 15:39 ` Alex Deucher [not found] ` <CADnq5_Piyp7mtHi+4GZ0pUw1kMNT68ZVyA6jp1TR4VhbCjWAOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Alex Deucher @ 2017-08-24 15:39 UTC (permalink / raw) To: Kuehling, Felix Cc: Christian König, Russell, Kent, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote: > Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs? > > I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi. > Ah, ok, sysfs is fine in that case. I thought this was just general debugging stuff. Alex > Regards, > Felix > ________________________________________ > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Russell, Kent <Kent.Russell@amd.com> > Sent: Thursday, August 24, 2017 9:06:22 AM > To: Alex Deucher > Cc: Christian König; amd-gfx@lists.freedesktop.org > Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > I can definitely add that as well. > > Kent > > -----Original Message----- > From: Alex Deucher [mailto:alexdeucher@gmail.com] > Sent: Thursday, August 24, 2017 8:56 AM > To: Russell, Kent > Cc: Christian König; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote: >> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! >> > > While you are at it, can you expose the vbios binary itself via debugfs? That's been on by todo list for a while. > > Alex > >> Kent >> >> -----Original Message----- >> From: Christian König [mailto:deathsimple@vodafone.de] >> Sent: Thursday, August 24, 2017 2:22 AM >> To: Russell, Kent; amd-gfx@lists.freedesktop.org >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> Am 23.08.2017 um 20:12 schrieb Kent Russell: >>> This won't change after initialization, so we can add the information >>> when we parse the atombios information. This ensures that we can find >>> out the VBIOS, even when the dmesg buffer fills up, and makes it >>> easier to associate which VBIOS is for which GPU on mGPU >>> configurations. Set the size to 20 characters in case of some weird >>> VBIOS suffix that exceeds the expected 17 character format (3-8-3\0) >> >> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. >> >> Christian. >> >>> >>> Signed-off-by: Kent Russell <kent.russell@amd.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ >>> drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- >>> drivers/gpu/drm/amd/amdgpu/atom.h | 1 + >>> 3 files changed, 28 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> index a1f9424..f40be71 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) >>> return r; >>> } >>> >>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) { >>> + struct drm_device *ddev = dev_get_drvdata(dev); >>> + struct amdgpu_device *adev = ddev->dev_private; >>> + struct atom_context *ctx = adev->mode_info.atom_context; >>> + >>> + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); } >>> + >>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, >>> + NULL); >>> + >>> /** >>> * amdgpu_atombios_fini - free the driver info and callbacks for atombios >>> * >>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) >>> adev->mode_info.atom_context = NULL; >>> kfree(adev->mode_info.atom_card_info); >>> adev->mode_info.atom_card_info = NULL; >>> + device_remove_file(adev->dev, &dev_attr_vbios_version); >>> } >>> >>> /** >>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>> { >>> struct card_info *atom_card_info = >>> kzalloc(sizeof(struct card_info), GFP_KERNEL); >>> + int ret; >>> >>> if (!atom_card_info) >>> return -ENOMEM; >>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>> amdgpu_atombios_scratch_regs_init(adev); >>> amdgpu_atombios_allocate_fb_scratch(adev); >>> } >>> + >>> + ret = device_create_file(adev->dev, &dev_attr_vbios_version); >>> + if (ret) { >>> + DRM_ERROR("Failed to create device file for VBIOS version\n"); >>> + return ret; >>> + } >>> + >>> return 0; >>> } >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c >>> b/drivers/gpu/drm/amd/amdgpu/atom.c >>> index d69aa2e..69500a8 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c >>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) >>> idx = 0x80; >>> >>> str = CSTR(idx); >>> - if (*str != '\0') >>> + if (*str != '\0') { >>> pr_info("ATOM BIOS: %s\n", str); >>> + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); >>> + } >>> + >>> >>> return ctx; >>> } >>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h >>> b/drivers/gpu/drm/amd/amdgpu/atom.h >>> index ddd8045..a391709 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h >>> @@ -140,6 +140,7 @@ struct atom_context { >>> int io_mode; >>> uint32_t *scratch; >>> int scratch_size_bytes; >>> + char vbios_version[20]; >>> }; >>> >>> extern int amdgpu_atom_debug; >> >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <CADnq5_Piyp7mtHi+4GZ0pUw1kMNT68ZVyA6jp1TR4VhbCjWAOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <CADnq5_Piyp7mtHi+4GZ0pUw1kMNT68ZVyA6jp1TR4VhbCjWAOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-08-24 16:37 ` Russell, Kent [not found] ` <BN6PR1201MB01804846B5629D28766FDFA0859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Russell, Kent @ 2017-08-24 16:37 UTC (permalink / raw) To: Alex Deucher, Kuehling, Felix Cc: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that. Kent -----Original Message----- From: Alex Deucher [mailto:alexdeucher@gmail.com] Sent: Thursday, August 24, 2017 11:39 AM To: Kuehling, Felix Cc: Russell, Kent; Christian König; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote: > Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs? > > I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi. > Ah, ok, sysfs is fine in that case. I thought this was just general debugging stuff. Alex > Regards, > Felix > ________________________________________ > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of > Russell, Kent <Kent.Russell@amd.com> > Sent: Thursday, August 24, 2017 9:06:22 AM > To: Alex Deucher > Cc: Christian König; amd-gfx@lists.freedesktop.org > Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > I can definitely add that as well. > > Kent > > -----Original Message----- > From: Alex Deucher [mailto:alexdeucher@gmail.com] > Sent: Thursday, August 24, 2017 8:56 AM > To: Russell, Kent > Cc: Christian König; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote: >> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! >> > > While you are at it, can you expose the vbios binary itself via debugfs? That's been on by todo list for a while. > > Alex > >> Kent >> >> -----Original Message----- >> From: Christian König [mailto:deathsimple@vodafone.de] >> Sent: Thursday, August 24, 2017 2:22 AM >> To: Russell, Kent; amd-gfx@lists.freedesktop.org >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> Am 23.08.2017 um 20:12 schrieb Kent Russell: >>> This won't change after initialization, so we can add the >>> information when we parse the atombios information. This ensures >>> that we can find out the VBIOS, even when the dmesg buffer fills up, >>> and makes it easier to associate which VBIOS is for which GPU on >>> mGPU configurations. Set the size to 20 characters in case of some >>> weird VBIOS suffix that exceeds the expected 17 character format >>> (3-8-3\0) >> >> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. >> >> Christian. >> >>> >>> Signed-off-by: Kent Russell <kent.russell@amd.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ >>> drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- >>> drivers/gpu/drm/amd/amdgpu/atom.h | 1 + >>> 3 files changed, 28 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> index a1f9424..f40be71 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) >>> return r; >>> } >>> >>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) { >>> + struct drm_device *ddev = dev_get_drvdata(dev); >>> + struct amdgpu_device *adev = ddev->dev_private; >>> + struct atom_context *ctx = adev->mode_info.atom_context; >>> + >>> + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); } >>> + >>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, >>> + NULL); >>> + >>> /** >>> * amdgpu_atombios_fini - free the driver info and callbacks for atombios >>> * >>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) >>> adev->mode_info.atom_context = NULL; >>> kfree(adev->mode_info.atom_card_info); >>> adev->mode_info.atom_card_info = NULL; >>> + device_remove_file(adev->dev, &dev_attr_vbios_version); >>> } >>> >>> /** >>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>> { >>> struct card_info *atom_card_info = >>> kzalloc(sizeof(struct card_info), GFP_KERNEL); >>> + int ret; >>> >>> if (!atom_card_info) >>> return -ENOMEM; >>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>> amdgpu_atombios_scratch_regs_init(adev); >>> amdgpu_atombios_allocate_fb_scratch(adev); >>> } >>> + >>> + ret = device_create_file(adev->dev, &dev_attr_vbios_version); >>> + if (ret) { >>> + DRM_ERROR("Failed to create device file for VBIOS version\n"); >>> + return ret; >>> + } >>> + >>> return 0; >>> } >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c >>> b/drivers/gpu/drm/amd/amdgpu/atom.c >>> index d69aa2e..69500a8 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c >>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) >>> idx = 0x80; >>> >>> str = CSTR(idx); >>> - if (*str != '\0') >>> + if (*str != '\0') { >>> pr_info("ATOM BIOS: %s\n", str); >>> + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); >>> + } >>> + >>> >>> return ctx; >>> } >>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h >>> b/drivers/gpu/drm/amd/amdgpu/atom.h >>> index ddd8045..a391709 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h >>> @@ -140,6 +140,7 @@ struct atom_context { >>> int io_mode; >>> uint32_t *scratch; >>> int scratch_size_bytes; >>> + char vbios_version[20]; >>> }; >>> >>> extern int amdgpu_atom_debug; >> >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <BN6PR1201MB01804846B5629D28766FDFA0859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR1201MB01804846B5629D28766FDFA0859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> @ 2017-08-24 18:10 ` Christian König [not found] ` <fa2dddb4-5ceb-87f3-6305-fb5f6e6e5c2d-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Christian König @ 2017-08-24 18:10 UTC (permalink / raw) To: Russell, Kent, Alex Deucher, Kuehling, Felix Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Actually the main difference is not root vs. user, but rather unstable vs stable interface. If you want a stable interface for an userspace tool you should use sysfs, if you don't care about that you should use debugfs. Christian. Am 24.08.2017 um 18:37 schrieb Russell, Kent: > We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that. > > Kent > > -----Original Message----- > From: Alex Deucher [mailto:alexdeucher@gmail.com] > Sent: Thursday, August 24, 2017 11:39 AM > To: Kuehling, Felix > Cc: Russell, Kent; Christian König; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote: >> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs? >> >> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi. >> > Ah, ok, sysfs is fine in that case. I thought this was just general debugging stuff. > > Alex > >> Regards, >> Felix >> ________________________________________ >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of >> Russell, Kent <Kent.Russell@amd.com> >> Sent: Thursday, August 24, 2017 9:06:22 AM >> To: Alex Deucher >> Cc: Christian König; amd-gfx@lists.freedesktop.org >> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> I can definitely add that as well. >> >> Kent >> >> -----Original Message----- >> From: Alex Deucher [mailto:alexdeucher@gmail.com] >> Sent: Thursday, August 24, 2017 8:56 AM >> To: Russell, Kent >> Cc: Christian König; amd-gfx@lists.freedesktop.org >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote: >>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! >>> >> While you are at it, can you expose the vbios binary itself via debugfs? That's been on by todo list for a while. >> >> Alex >> >>> Kent >>> >>> -----Original Message----- >>> From: Christian König [mailto:deathsimple@vodafone.de] >>> Sent: Thursday, August 24, 2017 2:22 AM >>> To: Russell, Kent; amd-gfx@lists.freedesktop.org >>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>> >>> Am 23.08.2017 um 20:12 schrieb Kent Russell: >>>> This won't change after initialization, so we can add the >>>> information when we parse the atombios information. This ensures >>>> that we can find out the VBIOS, even when the dmesg buffer fills up, >>>> and makes it easier to associate which VBIOS is for which GPU on >>>> mGPU configurations. Set the size to 20 characters in case of some >>>> weird VBIOS suffix that exceeds the expected 17 character format >>>> (3-8-3\0) >>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. >>> >>> Christian. >>> >>>> Signed-off-by: Kent Russell <kent.russell@amd.com> >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ >>>> drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- >>>> drivers/gpu/drm/amd/amdgpu/atom.h | 1 + >>>> 3 files changed, 28 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>> index a1f9424..f40be71 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) >>>> return r; >>>> } >>>> >>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, >>>> + struct device_attribute *attr, >>>> + char *buf) { >>>> + struct drm_device *ddev = dev_get_drvdata(dev); >>>> + struct amdgpu_device *adev = ddev->dev_private; >>>> + struct atom_context *ctx = adev->mode_info.atom_context; >>>> + >>>> + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); } >>>> + >>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, >>>> + NULL); >>>> + >>>> /** >>>> * amdgpu_atombios_fini - free the driver info and callbacks for atombios >>>> * >>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) >>>> adev->mode_info.atom_context = NULL; >>>> kfree(adev->mode_info.atom_card_info); >>>> adev->mode_info.atom_card_info = NULL; >>>> + device_remove_file(adev->dev, &dev_attr_vbios_version); >>>> } >>>> >>>> /** >>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>>> { >>>> struct card_info *atom_card_info = >>>> kzalloc(sizeof(struct card_info), GFP_KERNEL); >>>> + int ret; >>>> >>>> if (!atom_card_info) >>>> return -ENOMEM; >>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>>> amdgpu_atombios_scratch_regs_init(adev); >>>> amdgpu_atombios_allocate_fb_scratch(adev); >>>> } >>>> + >>>> + ret = device_create_file(adev->dev, &dev_attr_vbios_version); >>>> + if (ret) { >>>> + DRM_ERROR("Failed to create device file for VBIOS version\n"); >>>> + return ret; >>>> + } >>>> + >>>> return 0; >>>> } >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c >>>> b/drivers/gpu/drm/amd/amdgpu/atom.c >>>> index d69aa2e..69500a8 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c >>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) >>>> idx = 0x80; >>>> >>>> str = CSTR(idx); >>>> - if (*str != '\0') >>>> + if (*str != '\0') { >>>> pr_info("ATOM BIOS: %s\n", str); >>>> + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); >>>> + } >>>> + >>>> >>>> return ctx; >>>> } >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h >>>> b/drivers/gpu/drm/amd/amdgpu/atom.h >>>> index ddd8045..a391709 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h >>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h >>>> @@ -140,6 +140,7 @@ struct atom_context { >>>> int io_mode; >>>> uint32_t *scratch; >>>> int scratch_size_bytes; >>>> + char vbios_version[20]; >>>> }; >>>> >>>> extern int amdgpu_atom_debug; >>> >>> _______________________________________________ >>> amd-gfx mailing list >>> amd-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <fa2dddb4-5ceb-87f3-6305-fb5f6e6e5c2d-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <fa2dddb4-5ceb-87f3-6305-fb5f6e6e5c2d-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> @ 2017-08-24 21:30 ` Russell, Kent [not found] ` <BN6PR1201MB0180502DC4720A3C33D642DB859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Russell, Kent @ 2017-08-24 21:30 UTC (permalink / raw) To: Christian König, Alex Deucher, Kuehling, Felix Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW [-- Attachment #1.1: Type: text/plain, Size: 8751 bytes --] The plan is for the vbios version to be available through the ROCM-SMI utility. We have the GPU power usage listed in the debugfs currently. If they are both to be used for a userspace utility, should we be moving both of those to sysfs instead? Kent KENT RUSSELL Software Engineer | Vertical Workstation/Compute 1 Commerce Valley Drive East Markham, ON L3T 7X6 Canada O +(1) 289-695-2122 | Ext x72122 ________________________________ From: Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> Sent: Thursday, August 24, 2017 2:10:35 PM To: Russell, Kent; Alex Deucher; Kuehling, Felix Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS Actually the main difference is not root vs. user, but rather unstable vs stable interface. If you want a stable interface for an userspace tool you should use sysfs, if you don't care about that you should use debugfs. Christian. Am 24.08.2017 um 18:37 schrieb Russell, Kent: > We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that. > > Kent > > -----Original Message----- > From: Alex Deucher [mailto:alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] > Sent: Thursday, August 24, 2017 11:39 AM > To: Kuehling, Felix > Cc: Russell, Kent; Christian König; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org> wrote: >> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs? >> >> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi. >> > Ah, ok, sysfs is fine in that case. I thought this was just general debugging stuff. > > Alex > >> Regards, >> Felix >> ________________________________________ >> From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of >> Russell, Kent <Kent.Russell-5C7GfCeVMHo@public.gmane.org> >> Sent: Thursday, August 24, 2017 9:06:22 AM >> To: Alex Deucher >> Cc: Christian König; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> I can definitely add that as well. >> >> Kent >> >> -----Original Message----- >> From: Alex Deucher [mailto:alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] >> Sent: Thursday, August 24, 2017 8:56 AM >> To: Russell, Kent >> Cc: Christian König; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell-5C7GfCeVMHo@public.gmane.org> wrote: >>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! >>> >> While you are at it, can you expose the vbios binary itself via debugfs? That's been on by todo list for a while. >> >> Alex >> >>> Kent >>> >>> -----Original Message----- >>> From: Christian König [mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org] >>> Sent: Thursday, August 24, 2017 2:22 AM >>> To: Russell, Kent; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>> >>> Am 23.08.2017 um 20:12 schrieb Kent Russell: >>>> This won't change after initialization, so we can add the >>>> information when we parse the atombios information. This ensures >>>> that we can find out the VBIOS, even when the dmesg buffer fills up, >>>> and makes it easier to associate which VBIOS is for which GPU on >>>> mGPU configurations. Set the size to 20 characters in case of some >>>> weird VBIOS suffix that exceeds the expected 17 character format >>>> (3-8-3\0) >>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. >>> >>> Christian. >>> >>>> Signed-off-by: Kent Russell <kent.russell-5C7GfCeVMHo@public.gmane.org> >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ >>>> drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- >>>> drivers/gpu/drm/amd/amdgpu/atom.h | 1 + >>>> 3 files changed, 28 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>> index a1f9424..f40be71 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) >>>> return r; >>>> } >>>> >>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, >>>> + struct device_attribute *attr, >>>> + char *buf) { >>>> + struct drm_device *ddev = dev_get_drvdata(dev); >>>> + struct amdgpu_device *adev = ddev->dev_private; >>>> + struct atom_context *ctx = adev->mode_info.atom_context; >>>> + >>>> + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); } >>>> + >>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, >>>> + NULL); >>>> + >>>> /** >>>> * amdgpu_atombios_fini - free the driver info and callbacks for atombios >>>> * >>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) >>>> adev->mode_info.atom_context = NULL; >>>> kfree(adev->mode_info.atom_card_info); >>>> adev->mode_info.atom_card_info = NULL; >>>> + device_remove_file(adev->dev, &dev_attr_vbios_version); >>>> } >>>> >>>> /** >>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>>> { >>>> struct card_info *atom_card_info = >>>> kzalloc(sizeof(struct card_info), GFP_KERNEL); >>>> + int ret; >>>> >>>> if (!atom_card_info) >>>> return -ENOMEM; >>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>>> amdgpu_atombios_scratch_regs_init(adev); >>>> amdgpu_atombios_allocate_fb_scratch(adev); >>>> } >>>> + >>>> + ret = device_create_file(adev->dev, &dev_attr_vbios_version); >>>> + if (ret) { >>>> + DRM_ERROR("Failed to create device file for VBIOS version\n"); >>>> + return ret; >>>> + } >>>> + >>>> return 0; >>>> } >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c >>>> b/drivers/gpu/drm/amd/amdgpu/atom.c >>>> index d69aa2e..69500a8 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c >>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) >>>> idx = 0x80; >>>> >>>> str = CSTR(idx); >>>> - if (*str != '\0') >>>> + if (*str != '\0') { >>>> pr_info("ATOM BIOS: %s\n", str); >>>> + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); >>>> + } >>>> + >>>> >>>> return ctx; >>>> } >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h >>>> b/drivers/gpu/drm/amd/amdgpu/atom.h >>>> index ddd8045..a391709 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h >>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h >>>> @@ -140,6 +140,7 @@ struct atom_context { >>>> int io_mode; >>>> uint32_t *scratch; >>>> int scratch_size_bytes; >>>> + char vbios_version[20]; >>>> }; >>>> >>>> extern int amdgpu_atom_debug; >>> >>> _______________________________________________ >>> amd-gfx mailing list >>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ > amd-gfx mailing list > amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx [-- Attachment #1.2: Type: text/html, Size: 14812 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <BN6PR1201MB0180502DC4720A3C33D642DB859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR1201MB0180502DC4720A3C33D642DB859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> @ 2017-08-25 12:36 ` Tom St Denis [not found] ` <4cec0dad-502e-68f2-68c9-b5d0103d7093-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Tom St Denis @ 2017-08-25 12:36 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Hi Kent, Loosely following this thread. Was there a decision on whether to leave this in debugfs or sysfs? I'd like to throw the version string in umr's config output (umr -c) :-) Cheers, Tom On 24/08/17 05:30 PM, Russell, Kent wrote: > The plan is for the vbios version to be available through the ROCM-SMI > utility. We have the GPU power usage listed in the debugfs currently. If > they are both to be used for a userspace utility, should we be moving > both of those to sysfs instead? > > Kent > > KENT RUSSELL > Software Engineer | Vertical Workstation/Compute > 1 Commerce Valley Drive East > Markham, ON L3T 7X6 > Canada > O +(1) 289-695-2122 | Ext x72122 > > > ------------------------------------------------------------------------ > *From:* Christian König <deathsimple@vodafone.de> > *Sent:* Thursday, August 24, 2017 2:10:35 PM > *To:* Russell, Kent; Alex Deucher; Kuehling, Felix > *Cc:* amd-gfx@lists.freedesktop.org > *Subject:* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > Actually the main difference is not root vs. user, but rather unstable > vs stable interface. > > If you want a stable interface for an userspace tool you should use > sysfs, if you don't care about that you should use debugfs. > > Christian. > > Am 24.08.2017 um 18:37 schrieb Russell, Kent: >> We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that. >> >> Kent >> >> -----Original Message----- >> From: Alex Deucher [mailto:alexdeucher@gmail.com] >> Sent: Thursday, August 24, 2017 11:39 AM >> To: Kuehling, Felix >> Cc: Russell, Kent; Christian König; amd-gfx@lists.freedesktop.org >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote: >>> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs? >>> >>> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi. >>> >> Ah, ok, sysfs is fine in that case. I thought this was just general debugging stuff. >> >> Alex >> >>> Regards, >>> Felix >>> ________________________________________ >>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of >>> Russell, Kent <Kent.Russell@amd.com> >>> Sent: Thursday, August 24, 2017 9:06:22 AM >>> To: Alex Deucher >>> Cc: Christian König; amd-gfx@lists.freedesktop.org >>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>> >>> I can definitely add that as well. >>> >>> Kent >>> >>> -----Original Message----- >>> From: Alex Deucher [mailto:alexdeucher@gmail.com] >>> Sent: Thursday, August 24, 2017 8:56 AM >>> To: Russell, Kent >>> Cc: Christian König; amd-gfx@lists.freedesktop.org >>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>> >>> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote: >>>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! >>>> >>> While you are at it, can you expose the vbios binary itself via debugfs? That's been on by todo list for a while. >>> >>> Alex >>> >>>> Kent >>>> >>>> -----Original Message----- >>>> From: Christian König [mailto:deathsimple@vodafone.de] >>>> Sent: Thursday, August 24, 2017 2:22 AM >>>> To: Russell, Kent; amd-gfx@lists.freedesktop.org >>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>>> >>>> Am 23.08.2017 um 20:12 schrieb Kent Russell: >>>>> This won't change after initialization, so we can add the >>>>> information when we parse the atombios information. This ensures >>>>> that we can find out the VBIOS, even when the dmesg buffer fills up, >>>>> and makes it easier to associate which VBIOS is for which GPU on >>>>> mGPU configurations. Set the size to 20 characters in case of some >>>>> weird VBIOS suffix that exceeds the expected 17 character format >>>>> (3-8-3\0) >>>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. >>>> >>>> Christian. >>>> >>>>> Signed-off-by: Kent Russell <kent.russell@amd.com> >>>>> --- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ >>>>> drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- >>>>> drivers/gpu/drm/amd/amdgpu/atom.h | 1 + >>>>> 3 files changed, 28 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> index a1f9424..f40be71 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) >>>>> return r; >>>>> } >>>>> >>>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, >>>>> + struct device_attribute *attr, >>>>> + char *buf) { >>>>> + struct drm_device *ddev = dev_get_drvdata(dev); >>>>> + struct amdgpu_device *adev = ddev->dev_private; >>>>> + struct atom_context *ctx = adev->mode_info.atom_context; >>>>> + >>>>> + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); } >>>>> + >>>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, >>>>> + NULL); >>>>> + >>>>> /** >>>>> * amdgpu_atombios_fini - free the driver info and callbacks for atombios >>>>> * >>>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) >>>>> adev->mode_info.atom_context = NULL; >>>>> kfree(adev->mode_info.atom_card_info); >>>>> adev->mode_info.atom_card_info = NULL; >>>>> + device_remove_file(adev->dev, &dev_attr_vbios_version); >>>>> } >>>>> >>>>> /** >>>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>>>> { >>>>> struct card_info *atom_card_info = >>>>> kzalloc(sizeof(struct card_info), GFP_KERNEL); >>>>> + int ret; >>>>> >>>>> if (!atom_card_info) >>>>> return -ENOMEM; >>>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>>>> amdgpu_atombios_scratch_regs_init(adev); >>>>> amdgpu_atombios_allocate_fb_scratch(adev); >>>>> } >>>>> + >>>>> + ret = device_create_file(adev->dev, &dev_attr_vbios_version); >>>>> + if (ret) { >>>>> + DRM_ERROR("Failed to create device file for VBIOS version\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> return 0; >>>>> } >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c >>>>> b/drivers/gpu/drm/amd/amdgpu/atom.c >>>>> index d69aa2e..69500a8 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c >>>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) >>>>> idx = 0x80; >>>>> >>>>> str = CSTR(idx); >>>>> - if (*str != '\0') >>>>> + if (*str != '\0') { >>>>> pr_info("ATOM BIOS: %s\n", str); >>>>> + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); >>>>> + } >>>>> + >>>>> >>>>> return ctx; >>>>> } >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h >>>>> b/drivers/gpu/drm/amd/amdgpu/atom.h >>>>> index ddd8045..a391709 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h >>>>> @@ -140,6 +140,7 @@ struct atom_context { >>>>> int io_mode; >>>>> uint32_t *scratch; >>>>> int scratch_size_bytes; >>>>> + char vbios_version[20]; >>>>> }; >>>>> >>>>> extern int amdgpu_atom_debug; >>>> >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>> _______________________________________________ >>> amd-gfx mailing list >>> amd-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > > > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <4cec0dad-502e-68f2-68c9-b5d0103d7093-5C7GfCeVMHo@public.gmane.org>]
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <4cec0dad-502e-68f2-68c9-b5d0103d7093-5C7GfCeVMHo@public.gmane.org> @ 2017-08-25 12:40 ` Russell, Kent [not found] ` <BN6PR1201MB01803DD5EBD0049397F4E9F0859B0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Russell, Kent @ 2017-08-25 12:40 UTC (permalink / raw) To: StDenis, Tom, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW The decision to move the VBIOS version to sysfs has been decided. I didn't get any confirmation regarding moving the GPU power usage, or the actual VBIOS dump, as the dump seems to make more sense in debugfs. I'd like to move power usage to sysfs as the SMI tool needs root privileges just to read the value, which makes people cranky. I just want to make sure that it's alright to do so from the powers-that-be. Kent -----Original Message----- From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Tom St Denis Sent: Friday, August 25, 2017 8:37 AM To: amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS Hi Kent, Loosely following this thread. Was there a decision on whether to leave this in debugfs or sysfs? I'd like to throw the version string in umr's config output (umr -c) :-) Cheers, Tom On 24/08/17 05:30 PM, Russell, Kent wrote: > The plan is for the vbios version to be available through the ROCM-SMI > utility. We have the GPU power usage listed in the debugfs currently. > If they are both to be used for a userspace utility, should we be > moving both of those to sysfs instead? > > Kent > > KENT RUSSELL > Software Engineer | Vertical Workstation/Compute > 1 Commerce Valley Drive East > Markham, ON L3T 7X6 > Canada > O +(1) 289-695-2122 | Ext x72122 > > > ---------------------------------------------------------------------- > -- > *From:* Christian König <deathsimple@vodafone.de> > *Sent:* Thursday, August 24, 2017 2:10:35 PM > *To:* Russell, Kent; Alex Deucher; Kuehling, Felix > *Cc:* amd-gfx@lists.freedesktop.org > *Subject:* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS Actually > the main difference is not root vs. user, but rather unstable vs > stable interface. > > If you want a stable interface for an userspace tool you should use > sysfs, if you don't care about that you should use debugfs. > > Christian. > > Am 24.08.2017 um 18:37 schrieb Russell, Kent: >> We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that. >> >> Kent >> >> -----Original Message----- >> From: Alex Deucher [mailto:alexdeucher@gmail.com] >> Sent: Thursday, August 24, 2017 11:39 AM >> To: Kuehling, Felix >> Cc: Russell, Kent; Christian König; amd-gfx@lists.freedesktop.org >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote: >>> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs? >>> >>> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi. >>> >> Ah, ok, sysfs is fine in that case. I thought this was just general debugging stuff. >> >> Alex >> >>> Regards, >>> Felix >>> ________________________________________ >>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of >>> Russell, Kent <Kent.Russell@amd.com> >>> Sent: Thursday, August 24, 2017 9:06:22 AM >>> To: Alex Deucher >>> Cc: Christian König; amd-gfx@lists.freedesktop.org >>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>> >>> I can definitely add that as well. >>> >>> Kent >>> >>> -----Original Message----- >>> From: Alex Deucher [mailto:alexdeucher@gmail.com] >>> Sent: Thursday, August 24, 2017 8:56 AM >>> To: Russell, Kent >>> Cc: Christian König; amd-gfx@lists.freedesktop.org >>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>> >>> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote: >>>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! >>>> >>> While you are at it, can you expose the vbios binary itself via debugfs? That's been on by todo list for a while. >>> >>> Alex >>> >>>> Kent >>>> >>>> -----Original Message----- >>>> From: Christian König [mailto:deathsimple@vodafone.de] >>>> Sent: Thursday, August 24, 2017 2:22 AM >>>> To: Russell, Kent; amd-gfx@lists.freedesktop.org >>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>>> >>>> Am 23.08.2017 um 20:12 schrieb Kent Russell: >>>>> This won't change after initialization, so we can add the >>>>> information when we parse the atombios information. This ensures >>>>> that we can find out the VBIOS, even when the dmesg buffer fills >>>>> up, and makes it easier to associate which VBIOS is for which GPU >>>>> on mGPU configurations. Set the size to 20 characters in case of >>>>> some weird VBIOS suffix that exceeds the expected 17 character >>>>> format >>>>> (3-8-3\0) >>>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. >>>> >>>> Christian. >>>> >>>>> Signed-off-by: Kent Russell <kent.russell@amd.com> >>>>> --- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ >>>>> drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- >>>>> drivers/gpu/drm/amd/amdgpu/atom.h | 1 + >>>>> 3 files changed, 28 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> index a1f9424..f40be71 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) >>>>> return r; >>>>> } >>>>> >>>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, >>>>> + struct device_attribute *attr, >>>>> + char *buf) { >>>>> + struct drm_device *ddev = dev_get_drvdata(dev); >>>>> + struct amdgpu_device *adev = ddev->dev_private; >>>>> + struct atom_context *ctx = adev->mode_info.atom_context; >>>>> + >>>>> + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); >>>>> + } >>>>> + >>>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, >>>>> + NULL); >>>>> + >>>>> /** >>>>> * amdgpu_atombios_fini - free the driver info and callbacks for atombios >>>>> * >>>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) >>>>> adev->mode_info.atom_context = NULL; >>>>> kfree(adev->mode_info.atom_card_info); >>>>> adev->mode_info.atom_card_info = NULL; >>>>> + device_remove_file(adev->dev, &dev_attr_vbios_version); >>>>> } >>>>> >>>>> /** >>>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>>>> { >>>>> struct card_info *atom_card_info = >>>>> kzalloc(sizeof(struct card_info), GFP_KERNEL); >>>>> + int ret; >>>>> >>>>> if (!atom_card_info) >>>>> return -ENOMEM; >>>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>>>> amdgpu_atombios_scratch_regs_init(adev); >>>>> amdgpu_atombios_allocate_fb_scratch(adev); >>>>> } >>>>> + >>>>> + ret = device_create_file(adev->dev, &dev_attr_vbios_version); >>>>> + if (ret) { >>>>> + DRM_ERROR("Failed to create device file for VBIOS version\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> return 0; >>>>> } >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c >>>>> b/drivers/gpu/drm/amd/amdgpu/atom.c >>>>> index d69aa2e..69500a8 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c >>>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) >>>>> idx = 0x80; >>>>> >>>>> str = CSTR(idx); >>>>> - if (*str != '\0') >>>>> + if (*str != '\0') { >>>>> pr_info("ATOM BIOS: %s\n", str); >>>>> + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); >>>>> + } >>>>> + >>>>> >>>>> return ctx; >>>>> } >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h >>>>> b/drivers/gpu/drm/amd/amdgpu/atom.h >>>>> index ddd8045..a391709 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h >>>>> @@ -140,6 +140,7 @@ struct atom_context { >>>>> int io_mode; >>>>> uint32_t *scratch; >>>>> int scratch_size_bytes; >>>>> + char vbios_version[20]; >>>>> }; >>>>> >>>>> extern int amdgpu_atom_debug; >>>> >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>> _______________________________________________ >>> amd-gfx mailing list >>> amd-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > > > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <BN6PR1201MB01803DD5EBD0049397F4E9F0859B0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR1201MB01803DD5EBD0049397F4E9F0859B0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> @ 2017-08-25 12:56 ` Christian König [not found] ` <0a3d4d07-e3cd-6216-466b-30bc60c8a26a-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Christian König @ 2017-08-25 12:56 UTC (permalink / raw) To: Russell, Kent, StDenis, Tom, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Hi Kent, agree on the VBIOS dump file, that clearly belongs to debugsf. The power usage stuff I can't say much about cause I'm not deeply into this, but keep in mind the restriction for sysfs: 1. It's a stable interface. So it must be very well designed. 2. Only one value per file. I think the power stuff doesn't fulfill that requirement at the moment. Regards, Christian. Am 25.08.2017 um 14:40 schrieb Russell, Kent: > The decision to move the VBIOS version to sysfs has been decided. I didn't get any confirmation regarding moving the GPU power usage, or the actual VBIOS dump, as the dump seems to make more sense in debugfs. I'd like to move power usage to sysfs as the SMI tool needs root privileges just to read the value, which makes people cranky. I just want to make sure that it's alright to do so from the powers-that-be. > > Kent > > -----Original Message----- > From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Tom St Denis > Sent: Friday, August 25, 2017 8:37 AM > To: amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > Hi Kent, > > Loosely following this thread. Was there a decision on whether to leave this in debugfs or sysfs? > > I'd like to throw the version string in umr's config output (umr -c) :-) > > Cheers, > Tom > > On 24/08/17 05:30 PM, Russell, Kent wrote: >> The plan is for the vbios version to be available through the ROCM-SMI >> utility. We have the GPU power usage listed in the debugfs currently. >> If they are both to be used for a userspace utility, should we be >> moving both of those to sysfs instead? >> >> Kent >> >> KENT RUSSELL >> Software Engineer | Vertical Workstation/Compute >> 1 Commerce Valley Drive East >> Markham, ON L3T 7X6 >> Canada >> O +(1) 289-695-2122 | Ext x72122 >> >> >> ---------------------------------------------------------------------- >> -- >> *From:* Christian König <deathsimple@vodafone.de> >> *Sent:* Thursday, August 24, 2017 2:10:35 PM >> *To:* Russell, Kent; Alex Deucher; Kuehling, Felix >> *Cc:* amd-gfx@lists.freedesktop.org >> *Subject:* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS Actually >> the main difference is not root vs. user, but rather unstable vs >> stable interface. >> >> If you want a stable interface for an userspace tool you should use >> sysfs, if you don't care about that you should use debugfs. >> >> Christian. >> >> Am 24.08.2017 um 18:37 schrieb Russell, Kent: >>> We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that. >>> >>> Kent >>> >>> -----Original Message----- >>> From: Alex Deucher [mailto:alexdeucher@gmail.com] >>> Sent: Thursday, August 24, 2017 11:39 AM >>> To: Kuehling, Felix >>> Cc: Russell, Kent; Christian König; amd-gfx@lists.freedesktop.org >>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>> >>> On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote: >>>> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs? >>>> >>>> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi. >>>> >>> Ah, ok, sysfs is fine in that case. I thought this was just general debugging stuff. >>> >>> Alex >>> >>>> Regards, >>>> Felix >>>> ________________________________________ >>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of >>>> Russell, Kent <Kent.Russell@amd.com> >>>> Sent: Thursday, August 24, 2017 9:06:22 AM >>>> To: Alex Deucher >>>> Cc: Christian König; amd-gfx@lists.freedesktop.org >>>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>>> >>>> I can definitely add that as well. >>>> >>>> Kent >>>> >>>> -----Original Message----- >>>> From: Alex Deucher [mailto:alexdeucher@gmail.com] >>>> Sent: Thursday, August 24, 2017 8:56 AM >>>> To: Russell, Kent >>>> Cc: Christian König; amd-gfx@lists.freedesktop.org >>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>>> >>>> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote: >>>>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you! >>>>> >>>> While you are at it, can you expose the vbios binary itself via debugfs? That's been on by todo list for a while. >>>> >>>> Alex >>>> >>>>> Kent >>>>> >>>>> -----Original Message----- >>>>> From: Christian König [mailto:deathsimple@vodafone.de] >>>>> Sent: Thursday, August 24, 2017 2:22 AM >>>>> To: Russell, Kent; amd-gfx@lists.freedesktop.org >>>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>>>> >>>>> Am 23.08.2017 um 20:12 schrieb Kent Russell: >>>>>> This won't change after initialization, so we can add the >>>>>> information when we parse the atombios information. This ensures >>>>>> that we can find out the VBIOS, even when the dmesg buffer fills >>>>>> up, and makes it easier to associate which VBIOS is for which GPU >>>>>> on mGPU configurations. Set the size to 20 characters in case of >>>>>> some weird VBIOS suffix that exceeds the expected 17 character >>>>>> format >>>>>> (3-8-3\0) >>>>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs. >>>>> >>>>> Christian. >>>>> >>>>>> Signed-off-by: Kent Russell <kent.russell@amd.com> >>>>>> --- >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++ >>>>>> drivers/gpu/drm/amd/amdgpu/atom.c | 5 ++++- >>>>>> drivers/gpu/drm/amd/amdgpu/atom.h | 1 + >>>>>> 3 files changed, 28 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>>> index a1f9424..f40be71 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg) >>>>>> return r; >>>>>> } >>>>>> >>>>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, >>>>>> + struct device_attribute *attr, >>>>>> + char *buf) { >>>>>> + struct drm_device *ddev = dev_get_drvdata(dev); >>>>>> + struct amdgpu_device *adev = ddev->dev_private; >>>>>> + struct atom_context *ctx = adev->mode_info.atom_context; >>>>>> + >>>>>> + return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); >>>>>> + } >>>>>> + >>>>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, >>>>>> + NULL); >>>>>> + >>>>>> /** >>>>>> * amdgpu_atombios_fini - free the driver info and callbacks for atombios >>>>>> * >>>>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev) >>>>>> adev->mode_info.atom_context = NULL; >>>>>> kfree(adev->mode_info.atom_card_info); >>>>>> adev->mode_info.atom_card_info = NULL; >>>>>> + device_remove_file(adev->dev, &dev_attr_vbios_version); >>>>>> } >>>>>> >>>>>> /** >>>>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>>>>> { >>>>>> struct card_info *atom_card_info = >>>>>> kzalloc(sizeof(struct card_info), GFP_KERNEL); >>>>>> + int ret; >>>>>> >>>>>> if (!atom_card_info) >>>>>> return -ENOMEM; >>>>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev) >>>>>> amdgpu_atombios_scratch_regs_init(adev); >>>>>> amdgpu_atombios_allocate_fb_scratch(adev); >>>>>> } >>>>>> + >>>>>> + ret = device_create_file(adev->dev, &dev_attr_vbios_version); >>>>>> + if (ret) { >>>>>> + DRM_ERROR("Failed to create device file for VBIOS version\n"); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> return 0; >>>>>> } >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c >>>>>> b/drivers/gpu/drm/amd/amdgpu/atom.c >>>>>> index d69aa2e..69500a8 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c >>>>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios) >>>>>> idx = 0x80; >>>>>> >>>>>> str = CSTR(idx); >>>>>> - if (*str != '\0') >>>>>> + if (*str != '\0') { >>>>>> pr_info("ATOM BIOS: %s\n", str); >>>>>> + strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version)); >>>>>> + } >>>>>> + >>>>>> >>>>>> return ctx; >>>>>> } >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h >>>>>> b/drivers/gpu/drm/amd/amdgpu/atom.h >>>>>> index ddd8045..a391709 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h >>>>>> @@ -140,6 +140,7 @@ struct atom_context { >>>>>> int io_mode; >>>>>> uint32_t *scratch; >>>>>> int scratch_size_bytes; >>>>>> + char vbios_version[20]; >>>>>> }; >>>>>> >>>>>> extern int amdgpu_atom_debug; >>>>> _______________________________________________ >>>>> amd-gfx mailing list >>>>> amd-gfx@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>> _______________________________________________ >>> amd-gfx mailing list >>> amd-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> >> >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <0a3d4d07-e3cd-6216-466b-30bc60c8a26a-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <0a3d4d07-e3cd-6216-466b-30bc60c8a26a-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> @ 2017-08-25 12:57 ` Tom St Denis [not found] ` <22c4b9e2-67e2-bd0a-1147-6e896843783e-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Tom St Denis @ 2017-08-25 12:57 UTC (permalink / raw) To: Christian König, Russell, Kent, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 25/08/17 08:56 AM, Christian König wrote: > Hi Kent, > > agree on the VBIOS dump file, that clearly belongs to debugsf. > > The power usage stuff I can't say much about cause I'm not deeply into > this, but keep in mind the restriction for sysfs: > 1. It's a stable interface. So it must be very well designed. > 2. Only one value per file. I think the power stuff doesn't fulfill that > requirement at the moment. What "power" stuff are we talking about? The sensors interface or the pm_info or something else? Keep in mind umr uses the sensors debugfs file in --top mode. Tom _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <22c4b9e2-67e2-bd0a-1147-6e896843783e-5C7GfCeVMHo@public.gmane.org>]
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <22c4b9e2-67e2-bd0a-1147-6e896843783e-5C7GfCeVMHo@public.gmane.org> @ 2017-08-25 12:59 ` Russell, Kent [not found] ` <BN6PR1201MB01806ACF078801D91460C110859B0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Russell, Kent @ 2017-08-25 12:59 UTC (permalink / raw) To: StDenis, Tom, Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW There is GPU Power usage reported through amdgpu_pm_info, which also has some other information as well. I'd like that in sysfs, but I am unsure if we are allowed to due to the other information reported there. Kent -----Original Message----- From: StDenis, Tom Sent: Friday, August 25, 2017 8:58 AM To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS On 25/08/17 08:56 AM, Christian König wrote: > Hi Kent, > > agree on the VBIOS dump file, that clearly belongs to debugsf. > > The power usage stuff I can't say much about cause I'm not deeply into > this, but keep in mind the restriction for sysfs: > 1. It's a stable interface. So it must be very well designed. > 2. Only one value per file. I think the power stuff doesn't fulfill > that requirement at the moment. What "power" stuff are we talking about? The sensors interface or the pm_info or something else? Keep in mind umr uses the sensors debugfs file in --top mode. Tom _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <BN6PR1201MB01806ACF078801D91460C110859B0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR1201MB01806ACF078801D91460C110859B0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> @ 2017-08-25 13:05 ` Tom St Denis [not found] ` <ebeb7962-c55c-9ba3-6969-6f7cf9934a5b-5C7GfCeVMHo@public.gmane.org> 2017-08-25 14:36 ` Alex Deucher 2017-08-25 15:56 ` Deucher, Alexander 2 siblings, 1 reply; 29+ messages in thread From: Tom St Denis @ 2017-08-25 13:05 UTC (permalink / raw) To: Russell, Kent, Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 25/08/17 08:59 AM, Russell, Kent wrote: > There is GPU Power usage reported through amdgpu_pm_info, which also has some other information as well. I'd like that in sysfs, but I am unsure if we are allowed to due to the other information reported there. I thought the sensors were added to the DRM ioctl but apparently not. Seems like that might be a bit easier place to put them than creating sysfs entries for every single sensor (it also means as new sensors are added you have to circle around back to add yet more sysfs entries). Tom > > Kent > > -----Original Message----- > From: StDenis, Tom > Sent: Friday, August 25, 2017 8:58 AM > To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > On 25/08/17 08:56 AM, Christian König wrote: >> Hi Kent, >> >> agree on the VBIOS dump file, that clearly belongs to debugsf. >> >> The power usage stuff I can't say much about cause I'm not deeply into >> this, but keep in mind the restriction for sysfs: >> 1. It's a stable interface. So it must be very well designed. >> 2. Only one value per file. I think the power stuff doesn't fulfill >> that requirement at the moment. > > What "power" stuff are we talking about? The sensors interface or the pm_info or something else? > > Keep in mind umr uses the sensors debugfs file in --top mode. > > Tom > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <ebeb7962-c55c-9ba3-6969-6f7cf9934a5b-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <ebeb7962-c55c-9ba3-6969-6f7cf9934a5b-5C7GfCeVMHo@public.gmane.org> @ 2017-08-25 13:32 ` Tom St Denis 2017-08-25 13:34 ` Tom St Denis 1 sibling, 0 replies; 29+ messages in thread From: Tom St Denis @ 2017-08-25 13:32 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 25/08/17 09:05 AM, Tom St Denis wrote: > On 25/08/17 08:59 AM, Russell, Kent wrote: >> There is GPU Power usage reported through amdgpu_pm_info, which also >> has some other information as well. I'd like that in sysfs, but I am >> unsure if we are allowed to due to the other information reported there. > > I thought the sensors were added to the DRM ioctl but apparently not. No it was, under AMDGPU_INFO you request info->query == AMDGPU_INFO_SENSOR and then specify which sensor you want in info->sensor_info.type (see amdgpu_kms.c). So you can already read this from a normal user process who has an open DRM file handle. Tom _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <ebeb7962-c55c-9ba3-6969-6f7cf9934a5b-5C7GfCeVMHo@public.gmane.org> 2017-08-25 13:32 ` Tom St Denis @ 2017-08-25 13:34 ` Tom St Denis [not found] ` <cc70864c-1106-1668-d80f-49cfbb6a6325-5C7GfCeVMHo@public.gmane.org> 1 sibling, 1 reply; 29+ messages in thread From: Tom St Denis @ 2017-08-25 13:34 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 25/08/17 09:05 AM, Tom St Denis wrote: > On 25/08/17 08:59 AM, Russell, Kent wrote: >> There is GPU Power usage reported through amdgpu_pm_info, which also >> has some other information as well. I'd like that in sysfs, but I am >> unsure if we are allowed to due to the other information reported there. > > I thought the sensors were added to the DRM ioctl but apparently not. Nope, it was actually (see amdgpu_kms.c). Under an AMDGPU_INFO command you ask for AMDGPU_INFO_SENSOR and then fill out sensor_info.type. Tom _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <cc70864c-1106-1668-d80f-49cfbb6a6325-5C7GfCeVMHo@public.gmane.org>]
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <cc70864c-1106-1668-d80f-49cfbb6a6325-5C7GfCeVMHo@public.gmane.org> @ 2017-08-25 13:36 ` Russell, Kent 0 siblings, 0 replies; 29+ messages in thread From: Russell, Kent @ 2017-08-25 13:36 UTC (permalink / raw) To: StDenis, Tom, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Right. And according to Christian, it doesn't fit the format anyways, so power usage can stay. I'm working on the patch to move the vbios version to sysfs from debugfs. Should be up shortly. Maybe I can just make a new sysfs file for Average GPU usage by itself. Another task for another day. But I won't be moving the sensors over. Just vbios version for now, maybe average GPU power usage later. Kent -----Original Message----- From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Tom St Denis Sent: Friday, August 25, 2017 9:34 AM To: amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS On 25/08/17 09:05 AM, Tom St Denis wrote: > On 25/08/17 08:59 AM, Russell, Kent wrote: >> There is GPU Power usage reported through amdgpu_pm_info, which also >> has some other information as well. I'd like that in sysfs, but I am >> unsure if we are allowed to due to the other information reported there. > > I thought the sensors were added to the DRM ioctl but apparently not. Nope, it was actually (see amdgpu_kms.c). Under an AMDGPU_INFO command you ask for AMDGPU_INFO_SENSOR and then fill out sensor_info.type. Tom _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR1201MB01806ACF078801D91460C110859B0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 2017-08-25 13:05 ` Tom St Denis @ 2017-08-25 14:36 ` Alex Deucher 2017-08-25 15:56 ` Deucher, Alexander 2 siblings, 0 replies; 29+ messages in thread From: Alex Deucher @ 2017-08-25 14:36 UTC (permalink / raw) To: Russell, Kent Cc: StDenis, Tom, Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On Fri, Aug 25, 2017 at 8:59 AM, Russell, Kent <Kent.Russell@amd.com> wrote: > There is GPU Power usage reported through amdgpu_pm_info, which also has some other information as well. I'd like that in sysfs, but I am unsure if we are allowed to due to the other information reported there. > We can add a sysfs file for power info, but I'd like the keep the debugfs one as well since we regularly change the format as we add features and such. Depending on what info you want to get, you may already have what you need via the sysfs pp files. Alex > Kent > > -----Original Message----- > From: StDenis, Tom > Sent: Friday, August 25, 2017 8:58 AM > To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > On 25/08/17 08:56 AM, Christian König wrote: >> Hi Kent, >> >> agree on the VBIOS dump file, that clearly belongs to debugsf. >> >> The power usage stuff I can't say much about cause I'm not deeply into >> this, but keep in mind the restriction for sysfs: >> 1. It's a stable interface. So it must be very well designed. >> 2. Only one value per file. I think the power stuff doesn't fulfill >> that requirement at the moment. > > What "power" stuff are we talking about? The sensors interface or the pm_info or something else? > > Keep in mind umr uses the sensors debugfs file in --top mode. > > Tom > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR1201MB01806ACF078801D91460C110859B0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 2017-08-25 13:05 ` Tom St Denis 2017-08-25 14:36 ` Alex Deucher @ 2017-08-25 15:56 ` Deucher, Alexander [not found] ` <BN6PR12MB16526CDFB29BC696816F2326F79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 2 siblings, 1 reply; 29+ messages in thread From: Deucher, Alexander @ 2017-08-25 15:56 UTC (permalink / raw) To: Russell, Kent, StDenis, Tom, Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW > -----Original Message----- > From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf > Of Russell, Kent > Sent: Friday, August 25, 2017 9:00 AM > To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org > Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > There is GPU Power usage reported through amdgpu_pm_info, which also > has some other information as well. I'd like that in sysfs, but I am unsure if > we are allowed to due to the other information reported there. For power and voltage, I believe there are standard hwmon interfaces. It would probably be best to expose it via those. For clocks/pcie, I think you can already determine them via the existing pp sysfs interfaces. Alex > > Kent > > -----Original Message----- > From: StDenis, Tom > Sent: Friday, August 25, 2017 8:58 AM > To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > On 25/08/17 08:56 AM, Christian König wrote: > > Hi Kent, > > > > agree on the VBIOS dump file, that clearly belongs to debugsf. > > > > The power usage stuff I can't say much about cause I'm not deeply into > > this, but keep in mind the restriction for sysfs: > > 1. It's a stable interface. So it must be very well designed. > > 2. Only one value per file. I think the power stuff doesn't fulfill > > that requirement at the moment. > > What "power" stuff are we talking about? The sensors interface or the > pm_info or something else? > > Keep in mind umr uses the sensors debugfs file in --top mode. > > Tom > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <BN6PR12MB16526CDFB29BC696816F2326F79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>]
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR12MB16526CDFB29BC696816F2326F79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> @ 2017-08-25 15:58 ` Russell, Kent 2017-08-25 19:33 ` Felix Kuehling 1 sibling, 0 replies; 29+ messages in thread From: Russell, Kent @ 2017-08-25 15:58 UTC (permalink / raw) To: Deucher, Alexander, StDenis, Tom, Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW That sounds good. There's already the sclk/mclk/pcieclk in the device sysfs tree, it was just power that we had to obtain in the debugfs. Thanks, I'll work on that now. And I'll add voltage because I am sure someone will ask for it soon enough. hwmon makes perfect sense though, I'll add it in there. Kent -----Original Message----- From: Deucher, Alexander Sent: Friday, August 25, 2017 11:56 AM To: Russell, Kent; StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > -----Original Message----- > From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf > Of Russell, Kent > Sent: Friday, August 25, 2017 9:00 AM > To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org > Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > There is GPU Power usage reported through amdgpu_pm_info, which also > has some other information as well. I'd like that in sysfs, but I am > unsure if we are allowed to due to the other information reported there. For power and voltage, I believe there are standard hwmon interfaces. It would probably be best to expose it via those. For clocks/pcie, I think you can already determine them via the existing pp sysfs interfaces. Alex > > Kent > > -----Original Message----- > From: StDenis, Tom > Sent: Friday, August 25, 2017 8:58 AM > To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > On 25/08/17 08:56 AM, Christian König wrote: > > Hi Kent, > > > > agree on the VBIOS dump file, that clearly belongs to debugsf. > > > > The power usage stuff I can't say much about cause I'm not deeply > > into this, but keep in mind the restriction for sysfs: > > 1. It's a stable interface. So it must be very well designed. > > 2. Only one value per file. I think the power stuff doesn't fulfill > > that requirement at the moment. > > What "power" stuff are we talking about? The sensors interface or the > pm_info or something else? > > Keep in mind umr uses the sensors debugfs file in --top mode. > > Tom > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR12MB16526CDFB29BC696816F2326F79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 2017-08-25 15:58 ` Russell, Kent @ 2017-08-25 19:33 ` Felix Kuehling [not found] ` <88a3901d-d05b-15c2-919a-03a5c90961ac-5C7GfCeVMHo@public.gmane.org> 1 sibling, 1 reply; 29+ messages in thread From: Felix Kuehling @ 2017-08-25 19:33 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW I think the power measurement is a bit of a hack right now. It requires pinging the SMU twice to start and stop the measurement, and waiting for some time (but not too long) in between. I think if we want to expose this via hwmon, we'll need to have some kind of timer that polls it in regular intervals in the background and updates a value that can be queried through HWMon without delay. Regards, Felix On 2017-08-25 11:56 AM, Deucher, Alexander wrote: >> -----Original Message----- >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf >> Of Russell, Kent >> Sent: Friday, August 25, 2017 9:00 AM >> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org >> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> There is GPU Power usage reported through amdgpu_pm_info, which also >> has some other information as well. I'd like that in sysfs, but I am unsure if >> we are allowed to due to the other information reported there. > For power and voltage, I believe there are standard hwmon interfaces. It would probably be best to expose it via those. For clocks/pcie, I think you can already determine them via the existing pp sysfs interfaces. > > Alex > >> Kent >> >> -----Original Message----- >> From: StDenis, Tom >> Sent: Friday, August 25, 2017 8:58 AM >> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> On 25/08/17 08:56 AM, Christian König wrote: >>> Hi Kent, >>> >>> agree on the VBIOS dump file, that clearly belongs to debugsf. >>> >>> The power usage stuff I can't say much about cause I'm not deeply into >>> this, but keep in mind the restriction for sysfs: >>> 1. It's a stable interface. So it must be very well designed. >>> 2. Only one value per file. I think the power stuff doesn't fulfill >>> that requirement at the moment. >> What "power" stuff are we talking about? The sensors interface or the >> pm_info or something else? >> >> Keep in mind umr uses the sensors debugfs file in --top mode. >> >> Tom >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <88a3901d-d05b-15c2-919a-03a5c90961ac-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <88a3901d-d05b-15c2-919a-03a5c90961ac-5C7GfCeVMHo@public.gmane.org> @ 2017-08-25 19:36 ` Tom St Denis 2017-08-25 19:40 ` Deucher, Alexander 1 sibling, 0 replies; 29+ messages in thread From: Tom St Denis @ 2017-08-25 19:36 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 25/08/17 03:33 PM, Felix Kuehling wrote: > I think the power measurement is a bit of a hack right now. It requires > pinging the SMU twice to start and stop the measurement, and waiting for > some time (but not too long) in between. I think if we want to expose > this via hwmon, we'll need to have some kind of timer that polls it in > regular intervals in the background and updates a value that can be > queried through HWMon without delay. We had this debate about 8 months ago :-) I don't know if adding an ever present kthread just for this though is a super use of resources. In umr when I read it I read the sensors in a separate thread from the main UI so that it doesn't mess with things. Tom _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <88a3901d-d05b-15c2-919a-03a5c90961ac-5C7GfCeVMHo@public.gmane.org> 2017-08-25 19:36 ` Tom St Denis @ 2017-08-25 19:40 ` Deucher, Alexander [not found] ` <BN6PR12MB165282855F85ABA5587C99BDF79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 1 sibling, 1 reply; 29+ messages in thread From: Deucher, Alexander @ 2017-08-25 19:40 UTC (permalink / raw) To: Kuehling, Felix, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW > -----Original Message----- > From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf > Of Felix Kuehling > Sent: Friday, August 25, 2017 3:34 PM > To: amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > I think the power measurement is a bit of a hack right now. It requires > pinging the SMU twice to start and stop the measurement, and waiting for > some time (but not too long) in between. I think if we want to expose > this via hwmon, we'll need to have some kind of timer that polls it in > regular intervals in the background and updates a value that can be > queried through HWMon without delay. I don't know that hwmon has any requirements with respect to delay. I think a slight delay in reading it back through hwmon is preferable to a background thread polling it. Regular polling may also have negative impacts on performance or stability, depending on how it was validated. Alex > > Regards, > Felix > > > On 2017-08-25 11:56 AM, Deucher, Alexander wrote: > >> -----Original Message----- > >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On > Behalf > >> Of Russell, Kent > >> Sent: Friday, August 25, 2017 9:00 AM > >> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org > >> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > >> > >> There is GPU Power usage reported through amdgpu_pm_info, which > also > >> has some other information as well. I'd like that in sysfs, but I am unsure if > >> we are allowed to due to the other information reported there. > > For power and voltage, I believe there are standard hwmon interfaces. It > would probably be best to expose it via those. For clocks/pcie, I think you > can already determine them via the existing pp sysfs interfaces. > > > > Alex > > > >> Kent > >> > >> -----Original Message----- > >> From: StDenis, Tom > >> Sent: Friday, August 25, 2017 8:58 AM > >> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org > >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > >> > >> On 25/08/17 08:56 AM, Christian König wrote: > >>> Hi Kent, > >>> > >>> agree on the VBIOS dump file, that clearly belongs to debugsf. > >>> > >>> The power usage stuff I can't say much about cause I'm not deeply into > >>> this, but keep in mind the restriction for sysfs: > >>> 1. It's a stable interface. So it must be very well designed. > >>> 2. Only one value per file. I think the power stuff doesn't fulfill > >>> that requirement at the moment. > >> What "power" stuff are we talking about? The sensors interface or the > >> pm_info or something else? > >> > >> Keep in mind umr uses the sensors debugfs file in --top mode. > >> > >> Tom > >> _______________________________________________ > >> amd-gfx mailing list > >> amd-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <BN6PR12MB165282855F85ABA5587C99BDF79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR12MB165282855F85ABA5587C99BDF79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> @ 2017-08-25 19:59 ` Felix Kuehling [not found] ` <863e1868-5a14-0748-cffe-81ff88c144ec-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Felix Kuehling @ 2017-08-25 19:59 UTC (permalink / raw) To: Deucher, Alexander, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 2017-08-25 03:40 PM, Deucher, Alexander wrote: >> -----Original Message----- >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf >> Of Felix Kuehling >> Sent: Friday, August 25, 2017 3:34 PM >> To: amd-gfx@lists.freedesktop.org >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >> >> I think the power measurement is a bit of a hack right now. It requires >> pinging the SMU twice to start and stop the measurement, and waiting for >> some time (but not too long) in between. I think if we want to expose >> this via hwmon, we'll need to have some kind of timer that polls it in >> regular intervals in the background and updates a value that can be >> queried through HWMon without delay. > I don't know that hwmon has any requirements with respect to delay. I think a slight delay in reading it back through hwmon is preferable to a background thread polling it. Regular polling may also have negative impacts on performance or stability, depending on how it was validated. If an application is reading hwmon data from 4 GPUs and each query for power usage from each GPU takes one second, that would seriously limit the update frequency of the application. Maybe the first read could block long enough to get useful data. But subsequent reads could probably avoid blocking if we keep track of the time stamp of the last query. If it was very recently, we can return the last value. If it was long enough ago, we can get an updated value from the SMU. If the last query was too long ago, we start over and block for a second. Regards, Felix > Alex > >> Regards, >> Felix >> >> >> On 2017-08-25 11:56 AM, Deucher, Alexander wrote: >>>> -----Original Message----- >>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On >> Behalf >>>> Of Russell, Kent >>>> Sent: Friday, August 25, 2017 9:00 AM >>>> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org >>>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>>> >>>> There is GPU Power usage reported through amdgpu_pm_info, which >> also >>>> has some other information as well. I'd like that in sysfs, but I am unsure if >>>> we are allowed to due to the other information reported there. >>> For power and voltage, I believe there are standard hwmon interfaces. It >> would probably be best to expose it via those. For clocks/pcie, I think you >> can already determine them via the existing pp sysfs interfaces. >>> Alex >>> >>>> Kent >>>> >>>> -----Original Message----- >>>> From: StDenis, Tom >>>> Sent: Friday, August 25, 2017 8:58 AM >>>> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org >>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS >>>> >>>> On 25/08/17 08:56 AM, Christian König wrote: >>>>> Hi Kent, >>>>> >>>>> agree on the VBIOS dump file, that clearly belongs to debugsf. >>>>> >>>>> The power usage stuff I can't say much about cause I'm not deeply into >>>>> this, but keep in mind the restriction for sysfs: >>>>> 1. It's a stable interface. So it must be very well designed. >>>>> 2. Only one value per file. I think the power stuff doesn't fulfill >>>>> that requirement at the moment. >>>> What "power" stuff are we talking about? The sensors interface or the >>>> pm_info or something else? >>>> >>>> Keep in mind umr uses the sensors debugfs file in --top mode. >>>> >>>> Tom >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>> _______________________________________________ >>> amd-gfx mailing list >>> amd-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <863e1868-5a14-0748-cffe-81ff88c144ec-5C7GfCeVMHo@public.gmane.org>]
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <863e1868-5a14-0748-cffe-81ff88c144ec-5C7GfCeVMHo@public.gmane.org> @ 2017-08-25 20:06 ` Deucher, Alexander [not found] ` <BN6PR12MB1652B766212C3249B22FC286F79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 0 siblings, 1 reply; 29+ messages in thread From: Deucher, Alexander @ 2017-08-25 20:06 UTC (permalink / raw) To: Kuehling, Felix, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW > -----Original Message----- > From: Kuehling, Felix > Sent: Friday, August 25, 2017 3:59 PM > To: Deucher, Alexander; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > On 2017-08-25 03:40 PM, Deucher, Alexander wrote: > >> -----Original Message----- > >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On > Behalf > >> Of Felix Kuehling > >> Sent: Friday, August 25, 2017 3:34 PM > >> To: amd-gfx@lists.freedesktop.org > >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > >> > >> I think the power measurement is a bit of a hack right now. It requires > >> pinging the SMU twice to start and stop the measurement, and waiting > for > >> some time (but not too long) in between. I think if we want to expose > >> this via hwmon, we'll need to have some kind of timer that polls it in > >> regular intervals in the background and updates a value that can be > >> queried through HWMon without delay. > > I don't know that hwmon has any requirements with respect to delay. I > think a slight delay in reading it back through hwmon is preferable to a > background thread polling it. Regular polling may also have negative impacts > on performance or stability, depending on how it was validated. > > If an application is reading hwmon data from 4 GPUs and each query for > power usage from each GPU takes one second, that would seriously limit > the update frequency of the application. > > Maybe the first read could block long enough to get useful data. But > subsequent reads could probably avoid blocking if we keep track of the > time stamp of the last query. If it was very recently, we can return the > last value. If it was long enough ago, we can get an updated value from > the SMU. If the last query was too long ago, we start over and block for > a second. Could we just set the sampling period down? If the application is polling for instantaneous power we'd probably want a short smu sampling period anyway. Otherwise, the application shouldn’t be polling so frequently in the first place. Alex > > Regards, > Felix > > > Alex > > > >> Regards, > >> Felix > >> > >> > >> On 2017-08-25 11:56 AM, Deucher, Alexander wrote: > >>>> -----Original Message----- > >>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On > >> Behalf > >>>> Of Russell, Kent > >>>> Sent: Friday, August 25, 2017 9:00 AM > >>>> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org > >>>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > >>>> > >>>> There is GPU Power usage reported through amdgpu_pm_info, which > >> also > >>>> has some other information as well. I'd like that in sysfs, but I am > unsure if > >>>> we are allowed to due to the other information reported there. > >>> For power and voltage, I believe there are standard hwmon interfaces. > It > >> would probably be best to expose it via those. For clocks/pcie, I think you > >> can already determine them via the existing pp sysfs interfaces. > >>> Alex > >>> > >>>> Kent > >>>> > >>>> -----Original Message----- > >>>> From: StDenis, Tom > >>>> Sent: Friday, August 25, 2017 8:58 AM > >>>> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org > >>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > >>>> > >>>> On 25/08/17 08:56 AM, Christian König wrote: > >>>>> Hi Kent, > >>>>> > >>>>> agree on the VBIOS dump file, that clearly belongs to debugsf. > >>>>> > >>>>> The power usage stuff I can't say much about cause I'm not deeply > into > >>>>> this, but keep in mind the restriction for sysfs: > >>>>> 1. It's a stable interface. So it must be very well designed. > >>>>> 2. Only one value per file. I think the power stuff doesn't fulfill > >>>>> that requirement at the moment. > >>>> What "power" stuff are we talking about? The sensors interface or the > >>>> pm_info or something else? > >>>> > >>>> Keep in mind umr uses the sensors debugfs file in --top mode. > >>>> > >>>> Tom > >>>> _______________________________________________ > >>>> amd-gfx mailing list > >>>> amd-gfx@lists.freedesktop.org > >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >>> _______________________________________________ > >>> amd-gfx mailing list > >>> amd-gfx@lists.freedesktop.org > >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >> _______________________________________________ > >> amd-gfx mailing list > >> amd-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <BN6PR12MB1652B766212C3249B22FC286F79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>]
* RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS [not found] ` <BN6PR12MB1652B766212C3249B22FC286F79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> @ 2017-08-25 20:14 ` Deucher, Alexander 0 siblings, 0 replies; 29+ messages in thread From: Deucher, Alexander @ 2017-08-25 20:14 UTC (permalink / raw) To: Deucher, Alexander, Kuehling, Felix, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW > -----Original Message----- > From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf > Of Deucher, Alexander > Sent: Friday, August 25, 2017 4:07 PM > To: Kuehling, Felix; amd-gfx@lists.freedesktop.org > Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > > -----Original Message----- > > From: Kuehling, Felix > > Sent: Friday, August 25, 2017 3:59 PM > > To: Deucher, Alexander; amd-gfx@lists.freedesktop.org > > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > > > On 2017-08-25 03:40 PM, Deucher, Alexander wrote: > > >> -----Original Message----- > > >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On > > Behalf > > >> Of Felix Kuehling > > >> Sent: Friday, August 25, 2017 3:34 PM > > >> To: amd-gfx@lists.freedesktop.org > > >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > >> > > >> I think the power measurement is a bit of a hack right now. It requires > > >> pinging the SMU twice to start and stop the measurement, and waiting > > for > > >> some time (but not too long) in between. I think if we want to expose > > >> this via hwmon, we'll need to have some kind of timer that polls it in > > >> regular intervals in the background and updates a value that can be > > >> queried through HWMon without delay. > > > I don't know that hwmon has any requirements with respect to delay. I > > think a slight delay in reading it back through hwmon is preferable to a > > background thread polling it. Regular polling may also have negative > impacts > > on performance or stability, depending on how it was validated. > > > > If an application is reading hwmon data from 4 GPUs and each query for > > power usage from each GPU takes one second, that would seriously limit > > the update frequency of the application. > > > > Maybe the first read could block long enough to get useful data. But > > subsequent reads could probably avoid blocking if we keep track of the > > time stamp of the last query. If it was very recently, we can return the > > last value. If it was long enough ago, we can get an updated value from > > the SMU. If the last query was too long ago, we start over and block for > > a second. > > Could we just set the sampling period down? If the application is polling for > instantaneous power we'd probably want a short smu sampling period > anyway. Otherwise, the application shouldn’t be polling so frequently in the > first place. Another alternative would be to expose the SMU interface directly. So getting the power would be something like: echo 1 > power #wait echo 0 > power cat power or: echo 2 > power # seconds to sample cat power #returns -EBUSY until the sampling period is done Alex > > Alex > > > > > Regards, > > Felix > > > > > Alex > > > > > >> Regards, > > >> Felix > > >> > > >> > > >> On 2017-08-25 11:56 AM, Deucher, Alexander wrote: > > >>>> -----Original Message----- > > >>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On > > >> Behalf > > >>>> Of Russell, Kent > > >>>> Sent: Friday, August 25, 2017 9:00 AM > > >>>> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org > > >>>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > >>>> > > >>>> There is GPU Power usage reported through amdgpu_pm_info, > which > > >> also > > >>>> has some other information as well. I'd like that in sysfs, but I am > > unsure if > > >>>> we are allowed to due to the other information reported there. > > >>> For power and voltage, I believe there are standard hwmon interfaces. > > It > > >> would probably be best to expose it via those. For clocks/pcie, I think > you > > >> can already determine them via the existing pp sysfs interfaces. > > >>> Alex > > >>> > > >>>> Kent > > >>>> > > >>>> -----Original Message----- > > >>>> From: StDenis, Tom > > >>>> Sent: Friday, August 25, 2017 8:58 AM > > >>>> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org > > >>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS > > >>>> > > >>>> On 25/08/17 08:56 AM, Christian König wrote: > > >>>>> Hi Kent, > > >>>>> > > >>>>> agree on the VBIOS dump file, that clearly belongs to debugsf. > > >>>>> > > >>>>> The power usage stuff I can't say much about cause I'm not deeply > > into > > >>>>> this, but keep in mind the restriction for sysfs: > > >>>>> 1. It's a stable interface. So it must be very well designed. > > >>>>> 2. Only one value per file. I think the power stuff doesn't fulfill > > >>>>> that requirement at the moment. > > >>>> What "power" stuff are we talking about? The sensors interface or > the > > >>>> pm_info or something else? > > >>>> > > >>>> Keep in mind umr uses the sensors debugfs file in --top mode. > > >>>> > > >>>> Tom > > >>>> _______________________________________________ > > >>>> amd-gfx mailing list > > >>>> amd-gfx@lists.freedesktop.org > > >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > >>> _______________________________________________ > > >>> amd-gfx mailing list > > >>> amd-gfx@lists.freedesktop.org > > >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > >> _______________________________________________ > > >> amd-gfx mailing list > > >> amd-gfx@lists.freedesktop.org > > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2017-08-25 20:14 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-23 18:12 [PATCH] drm/amdgpu: Add sysfs file for VBIOS Kent Russell [not found] ` <1503511940-645-1-git-send-email-kent.russell-5C7GfCeVMHo@public.gmane.org> 2017-08-24 1:21 ` Michel Dänzer 2017-08-24 6:21 ` Christian König [not found] ` <1e64a642-411e-ea58-1639-390e4c3cd75d-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> 2017-08-24 9:58 ` Russell, Kent [not found] ` <BN6PR1201MB01801A32C06AE0DA6634F440859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 2017-08-24 12:55 ` Alex Deucher [not found] ` <CADnq5_PD3s8HHPtr49ZJ3DXPVXgP5xyvFZj_YTzsGCi=cnOUKA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-08-24 13:06 ` Russell, Kent [not found] ` <BN6PR1201MB018010027CA78E203F629BA7859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 2017-08-24 15:35 ` Kuehling, Felix [not found] ` <DM5PR1201MB023503F29269CEC80DC7DAD8929A0-grEf7a3NxMBd8L2jMOIKKmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 2017-08-24 15:39 ` Alex Deucher [not found] ` <CADnq5_Piyp7mtHi+4GZ0pUw1kMNT68ZVyA6jp1TR4VhbCjWAOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-08-24 16:37 ` Russell, Kent [not found] ` <BN6PR1201MB01804846B5629D28766FDFA0859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 2017-08-24 18:10 ` Christian König [not found] ` <fa2dddb4-5ceb-87f3-6305-fb5f6e6e5c2d-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> 2017-08-24 21:30 ` Russell, Kent [not found] ` <BN6PR1201MB0180502DC4720A3C33D642DB859A0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 2017-08-25 12:36 ` Tom St Denis [not found] ` <4cec0dad-502e-68f2-68c9-b5d0103d7093-5C7GfCeVMHo@public.gmane.org> 2017-08-25 12:40 ` Russell, Kent [not found] ` <BN6PR1201MB01803DD5EBD0049397F4E9F0859B0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 2017-08-25 12:56 ` Christian König [not found] ` <0a3d4d07-e3cd-6216-466b-30bc60c8a26a-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> 2017-08-25 12:57 ` Tom St Denis [not found] ` <22c4b9e2-67e2-bd0a-1147-6e896843783e-5C7GfCeVMHo@public.gmane.org> 2017-08-25 12:59 ` Russell, Kent [not found] ` <BN6PR1201MB01806ACF078801D91460C110859B0-6iU6OBHu2P/H0AMcJMwsYmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 2017-08-25 13:05 ` Tom St Denis [not found] ` <ebeb7962-c55c-9ba3-6969-6f7cf9934a5b-5C7GfCeVMHo@public.gmane.org> 2017-08-25 13:32 ` Tom St Denis 2017-08-25 13:34 ` Tom St Denis [not found] ` <cc70864c-1106-1668-d80f-49cfbb6a6325-5C7GfCeVMHo@public.gmane.org> 2017-08-25 13:36 ` Russell, Kent 2017-08-25 14:36 ` Alex Deucher 2017-08-25 15:56 ` Deucher, Alexander [not found] ` <BN6PR12MB16526CDFB29BC696816F2326F79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 2017-08-25 15:58 ` Russell, Kent 2017-08-25 19:33 ` Felix Kuehling [not found] ` <88a3901d-d05b-15c2-919a-03a5c90961ac-5C7GfCeVMHo@public.gmane.org> 2017-08-25 19:36 ` Tom St Denis 2017-08-25 19:40 ` Deucher, Alexander [not found] ` <BN6PR12MB165282855F85ABA5587C99BDF79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 2017-08-25 19:59 ` Felix Kuehling [not found] ` <863e1868-5a14-0748-cffe-81ff88c144ec-5C7GfCeVMHo@public.gmane.org> 2017-08-25 20:06 ` Deucher, Alexander [not found] ` <BN6PR12MB1652B766212C3249B22FC286F79B0-/b2+HYfkarQqUD6E6FAiowdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> 2017-08-25 20:14 ` Deucher, Alexander
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.