[PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

>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 at daenzer.net] 
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