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

["Michel Dänzer" <michel-otUistvHUpPR7s880joybQ@public.gmane.org>]

On 31/05/17 07:31 AM, Li, Samuel wrote:
> From: Michel Dänzer [mailto:michel@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.


>>> @@ -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.


>>> 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/.


>>> @@ -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).


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx