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@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Tom St Denis Sent: Friday, August 25, 2017 8:37 AM To: amd-gfx at 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 at vodafone.de> > *Sent:* Thursday, August 24, 2017 2:10:35 PM > *To:* Russell, Kent; Alex Deucher; Kuehling, Felix > *Cc:* amd-gfx at 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 at gmail.com] >> Sent: Thursday, August 24, 2017 11:39 AM >> To: Kuehling, Felix >> Cc: Russell, Kent; 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 11:35 AM, Kuehling, Felix <Felix.Kuehling at 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 at lists.freedesktop.org> 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 at gmail.com] >>> 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 >> _______________________________________________ >> 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 > _______________________________________________ amd-gfx mailing list amd-gfx at lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
