> - 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 at gmail.com] > Sent: Wednesday, July 05, 2017 7:03 AM > To: Li, Samuel <Samuel.Li at amd.com> > Cc: amd-gfx mailing list <amd-gfx at lists.freedesktop.org>; ML dri-devel <dri- > devel at 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 at amd.com> wrote: > > Change-Id: I24b6624789d1a9dc0fd3a446b0e6f21ed5183ff2 > > Signed-off-by: Samuel Li <Samuel.Li at 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
