Re: [PATCH] drm/amdgpu: Align serial size in drm_amdgpu_info_vbios

["Marek Olšák" <maraeo@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 12102 bytes --]

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@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@gmail.com>
> *Sent:* Tuesday, May 11, 2021 7:07 AM
> *To:* Deucher, Alexander <Alexander.Deucher@amd.com>; Marek Olšák <
> maraeo@gmail.com>
> *Cc:* Kees Cook <keescook@chromium.org>; Gu, JiaWei (Will) <
> JiaWei.Gu@amd.com>; amd-gfx list <amd-gfx@lists.freedesktop.org>; Deng,
> Emily <Emily.Deng@amd.com>; Alex Deucher <alexdeucher@gmail.com>; Nieto,
> David M <David.Nieto@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@lists.freedesktop.org>
> <amd-gfx-bounces@lists.freedesktop.org> on behalf of Marek Olšák
> <maraeo@gmail.com> <maraeo@gmail.com>
> *Sent:* Tuesday, May 11, 2021 4:18 AM
> *To:* Christian König <ckoenig.leichtzumerken@gmail.com>
> <ckoenig.leichtzumerken@gmail.com>
> *Cc:* Kees Cook <keescook@chromium.org> <keescook@chromium.org>; Gu,
> JiaWei (Will) <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>; amd-gfx list
> <amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org>; Deng,
> Emily <Emily.Deng@amd.com> <Emily.Deng@amd.com>; Alex Deucher
> <alexdeucher@gmail.com> <alexdeucher@gmail.com>; Nieto, David M
> <David.Nieto@amd.com> <David.Nieto@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@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@gmail.com>
> <ckoenig.leichtzumerken@gmail.com>
> *Date: *Monday, May 10, 2021 at 6:48 AM
> *To: *"Nieto, David M" <David.Nieto@amd.com> <David.Nieto@amd.com>, "Gu,
> JiaWei (Will)" <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> *Cc: *Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>,
> "Deng, Emily" <Emily.Deng@amd.com> <Emily.Deng@amd.com>, Kees Cook
> <keescook@chromium.org> <keescook@chromium.org>, amd-gfx list
> <amd-gfx@lists.freedesktop.org> <amd-gfx@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@amd.com> <JiaWei.Gu@amd.com>
> *Sent:* Monday, May 10, 2021 12:13:07 AM
> *To:* Nieto, David M <David.Nieto@amd.com> <David.Nieto@amd.com>
> *Cc:* Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>;
> amd-gfx list <amd-gfx@lists.freedesktop.org>
> <amd-gfx@lists.freedesktop.org>; Kees Cook <keescook@chromium.org>
> <keescook@chromium.org>; Deng, Emily <Emily.Deng@amd.com>
> <Emily.Deng@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@amd.com> <David.Nieto@amd.com>
> Sent: Monday, May 10, 2021 2:53 PM
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> Cc: Alex Deucher <alexdeucher@gmail.com> <alexdeucher@gmail.com>; amd-gfx
> list <amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org>;
> Kees Cook <keescook@chromium.org> <keescook@chromium.org>; Deng, Emily
> <Emily.Deng@amd.com> <Emily.Deng@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@amd.com>
> <JiaWei.Gu@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@gmail.com> <alexdeucher@gmail.com>
> > Sent: Sunday, May 9, 2021 11:59 PM
> > To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com> <JiaWei.Gu@amd.com>
> > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>
> <amd-gfx@lists.freedesktop.org>; Kees Cook
> > <keescook@chromium.org> <keescook@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@amd.com>
> <Jiawei.Gu@amd.com> wrote:
> >>
> >> 20 should be serial char size now instead of 16.
> >>
> >> Signed-off-by: Jiawei Gu <Jiawei.Gu@amd.com> <Jiawei.Gu@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@lists.freedesktop.org
> >> 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&amp;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&amp;sdata=kAJiC6WoJUTeExwk6ftrLfMoY2OTAwg9X7mGgJT3kLk%3D&amp;res
> >> e
> >> rved=0
>
>
>
> _______________________________________________
>
> amd-gfx mailing list
>
> amd-gfx@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@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>
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 19348 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx