[PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux