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 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 > *Sent:* Tuesday, May 11, 2021 7:07 AM > *To:* Deucher, Alexander ; Marek Olšák < > maraeo@gmail.com> > *Cc:* Kees Cook ; Gu, JiaWei (Will) < > JiaWei.Gu@amd.com>; amd-gfx list ; Deng, > Emily ; Alex Deucher ; Nieto, > David M > *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 > on behalf of Marek Olšák > > *Sent:* Tuesday, May 11, 2021 4:18 AM > *To:* Christian König > > *Cc:* Kees Cook ; Gu, > JiaWei (Will) ; amd-gfx list > ; Deng, > Emily ; Alex Deucher > ; Nieto, David M > > *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 > > *Date: *Monday, May 10, 2021 at 6:48 AM > *To: *"Nieto, David M" , "Gu, > JiaWei (Will)" > *Cc: *Alex Deucher , > "Deng, Emily" , Kees Cook > , amd-gfx list > > *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) > *Sent:* Monday, May 10, 2021 12:13:07 AM > *To:* Nieto, David M > *Cc:* Alex Deucher ; > amd-gfx list > ; Kees Cook > ; Deng, Emily > > *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 > Sent: Monday, May 10, 2021 2:53 PM > To: Gu, JiaWei (Will) > Cc: Alex Deucher ; amd-gfx > list ; > Kees Cook ; Deng, Emily > > 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) > 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 > > Sent: Sunday, May 9, 2021 11:59 PM > > To: Gu, JiaWei (Will) > > Cc: amd-gfx list > ; Kees Cook > > > > Subject: Re: [PATCH] drm/amdgpu: Align serial size in > > drm_amdgpu_info_vbios > > > >> On Sat, May 8, 2021 at 2:48 AM Jiawei Gu > wrote: > >> > >> 20 should be serial char size now instead of 16. > >> > >> Signed-off-by: Jiawei Gu > > > > 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 > > %2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CJ > >> i > >> awei.Gu%40amd.com > > %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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > > > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > > >