On Wed, May 31, 2017 at 1:15 AM, Michel Dänzer <michel at daenzer.net> wrote: > 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. > > >>>> @@ -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). Thinking about this more, it probably doesn't matter that much. By the time any of these cards with no marketing names get onto shelves, the names will be filled in. That said, it does seem strange to have these dummy entries. Alex
