- above all, as-is make check will fail
Right, I did not check that.
- keeping the radeon API symmetrical to the amdgpu one would a good idea
The issue is Radeon does not have a struct similar to amdgpu_device_handle. I think the current radeon API is simpler. Maybe a follow up change can change amdgpu's API similar to radeon.
- is adding yet another header really justified?
radeon_asic_id.h? That is going to be used by ddx/mesa.
Sam
-----Original Message----- From: Emil Velikov [mailto:emil.l.velikov@gmail.com] Sent: Wednesday, July 05, 2017 7:03 AM To: Li, Samuel Samuel.Li@amd.com Cc: amd-gfx mailing list amd-gfx@lists.freedesktop.org; ML dri-devel <dri- devel@lists.freedesktop.org> Subject: Re: [PATCH libdrm 2/2] radeon: use asic id table to get chipset name
Hi Samuel,
On 30 June 2017 at 20:25, Samuel Li Samuel.Li@amd.com wrote:
Change-Id: I24b6624789d1a9dc0fd3a446b0e6f21ed5183ff2 Signed-off-by: Samuel Li Samuel.Li@amd.com
radeon/Makefile.am | 6 +++ radeon/Makefile.sources | 6 ++- radeon/radeon_asic_id.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++ radeon/radeon_asic_id.h | 37 +++++++++++++++++ 4 files changed, 153 insertions(+), 2 deletions(-) create mode 100644 radeon/radeon_asic_id.c create mode 100644 radeon/radeon_asic_id.h
diff --git a/radeon/Makefile.am b/radeon/Makefile.am index e241531..69407bc 100644 --- a/radeon/Makefile.am +++ b/radeon/Makefile.am @@ -30,6 +30,12 @@ AM_CFLAGS = \ $(PTHREADSTUBS_CFLAGS) \ -I$(top_srcdir)/include/drm
+libdrmdatadir = @libdrmdatadir@ +ASIC_ID_TABLE_NUM_ENTRIES := $(shell egrep -ci '^[0-9a-f]{4},.*[0-9a-
f]+,' \
$(top_srcdir)/data/amdgpu.ids) AM_CPPFLAGS =+-DRADEON_ASIC_ID_TABLE="${libdrmdatadir}/amdgpu.ids" \
+-
DRADEON_ASIC_ID_TABLE_NUM_ENTRIES=$(ASIC_ID_TABLE_NUM_ENTRIE S)
Reusing amdgpu.ids by libdrm_radeon is not so obvious. I'm wondering if adding a comment in the file [amdgpu.ids] may be a good idea. "File is used by $LIST" or "File has multiple users" or anything that hints/makes people look up the details.
Couple of other ideas/suggestions:
- above all, as-is make check will fail
- keeping the radeon API symmetrical to the amdgpu one would a good idea
- is adding yet another header really justified?
-Emil