[PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios
Christian König
ckoenig.leichtzumerken at gmail.com
Sat May 15 13:22:03 UTC 2021
The uAPI isn't merged upstream yet, so can still be changed.
But I agree that this should be squashed into the original patch while
upstreaming.
When this is just for umr as open source client then the question is
still on the stable if this really need to be an IOCTL?
Christian.
Am 15.05.21 um 08:40 schrieb Marek Olšák:
> 1) Mesa doesn't plan to use this VBIOS query.
>
> 2) The patch is changing uapi, which is forbidden.
>
> Marek
>
> On Tue, May 11, 2021 at 12:35 PM Nieto, David M <David.Nieto at amd.com
> <mailto:David.Nieto at amd.com>> wrote:
>
> [AMD Public Use]
>
>
> The point of having the device ID in the structure is because we
> are reading it from the VBIOS header, not the asic registers. They
> should match, but an user may flash a VBIOS for a different devid
> and they may not match.
>
> Regarding sysfs vs ioctl I see value in providing it in both ways,
> Mesa uses IOCTL and other DRM based tools may benefit as well. I
> recently went through the trouble of having to add sysfs string
> parsing for some data not available in ioctl, and while not very
> complicated, it is a programming inconvenience.
>
> I understand that being uapi, changing it is not easy, but this is
> information extracted from a VBIOS header, something that has been
> kept stable for many years.
>
> David
> ------------------------------------------------------------------------
> *From:* Christian König <ckoenig.leichtzumerken at gmail.com
> <mailto:ckoenig.leichtzumerken at gmail.com>>
> *Sent:* Tuesday, May 11, 2021 7:07 AM
> *To:* Deucher, Alexander <Alexander.Deucher at amd.com
> <mailto:Alexander.Deucher at amd.com>>; Marek Olšák <maraeo at gmail.com
> <mailto:maraeo at gmail.com>>
> *Cc:* Kees Cook <keescook at chromium.org
> <mailto:keescook at chromium.org>>; Gu, JiaWei (Will)
> <JiaWei.Gu at amd.com <mailto:JiaWei.Gu at amd.com>>; amd-gfx list
> <amd-gfx at lists.freedesktop.org
> <mailto:amd-gfx at lists.freedesktop.org>>; Deng, Emily
> <Emily.Deng at amd.com <mailto:Emily.Deng at amd.com>>; Alex Deucher
> <alexdeucher at gmail.com <mailto:alexdeucher at gmail.com>>; Nieto,
> David M <David.Nieto at amd.com <mailto:David.Nieto at amd.com>>
> *Subject:* Re: [PATCH] drm/amdgpu: Align serial size in
> drm_amdgpu_info_vbios
> Yeah, but umr is making strong use of sysfs as well.
>
> The only justification of this interface would be if we want to
> use it in Mesa.
>
> And I agree with Marek that looks redundant with the device
> structure to me as well.
>
> Christian.
>
> Am 11.05.21 um 16:04 schrieb Deucher, Alexander:
>>
>> [AMD Public Use]
>>
>>
>> It's being used by umr and some other smi tools to provide vbios
>> information for debugging.
>>
>> Alex
>>
>> ------------------------------------------------------------------------
>> *From:* amd-gfx <amd-gfx-bounces at lists.freedesktop.org>
>> <mailto:amd-gfx-bounces at lists.freedesktop.org> on behalf of Marek
>> Olšák <maraeo at gmail.com> <mailto:maraeo at gmail.com>
>> *Sent:* Tuesday, May 11, 2021 4:18 AM
>> *To:* Christian König <ckoenig.leichtzumerken at gmail.com>
>> <mailto:ckoenig.leichtzumerken at gmail.com>
>> *Cc:* Kees Cook <keescook at chromium.org>
>> <mailto:keescook at chromium.org>; Gu, JiaWei (Will)
>> <JiaWei.Gu at amd.com> <mailto:JiaWei.Gu at amd.com>; amd-gfx list
>> <amd-gfx at lists.freedesktop.org>
>> <mailto:amd-gfx at lists.freedesktop.org>; Deng, Emily
>> <Emily.Deng at amd.com> <mailto:Emily.Deng at amd.com>; Alex Deucher
>> <alexdeucher at gmail.com> <mailto:alexdeucher at gmail.com>; Nieto,
>> David M <David.Nieto at amd.com> <mailto:David.Nieto at amd.com>
>> *Subject:* Re: [PATCH] drm/amdgpu: Align serial size in
>> drm_amdgpu_info_vbios
>> Mesa doesn't use sysfs.
>>
>> Note that this is a uapi, meaning that once it's in the kernel,
>> it can't be changed like that.
>>
>> What's the use case for this new interface? Isn't it partially
>> redundant with the current device info structure, which seems to
>> have the equivalent of dev_id and rev_id?
>>
>> Marek
>>
>> On Tue, May 11, 2021 at 3:51 AM Christian König
>> <ckoenig.leichtzumerken at gmail.com
>> <mailto:ckoenig.leichtzumerken at gmail.com>> wrote:
>>
>> Marek and other userspace folks need to decide that.
>>
>> Basic question here is if Mesa is already accessing sysfs
>> nodes for OpenGL or RADV. If that is the case then we should
>> probably expose the information there as well.
>>
>> If that isn't the case (which I think it is) then we should
>> implement it as IOCTL.
>>
>> Regards,
>> Christian.
>>
>> Am 10.05.21 um 22:19 schrieb Nieto, David M:
>>>
>>> One of the primary usecases is to add this information to
>>> the renderer string, I am not sure if there are other cases
>>> of UMD drivers accessing sysfs nodes, but I think if we
>>> think permissions, if a client is authenticated and opens
>>> the render device then it can use the IOCTL, it is unclear
>>> to me we can make a such an assumption for sysfs nodes…
>>>
>>> I think there is value in having both tbh.
>>>
>>> Regards,
>>>
>>> David
>>>
>>> *From: *Christian König <ckoenig.leichtzumerken at gmail.com>
>>> <mailto:ckoenig.leichtzumerken at gmail.com>
>>> *Date: *Monday, May 10, 2021 at 6:48 AM
>>> *To: *"Nieto, David M" <David.Nieto at amd.com>
>>> <mailto:David.Nieto at amd.com>, "Gu, JiaWei (Will)"
>>> <JiaWei.Gu at amd.com> <mailto:JiaWei.Gu at amd.com>
>>> *Cc: *Alex Deucher <alexdeucher at gmail.com>
>>> <mailto:alexdeucher at gmail.com>, "Deng, Emily"
>>> <Emily.Deng at amd.com> <mailto:Emily.Deng at amd.com>, Kees Cook
>>> <keescook at chromium.org> <mailto:keescook at chromium.org>,
>>> amd-gfx list <amd-gfx at lists.freedesktop.org>
>>> <mailto:amd-gfx at lists.freedesktop.org>
>>> *Subject: *Re: [PATCH] drm/amdgpu: Align serial size in
>>> drm_amdgpu_info_vbios
>>>
>>> Well we could add both as sysfs file(s).
>>>
>>> Question here is rather what is the primary use case of this
>>> and if the application has the necessary access permissions
>>> to the sysfs files?
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 10.05.21 um 15:42 schrieb Nieto, David M:
>>>
>>> Then the application would need to issue the ioctl and
>>> then open a sysfs file to get all the information it
>>> needs. It makes little sense from a programming
>>> perspective to add an incomplete interface in my opinion
>>>
>>> ------------------------------------------------------------------------
>>>
>>> *From:*Gu, JiaWei (Will) <JiaWei.Gu at amd.com>
>>> <mailto:JiaWei.Gu at amd.com>
>>> *Sent:* Monday, May 10, 2021 12:13:07 AM
>>> *To:* Nieto, David M <David.Nieto at amd.com>
>>> <mailto:David.Nieto at amd.com>
>>> *Cc:* Alex Deucher <alexdeucher at gmail.com>
>>> <mailto:alexdeucher at gmail.com>; amd-gfx list
>>> <amd-gfx at lists.freedesktop.org>
>>> <mailto:amd-gfx at lists.freedesktop.org>; Kees Cook
>>> <keescook at chromium.org> <mailto:keescook at chromium.org>;
>>> Deng, Emily <Emily.Deng at amd.com> <mailto:Emily.Deng at amd.com>
>>> *Subject:* RE: [PATCH] drm/amdgpu: Align serial size in
>>> drm_amdgpu_info_vbios
>>>
>>> [AMD Official Use Only - Internal Distribution Only]
>>>
>>> Hi David,
>>>
>>> What I meant is to ONLY delete the serial[16] from
>>> drm_amdgpu_info_vbios, not the whole struct.
>>>
>>> struct drm_amdgpu_info_vbios {
>>> __u8 name[64];
>>> __u32 dbdf;
>>> __u8 vbios_pn[64];
>>> __u32 version;
>>> __u8 date[32];
>>> __u8 serial[16]; // jiawei: shall we delete this
>>> __u32 dev_id;
>>> __u32 rev_id;
>>> __u32 sub_dev_id;
>>> __u32 sub_ved_id;
>>> };
>>>
>>> serial[16] in drm_amdgpu_info_vbios copied from
>>> adev->serial, but there's already a sysfs named
>>> serial_number, which exposes it already.
>>>
>>> static ssize_t amdgpu_device_get_serial_number(struct
>>> device *dev,
>>> struct device_attribute *attr, char *buf)
>>> {
>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>> struct amdgpu_device *adev = ddev->dev_private;
>>>
>>> return snprintf(buf, PAGE_SIZE, "%s\n",
>>> adev->serial);
>>> }
>>>
>>> Thanks,
>>> Jiawei
>>>
>>>
>>> -----Original Message-----
>>> From: Nieto, David M <David.Nieto at amd.com>
>>> <mailto:David.Nieto at amd.com>
>>> Sent: Monday, May 10, 2021 2:53 PM
>>> To: Gu, JiaWei (Will) <JiaWei.Gu at amd.com>
>>> <mailto:JiaWei.Gu at amd.com>
>>> Cc: Alex Deucher <alexdeucher at gmail.com>
>>> <mailto:alexdeucher at gmail.com>; amd-gfx list
>>> <amd-gfx at lists.freedesktop.org>
>>> <mailto:amd-gfx at lists.freedesktop.org>; Kees Cook
>>> <keescook at chromium.org> <mailto:keescook at chromium.org>;
>>> Deng, Emily <Emily.Deng at amd.com> <mailto:Emily.Deng at amd.com>
>>> Subject: Re: [PATCH] drm/amdgpu: Align serial size in
>>> drm_amdgpu_info_vbios
>>>
>>> No, this structure contains all the details of the
>>> vbios: date, serial number, name, etc.
>>>
>>> The sysfs node only contains the vbios name string
>>>
>>> > On May 9, 2021, at 23:33, Gu, JiaWei (Will)
>>> <JiaWei.Gu at amd.com> <mailto:JiaWei.Gu at amd.com> wrote:
>>> >
>>> > [AMD Official Use Only - Internal Distribution Only]
>>> >
>>> > With a second thought,
>>> > __u8 serial[16] in drm_amdgpu_info_vbios is a bit
>>> redundant, sysfs serial_number already exposes it.
>>> >
>>> > Is it fine to abandon it from drm_amdgpu_info_vbios
>>> struct? @Alex
>>> > Deucher @Nieto, David M
>>> >
>>> > Best regards,
>>> > Jiawei
>>> >
>>> > -----Original Message-----
>>> > From: Alex Deucher <alexdeucher at gmail.com>
>>> <mailto:alexdeucher at gmail.com>
>>> > Sent: Sunday, May 9, 2021 11:59 PM
>>> > To: Gu, JiaWei (Will) <JiaWei.Gu at amd.com>
>>> <mailto:JiaWei.Gu at amd.com>
>>> > Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>
>>> <mailto:amd-gfx at lists.freedesktop.org>; Kees Cook
>>> > <keescook at chromium.org> <mailto:keescook at chromium.org>
>>> > Subject: Re: [PATCH] drm/amdgpu: Align serial size in
>>> > drm_amdgpu_info_vbios
>>> >
>>> >> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu
>>> <Jiawei.Gu at amd.com> <mailto:Jiawei.Gu at amd.com> wrote:
>>> >>
>>> >> 20 should be serial char size now instead of 16.
>>> >>
>>> >> Signed-off-by: Jiawei Gu <Jiawei.Gu at amd.com>
>>> <mailto:Jiawei.Gu at amd.com>
>>> >
>>> > Please make sure this keeps proper 64 bit alignment in
>>> the structure.
>>> >
>>> > Alex
>>> >
>>> >
>>> >> ---
>>> >> include/uapi/drm/amdgpu_drm.h | 2 +-
>>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/include/uapi/drm/amdgpu_drm.h
>>> >> b/include/uapi/drm/amdgpu_drm.h index
>>> 2b487a8d2727..1c20721f90da
>>> >> 100644
>>> >> --- a/include/uapi/drm/amdgpu_drm.h
>>> >> +++ b/include/uapi/drm/amdgpu_drm.h
>>> >> @@ -957,7 +957,7 @@ struct drm_amdgpu_info_vbios {
>>> >> __u8 vbios_pn[64];
>>> >> __u32 version;
>>> >> __u8 date[32];
>>> >> - __u8 serial[16];
>>> >> + __u8 serial[20];
>>> >> __u32 dev_id;
>>> >> __u32 rev_id;
>>> >> __u32 sub_dev_id;
>>> >> --
>>> >> 2.17.1
>>> >>
>>> >> _______________________________________________
>>> >> amd-gfx mailing list
>>> >> amd-gfx at lists.freedesktop.org
>>> <mailto:amd-gfx at lists.freedesktop.org>
>>> >>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis>
>>> >> t
>>> >> s.freedesktop.org
>>> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fs.freedesktop.org%2F&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684906500%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gKeRCQ485gWespGfo1O01tZz8fDy1zPGHIDEvCqyp1I%3D&reserved=0>%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CJ
>>> >> i
>>> >> awei.Gu%40amd.com
>>> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2F40amd.com%2F&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684916541%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=8pM7WTh9%2F23KCF6DuXOc4%2Fi7rDUSxmH647BdpujXgWo%3D&reserved=0>%7Ccea31833184c41e8574508d9130360cc%7C3dd8961fe4884e
>>> >> 6
>>> >>
>>> 08e11a82d994e183d%7C0%7C0%7C637561727523880356%7CUnknown%7CTWFpbGZsb3
>>> >> d
>>> >>
>>> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
>>> >> C
>>> >>
>>> 1000&sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&res
>>> >> e
>>> >> rved=0
>>>
>>>
>>>
>>> _______________________________________________
>>>
>>> amd-gfx mailing list
>>>
>>> amd-gfx at lists.freedesktop.org <mailto:amd-gfx at lists.freedesktop.org>
>>>
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684926488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Hdhx2ogsQt2C3OIKNC%2BaswtNkiIWbV6Ot8a%2B3YFqa7w%3D&reserved=0>
>>>
>>>
>>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> <mailto:amd-gfx at lists.freedesktop.org>
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CDavid.Nieto%40amd.com%7Cf24b10fbdd964acea32008d914862738%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563388684926488%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Hdhx2ogsQt2C3OIKNC%2BaswtNkiIWbV6Ot8a%2B3YFqa7w%3D&reserved=0>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20210515/94fac90e/attachment-0001.htm>
More information about the amd-gfx
mailing list