>If amdgpu.ids living in the amdgpu directory prevents it from being used by libdrm_radeon (why?), let's put it in a new toplevel directory, e.g. >"data". >> README is also located in this directory. > Not the same thing. It documents things about the header files, and doesn't get installed anywhere. I think that is a personal preference. >> My preference is to pass the names only, not to audit from a coder's >> view ... >That's not how we do things. It is a data file, not really a part of code. It shall be your preference to decide how much time you would like to spend in auditing the names :) Sam -----Original Message----- From: Michel Dänzer [mailto:michel@xxxxxxxxxxx] Sent: Thursday, June 01, 2017 2:09 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 01/06/17 12:32 AM, Li, Samuel wrote: >> From: Michel Dänzer [mailto:michel at daenzer.net] 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 Thanks, I didn't know different search paths can apply with "" vs <>. One learns something new every day. :) You can ignore the above then. >>>>> 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. If amdgpu.ids living in the amdgpu directory prevents it from being used by libdrm_radeon (why?), let's put it in a new toplevel directory, e.g. "data". > README is also located in this directory. Not the same thing. It documents things about the header files, and doesn't get installed anywhere. >>>>> @@ -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. It's obviously not a "marketing name" but some kind of placeholder. We don't need those in libdrm. > My preference is to pass the names only, not to audit from a coder's > view ... That's not how we do things. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
