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 >
