Please see comments incline, -----Original Message----- From: Michel Dänzer [mailto:michel@xxxxxxxxxxx] Sent: Wednesday, May 31, 2017 1:15 AM To: Li, Samuel <Samuel.Li at amd.com> Cc: amd-gfx at lists.freedesktop.org; Yuan, Xiaojie <Xiaojie.Yuan at amd.com> Subject: Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file On 31/05/17 07:31 AM, Li, Samuel wrote: > From: Michel Dänzer [mailto:michel at daenzer.net] >> On 30/05/17 06:16 AM, Samuel Li wrote: >> >>> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c new >>> file mode 100644 index 0000000..a43ca33 >>> --- /dev/null >>> +++ b/amdgpu/amdgpu_asic_id.c >> >> [...] >> >>> +#include "xf86drm.h" >>> +#include "amdgpu_drm.h" >> >> Should be >> >> #include <xf86drm.h> >> #include <amdgpu_drm.h> >> >> since these header files are not located in the same directory as >> amdgpu_asic_id.c. > > [Sam] Actually, "" is used to include programmer-defined header > files, and <> is used for files pre-designated by the compiler/IDE. The only difference between the two is that #include "" first looks for the header file in the same directory where the file containing the #include directive (not necessarily the same as the original *.c file passed to the compiler/preprocessor) is located, after that it looks in the same paths in the same order as <>. So "" only really makes sense when the header file is in the same directory as the file including it. [Sam] Please see here https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html >>> @@ -267,6 +274,11 @@ int amdgpu_device_initialize(int fd, >>> amdgpu_vamgr_init(&dev->vamgr_32, start, max, >>> dev->dev_info.virtual_address_alignment); >>> >>> + r = amdgpu_parse_asic_ids(&dev->asic_ids); >>> + if (r) >>> + fprintf(stderr, "%s: Can not parse asic ids, 0x%x.", >>> + __func__, r); >> >> "Cannot parse ASIC IDs" >> >> Also, there should be curly braces around a multi-line statement. > > [Sam] Can be done. However, it is still a single statement. Does it matter? It might not be strictly required, but I think it does make the code clearer in this case. [Sam] OK. >>> diff --git a/include/drm/amdgpu.ids b/include/drm/amdgpu.ids new >>> file mode 100644 index 0000000..6d6b944 >>> --- /dev/null >>> +++ b/include/drm/amdgpu.ids >> >> I think the path of this file in the repository should be >> amdgpu/amdgpu.ids rather than include/drm/amdgpu.ids. > > [Sam] The file is going to be shared with radeon. We can cross that bridge when we get there. Meanwhile, it's not a header file and not installed under $prefix/include/, so it doesn't belong in include/. [Sam] I am planning to do it right after this. README is also located in this directory. >>> @@ -0,0 +1,170 @@ >>> +# List of AMDGPU ID's >> >> This should say "IDs" instead of "ID's". >> >> >>> +67FF, CF, 67FF:CF >>> +67FF, EF, 67FF:EF >> >> There should be no such dummy entries in the file. If it's useful, >> amdgpu_get_marketing_name can return a dummy string based on the PCI >> ID and revision when there's no matching entry in the file. > > [Sam] I forwarded another thread to you. Please make your argument explicitly, for the benefit of non-AMD readers of the amd-gfx list. Anyway, I don't think that invalidates what I wrote, and Alex seems to agree. "67FF:CF" isn't a marketing name, so there should be no such entries in this file. It's not necessary anyway; assuming it's useful for amdgpu_get_marketing_name to return such "names", it can generate them on the fly when there is no matching entry in the file. Ideally the issues above should be fixed in the original file we get from marketing (?), but meanwhile / failing that we should fix them up (and can easily with Git). [Sam] Essentially marketing names are defined by Marketing. They are complicated as I can imagine. If you have questions regarding the names, the thread I forwarded has the contact you can use. IIRC, the hex format in marketing names has been used for very long time. My preference is to pass the names only, not to audit from a coder's view ... that can make discussions much much more difficult. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
