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@xxxxxxxxxxxxxxxxxxxxx> on behalf of Russell, Kent <Kent.Russell at amd.com> Sent: Thursday, August 24, 2017 9:06:22 AM To: Alex Deucher Cc: Christian König; amd-gfx at 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@xxxxxxxxx] Sent: Thursday, August 24, 2017 8:56 AM To: Russell, Kent Cc: Christian König; amd-gfx at 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 at 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 at vodafone.de] > Sent: Thursday, August 24, 2017 2:22 AM > To: Russell, Kent; amd-gfx at 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 at 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 at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx at lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
