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_ENTRIES) + libdrm_radeon_la_LTLIBRARIES = libdrm_radeon.la libdrm_radeon_ladir = $(libdir) libdrm_radeon_la_LDFLAGS = -version-number 1:0:1 -no-undefined diff --git a/radeon/Makefile.sources b/radeon/Makefile.sources index 1cf482a..8eaf1c6 100644 --- a/radeon/Makefile.sources +++ b/radeon/Makefile.sources @@ -4,7 +4,8 @@ LIBDRM_RADEON_FILES := \ radeon_cs_space.c \ radeon_bo.c \ radeon_cs.c \ - radeon_surface.c + radeon_surface.c \ + radeon_asic_id.c
LIBDRM_RADEON_H_FILES := \ radeon_bo.h \ @@ -14,7 +15,8 @@ LIBDRM_RADEON_H_FILES := \ radeon_cs_gem.h \ radeon_bo_int.h \ radeon_cs_int.h \ - r600_pci_ids.h + r600_pci_ids.h \ + radeon_asic_id.h
LIBDRM_RADEON_BOF_FILES := \ bof.c \ diff --git a/radeon/radeon_asic_id.c b/radeon/radeon_asic_id.c new file mode 100644 index 0000000..b03502b --- /dev/null +++ b/radeon/radeon_asic_id.c @@ -0,0 +1,106 @@ +/* + * Copyright 2017 Advanced Micro Devices, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + */ + +/** + * \file radeon_asic_id.c + * + * Implementation of chipset name lookup functions for radeon device + * + */ + + +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +//#include <errno.h> +//#include <string.h> +#include <stdio.h> +#include <stdlib.h> +#include <pthread.h> +#include <stdint.h> +#include <assert.h> + +#include "xf86atomic.h" +#include "util/util_asic_id.h" +#include "radeon_asic_id.h" + + +static pthread_mutex_t asic_id_mutex = PTHREAD_MUTEX_INITIALIZER; +static struct util_asic_id *radeon_asic_ids; +static atomic_t refcount; + +int radeon_asic_id_initialize(void) +{ + int r = 0; + pthread_mutex_lock(&asic_id_mutex); + if (radeon_asic_ids) { + atomic_inc(&refcount); + pthread_mutex_unlock(&asic_id_mutex); + return r; + } + + r = util_parse_asic_ids(&radeon_asic_ids, RADEON_ASIC_ID_TABLE, + RADEON_ASIC_ID_TABLE_NUM_ENTRIES); + if (r) { + fprintf(stderr, "%s: Cannot parse ASIC IDs, 0x%x.", + __func__, r); + } else + atomic_inc(&refcount); + + pthread_mutex_unlock(&asic_id_mutex); + return r; +} + +void radeon_asic_id_deinitialize(void) +{ + const struct util_asic_id *id; + + assert(atomic_read(&refcount) > 0); + pthread_mutex_lock(&asic_id_mutex); + if (atomic_dec_and_test(&refcount)) { + if (radeon_asic_ids) { + for (id = radeon_asic_ids; id->did; id++) + free(id->marketing_name); + free(radeon_asic_ids); + radeon_asic_ids = NULL; + } + } + pthread_mutex_unlock(&asic_id_mutex); +} + +const char *radeon_get_marketing_name(uint32_t device_id, uint32_t pci_rev_id) +{ + const struct util_asic_id *id; + + if (!radeon_asic_ids) + return NULL; + + for (id = radeon_asic_ids; id->did; id++) { + if ((id->did == device_id) && + (id->rid == pci_rev_id)) + return id->marketing_name; + } + + return NULL; +} diff --git a/radeon/radeon_asic_id.h b/radeon/radeon_asic_id.h new file mode 100644 index 0000000..00bc110 --- /dev/null +++ b/radeon/radeon_asic_id.h @@ -0,0 +1,37 @@ +/* + * Copyright 2017 Advanced Micro Devices, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + */ + +/** + * \file radeon_asic_id.h + * + * Declare public API for chipset name lookup + * + */ +#ifndef _RADEON_ASIC_ID_H_ +#define _RADEON_ASIC_ID_H_ + +int radeon_asic_id_initialize(void); +void radeon_asic_id_deinitialize(void); +const char *radeon_get_marketing_name(uint32_t device_id, uint32_t pci_rev_id); + +#endif
Hi Sam,
do you also have a Mesa patch showing how the new APIs will be used? Without seeing that, some minor comments below.
On 01/07/17 04:25 AM, Samuel Li wrote:
+//#include <errno.h> +//#include <string.h>
Remove these lines.
+#include "util/util_asic_id.h"
Patch 1 adds the util directory to include paths, which would allow just
#include "util_asic_id.h"
Please consistently either use this, or don't add the util directory to the include path anywhere.
+int radeon_asic_id_initialize(void) +{
- int r = 0;
- pthread_mutex_lock(&asic_id_mutex);
Put an empty line between declarations and statements.
do you also have a Mesa patch showing how the new APIs will be used?
Sent out to mesa dev just now.
Remove these lines.
Right.
Please consistently either use this, or don't add the util directory to the include path anywhere.
OK.
Put an empty line between declarations and statements.
OK.
Sam
-----Original Message----- From: Michel Dänzer [mailto:michel@daenzer.net] Sent: Tuesday, July 04, 2017 5:43 AM To: Li, Samuel Samuel.Li@amd.com Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH libdrm 2/2] radeon: use asic id table to get chipset name
Hi Sam,
do you also have a Mesa patch showing how the new APIs will be used? Without seeing that, some minor comments below.
On 01/07/17 04:25 AM, Samuel Li wrote:
+//#include <errno.h> +//#include <string.h>
Remove these lines.
+#include "util/util_asic_id.h"
Patch 1 adds the util directory to include paths, which would allow just
#include "util_asic_id.h"
Please consistently either use this, or don't add the util directory to the include path anywhere.
+int radeon_asic_id_initialize(void) +{
- int r = 0;
- pthread_mutex_lock(&asic_id_mutex);
Put an empty line between declarations and statements.
-- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
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_ENTRIES)
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
- 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
On 5 July 2017 at 22:31, Li, Samuel Samuel.Li@amd.com wrote:
- 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.
Attach it to analogous primitive?
I think the current radeon API is simpler. Maybe a follow up change can change amdgpu's API similar to radeon.
Exposing 3 entry points instead of 1 is _not_simpler. Also you cannot change the existing API, since it also breaks the ABI. Leading to crash/cause memory corruption when using existing binaries.
- is adding yet another header really justified?
radeon_asic_id.h? That is going to be used by ddx/mesa.
Where it's used is orthogonal. You don't need a separate _public_ header for nearly every entry point ;-)
Thanks Emil
-----Original Message----- From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Emil Velikov Sent: Thursday, July 06, 2017 5:21 AM To: Li, Samuel Cc: ML dri-devel; amd-gfx mailing list Subject: Re: [PATCH libdrm 2/2] radeon: use asic id table to get chipset name
On 5 July 2017 at 22:31, Li, Samuel Samuel.Li@amd.com wrote:
- 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. Attach it to analogous primitive?
Radeon libdrm is much different than amdgpu. There is no analog.
I think the current radeon API is simpler. Maybe a follow up change can
change amdgpu's API similar to radeon.
Exposing 3 entry points instead of 1 is _not_simpler. Also you cannot change the existing API, since it also breaks the ABI. Leading to crash/cause memory corruption when using existing binaries.
- is adding yet another header really justified?
radeon_asic_id.h? That is going to be used by ddx/mesa.
Where it's used is orthogonal. You don't need a separate _public_ header for nearly every entry point ;-)
Actually having a separate header makes sense for radeon. We currently expose a separate header for each set of functionality (one for buffer management, one for command submission, one for surface management). Adding the asic names to any of the existing ones doesn’t really make sense from a functional standpoint.
Alex
Thanks Emil _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 6 July 2017 at 13:46, Deucher, Alexander Alexander.Deucher@amd.com wrote:
Attach it to analogous primitive?
Radeon libdrm is much different than amdgpu. There is no analog.
Upon a closer look, indeed there isn't. Must have gotten confused earlier.
I think the current radeon API is simpler. Maybe a follow up change can
change amdgpu's API similar to radeon.
Exposing 3 entry points instead of 1 is _not_simpler. Also you cannot change the existing API, since it also breaks the ABI. Leading to crash/cause memory corruption when using existing binaries.
- is adding yet another header really justified?
radeon_asic_id.h? That is going to be used by ddx/mesa.
Where it's used is orthogonal. You don't need a separate _public_ header for nearly every entry point ;-)
Actually having a separate header makes sense for radeon. We currently expose a separate header for each set of functionality (one for buffer management, one for command submission, one for surface management). Adding the asic names to any of the existing ones doesn’t really make sense from a functional standpoint.
Seems a bit unfortunate, but you have a point.
Having three extra entry points seems a bit much, but it seems like the best one can do atm.
Thanks Emil
dri-devel@lists.freedesktop.org