* [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
@ 2017-05-29 21:16 Samuel Li
[not found] ` <1496092589-10642-1-git-send-email-Samuel.Li-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Samuel Li @ 2017-05-29 21:16 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Samuel Li, Xiaojie Yuan
From: Xiaojie Yuan <Xiaojie.Yuan@amd.com>
v2: fix an off by one error and leading white spaces
v3: use thread safe strtok_r(); initialize len before calling getline();
change printf() to drmMsg(); add initial amdgpu.ids
v4: integrate some recent internal changes, including format changes
Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>
Signed-off-by: Samuel Li <Samuel.Li@amd.com>
---
Makefile.am | 3 +
amdgpu/Makefile.am | 2 +
amdgpu/Makefile.sources | 2 +-
amdgpu/amdgpu_asic_id.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++
amdgpu/amdgpu_asic_id.h | 165 -------------------------------------
amdgpu/amdgpu_device.c | 28 +++++--
amdgpu/amdgpu_internal.h | 10 +++
include/drm/amdgpu.ids | 170 ++++++++++++++++++++++++++++++++++++++
8 files changed, 413 insertions(+), 173 deletions(-)
create mode 100644 amdgpu/amdgpu_asic_id.c
delete mode 100644 amdgpu/amdgpu_asic_id.h
create mode 100644 include/drm/amdgpu.ids
diff --git a/Makefile.am b/Makefile.am
index dfb8fcd..8de8f6c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -45,6 +45,9 @@ AM_DISTCHECK_CONFIGURE_FLAGS = \
pkgconfigdir = @pkgconfigdir@
pkgconfig_DATA = libdrm.pc
+libdrmdatadir = $(datadir)/libdrm
+dist_libdrmdata_DATA = include/drm/amdgpu.ids
+export libdrmdatadir
if HAVE_LIBKMS
LIBKMS_SUBDIR = libkms
diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am
index cf7bc1b..da71c1c 100644
--- a/amdgpu/Makefile.am
+++ b/amdgpu/Makefile.am
@@ -30,6 +30,8 @@ AM_CFLAGS = \
$(PTHREADSTUBS_CFLAGS) \
-I$(top_srcdir)/include/drm
+AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${libdrmdatadir}/amdgpu.ids\"
+
libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la
libdrm_amdgpu_ladir = $(libdir)
libdrm_amdgpu_la_LDFLAGS = -version-number 1:0:0 -no-undefined
diff --git a/amdgpu/Makefile.sources b/amdgpu/Makefile.sources
index 487b9e0..bc3abaa 100644
--- a/amdgpu/Makefile.sources
+++ b/amdgpu/Makefile.sources
@@ -1,5 +1,5 @@
LIBDRM_AMDGPU_FILES := \
- amdgpu_asic_id.h \
+ amdgpu_asic_id.c \
amdgpu_bo.c \
amdgpu_cs.c \
amdgpu_device.c \
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
@@ -0,0 +1,206 @@
+/*
+ * Copyright © 2017 Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * 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.
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+#include <unistd.h>
+#include <errno.h>
+
+#include "xf86drm.h"
+#include "amdgpu_drm.h"
+#include "amdgpu_internal.h"
+
+static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
+{
+ char *buf, *saveptr;
+ char *s_did;
+ char *s_rid;
+ char *s_name;
+ char *endptr;
+ int r = 0;
+
+ buf = strdup(line);
+ if (!buf)
+ return -ENOMEM;
+
+ /* ignore empty line and commented line */
+ if (strlen(line) == 0 || line[0] == '#') {
+ r = -EAGAIN;
+ goto out;
+ }
+
+ /* device id */
+ s_did = strtok_r(buf, ",", &saveptr);
+ if (!s_did) {
+ r = -EINVAL;
+ goto out;
+ }
+
+ id->did = strtol(s_did, &endptr, 16);
+ if (*endptr) {
+ r = -EINVAL;
+ goto out;
+ }
+
+ /* revision id */
+ s_rid = strtok_r(NULL, ",", &saveptr);
+ if (!s_rid) {
+ r = -EINVAL;
+ goto out;
+ }
+
+ id->rid = strtol(s_rid, &endptr, 16);
+ if (*endptr) {
+ r = -EINVAL;
+ goto out;
+ }
+
+ /* marketing name */
+ s_name = strtok_r(NULL, ",", &saveptr);
+ if (!s_name) {
+ r = -EINVAL;
+ goto out;
+ }
+ /* trim leading whitespaces or tabs */
+ while (*s_name == ' ' || *s_name == '\t')
+ s_name++;
+ if (strlen(s_name) == 0) {
+ r = -EINVAL;
+ goto out;
+ }
+
+ id->marketing_name = strdup(s_name);
+ if (id->marketing_name == NULL) {
+ r = -EINVAL;
+ goto out;
+ }
+
+out:
+ free(buf);
+
+ return r;
+}
+
+int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
+{
+ struct amdgpu_asic_id *asic_id_table;
+ struct amdgpu_asic_id *id;
+ FILE *fp;
+ char *line = NULL;
+ size_t len = 0;
+ ssize_t n;
+ int line_num = 1;
+ size_t table_size = 0;
+ size_t table_max_size = 256;
+ int r = 0;
+
+ fp = fopen(AMDGPU_ASIC_ID_TABLE, "r");
+ if (!fp) {
+ fprintf(stderr, "%s: %s\n", AMDGPU_ASIC_ID_TABLE,
+ strerror(errno));
+ return -EINVAL;
+ }
+
+ asic_id_table = calloc(table_max_size, sizeof(struct amdgpu_asic_id));
+ if (!asic_id_table) {
+ r = -ENOMEM;
+ goto close;
+ }
+
+ /* 1st valid line is file version */
+ while ((n = getline(&line, &len, fp)) != -1) {
+ /* trim trailing newline */
+ if (line[n - 1] == '\n')
+ line[n - 1] = '\0';
+
+ /* ignore empty line and commented line */
+ if (strlen(line) == 0 || line[0] == '#')
+ continue;
+
+ drmMsg("%s version: %s\n", AMDGPU_ASIC_ID_TABLE, line);
+ break;
+ }
+
+ while ((n = getline(&line, &len, fp)) != -1) {
+ id = asic_id_table + table_size;
+
+ /* trim trailing newline */
+ if (line[n - 1] == '\n')
+ line[n - 1] = '\0';
+
+ r = parse_one_line(line, id);
+ if (r) {
+ if (r == -EAGAIN) {
+ line_num++;
+ continue;
+ }
+ fprintf(stderr, "Invalid format: %s: line %d: %s\n",
+ AMDGPU_ASIC_ID_TABLE, line_num, line);
+ goto free;
+ }
+
+ line_num++;
+ table_size++;
+
+ if (table_size >= table_max_size) {
+ /* double table size */
+ table_max_size *= 2;
+ asic_id_table = realloc(asic_id_table, table_max_size *
+ sizeof(struct amdgpu_asic_id));
+ if (!asic_id_table) {
+ r = -ENOMEM;
+ goto free;
+ }
+ }
+ }
+
+ /* end of table */
+ id = asic_id_table + table_size;
+ memset(id, 0, sizeof(struct amdgpu_asic_id));
+
+free:
+ free(line);
+
+ if (r && asic_id_table) {
+ while (table_size--) {
+ id = asic_id_table + table_size;
+ if (id->marketing_name != NULL)
+ free(id->marketing_name);
+ }
+ free(asic_id_table);
+ asic_id_table = NULL;
+ }
+close:
+ fclose(fp);
+
+ *p_asic_id_table = asic_id_table;
+
+ return r;
+}
diff --git a/amdgpu/amdgpu_asic_id.h b/amdgpu/amdgpu_asic_id.h
deleted file mode 100644
index 3e7d736..0000000
--- a/amdgpu/amdgpu_asic_id.h
+++ /dev/null
@@ -1,165 +0,0 @@
-/*
- * Copyright © 2016 Advanced Micro Devices, Inc.
- * All Rights Reserved.
- *
- * 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.
- *
- */
-
-#ifndef __AMDGPU_ASIC_ID_H__
-#define __AMDGPU_ASIC_ID_H__
-
-static struct amdgpu_asic_id_table_t {
- uint32_t did;
- uint32_t rid;
- const char *marketing_name;
-} const amdgpu_asic_id_table [] = {
- {0x6600, 0x0, "AMD Radeon HD 8600/8700M"},
- {0x6600, 0x81, "AMD Radeon R7 M370"},
- {0x6601, 0x0, "AMD Radeon HD 8500M/8700M"},
- {0x6604, 0x0, "AMD Radeon R7 M265 Series"},
- {0x6604, 0x81, "AMD Radeon R7 M350"},
- {0x6605, 0x0, "AMD Radeon R7 M260 Series"},
- {0x6605, 0x81, "AMD Radeon R7 M340"},
- {0x6606, 0x0, "AMD Radeon HD 8790M"},
- {0x6607, 0x0, "AMD Radeon HD8530M"},
- {0x6608, 0x0, "AMD FirePro W2100"},
- {0x6610, 0x0, "AMD Radeon HD 8600 Series"},
- {0x6610, 0x81, "AMD Radeon R7 350"},
- {0x6610, 0x83, "AMD Radeon R5 340"},
- {0x6611, 0x0, "AMD Radeon HD 8500 Series"},
- {0x6613, 0x0, "AMD Radeon HD 8500 series"},
- {0x6617, 0xC7, "AMD Radeon R7 240 Series"},
- {0x6640, 0x0, "AMD Radeon HD 8950"},
- {0x6640, 0x80, "AMD Radeon R9 M380"},
- {0x6646, 0x0, "AMD Radeon R9 M280X"},
- {0x6646, 0x80, "AMD Radeon R9 M470X"},
- {0x6647, 0x0, "AMD Radeon R9 M270X"},
- {0x6647, 0x80, "AMD Radeon R9 M380"},
- {0x6649, 0x0, "AMD FirePro W5100"},
- {0x6658, 0x0, "AMD Radeon R7 200 Series"},
- {0x665C, 0x0, "AMD Radeon HD 7700 Series"},
- {0x665D, 0x0, "AMD Radeon R7 200 Series"},
- {0x665F, 0x81, "AMD Radeon R7 300 Series"},
- {0x6660, 0x0, "AMD Radeon HD 8600M Series"},
- {0x6660, 0x81, "AMD Radeon R5 M335"},
- {0x6660, 0x83, "AMD Radeon R5 M330"},
- {0x6663, 0x0, "AMD Radeon HD 8500M Series"},
- {0x6663, 0x83, "AMD Radeon R5 M320"},
- {0x6664, 0x0, "AMD Radeon R5 M200 Series"},
- {0x6665, 0x0, "AMD Radeon R5 M200 Series"},
- {0x6665, 0x83, "AMD Radeon R5 M320"},
- {0x6667, 0x0, "AMD Radeon R5 M200 Series"},
- {0x666F, 0x0, "AMD Radeon HD 8500M"},
- {0x6780, 0x0, "ATI FirePro V (FireGL V) Graphics Adapter"},
- {0x678A, 0x0, "ATI FirePro V (FireGL V) Graphics Adapter"},
- {0x6798, 0x0, "AMD Radeon HD 7900 Series"},
- {0x679A, 0x0, "AMD Radeon HD 7900 Series"},
- {0x679B, 0x0, "AMD Radeon HD 7900 Series"},
- {0x679E, 0x0, "AMD Radeon HD 7800 Series"},
- {0x67A0, 0x0, "HAWAII XTGL (67A0)"},
- {0x67A1, 0x0, "HAWAII GL40 (67A1)"},
- {0x67B0, 0x0, "AMD Radeon R9 200 Series"},
- {0x67B0, 0x80, "AMD Radeon R9 390 Series"},
- {0x67B1, 0x0, "AMD Radeon R9 200 Series"},
- {0x67B1, 0x80, "AMD Radeon R9 390 Series"},
- {0x67B9, 0x0, "AMD Radeon R9 200 Series"},
- {0x67DF, 0xC4, "AMD Radeon RX 480 Graphics"},
- {0x67DF, 0xC5, "AMD Radeon RX 470 Graphics"},
- {0x67DF, 0xC7, "AMD Radeon RX 480 Graphics"},
- {0x67DF, 0xCF, "AMD Radeon RX 470 Graphics"},
- {0x67C4, 0x00, "AMD Radeon Pro WX 7100 Graphics"},
- {0x67C7, 0x00, "AMD Radeon Pro WX 5100 Graphics"},
- {0x67C0, 0x00, "AMD Radeon Pro WX 7100 Graphics"},
- {0x67E0, 0x00, "AMD Radeon Pro WX Series Graphics"},
- {0x67E3, 0x00, "AMD Radeon Pro WX 4100 Graphics"},
- {0x67E8, 0x00, "AMD Radeon Pro WX Series Graphics"},
- {0x67E8, 0x01, "AMD Radeon Pro WX Series Graphics"},
- {0x67E8, 0x80, "AMD Radeon E9260 Graphics"},
- {0x67EB, 0x00, "AMD Radeon Pro WX Series Graphics"},
- {0x67EF, 0xC0, "AMD Radeon RX Graphics"},
- {0x67EF, 0xC1, "AMD Radeon RX 460 Graphics"},
- {0x67EF, 0xC5, "AMD Radeon RX 460 Graphics"},
- {0x67EF, 0xC7, "AMD Radeon RX Graphics"},
- {0x67EF, 0xCF, "AMD Radeon RX 460 Graphics"},
- {0x67EF, 0xEF, "AMD Radeon RX Graphics"},
- {0x67FF, 0xC0, "AMD Radeon RX Graphics"},
- {0x67FF, 0xC1, "AMD Radeon RX Graphics"},
- {0x6800, 0x0, "AMD Radeon HD 7970M"},
- {0x6801, 0x0, "AMD Radeon(TM) HD8970M"},
- {0x6808, 0x0, "ATI FirePro V(FireGL V) Graphics Adapter"},
- {0x6809, 0x0, "ATI FirePro V(FireGL V) Graphics Adapter"},
- {0x6810, 0x0, "AMD Radeon(TM) HD 8800 Series"},
- {0x6810, 0x81, "AMD Radeon R7 370 Series"},
- {0x6811, 0x0, "AMD Radeon(TM) HD8800 Series"},
- {0x6811, 0x81, "AMD Radeon R7 300 Series"},
- {0x6818, 0x0, "AMD Radeon HD 7800 Series"},
- {0x6819, 0x0, "AMD Radeon HD 7800 Series"},
- {0x6820, 0x0, "AMD Radeon HD 8800M Series"},
- {0x6820, 0x81, "AMD Radeon R9 M375"},
- {0x6820, 0x83, "AMD Radeon R9 M375X"},
- {0x6821, 0x0, "AMD Radeon HD 8800M Series"},
- {0x6821, 0x87, "AMD Radeon R7 M380"},
- {0x6821, 0x83, "AMD Radeon R9 M370X"},
- {0x6822, 0x0, "AMD Radeon E8860"},
- {0x6823, 0x0, "AMD Radeon HD 8800M Series"},
- {0x6825, 0x0, "AMD Radeon HD 7800M Series"},
- {0x6827, 0x0, "AMD Radeon HD 7800M Series"},
- {0x6828, 0x0, "ATI FirePro V(FireGL V) Graphics Adapter"},
- {0x682B, 0x0, "AMD Radeon HD 8800M Series"},
- {0x682B, 0x87, "AMD Radeon R9 M360"},
- {0x682C, 0x0, "AMD FirePro W4100"},
- {0x682D, 0x0, "AMD Radeon HD 7700M Series"},
- {0x682F, 0x0, "AMD Radeon HD 7700M Series"},
- {0x6835, 0x0, "AMD Radeon R7 Series / HD 9000 Series"},
- {0x6837, 0x0, "AMD Radeon HD7700 Series"},
- {0x683D, 0x0, "AMD Radeon HD 7700 Series"},
- {0x683F, 0x0, "AMD Radeon HD 7700 Series"},
- {0x6900, 0x0, "AMD Radeon R7 M260"},
- {0x6900, 0x81, "AMD Radeon R7 M360"},
- {0x6900, 0x83, "AMD Radeon R7 M340"},
- {0x6901, 0x0, "AMD Radeon R5 M255"},
- {0x6907, 0x0, "AMD Radeon R5 M255"},
- {0x6907, 0x87, "AMD Radeon R5 M315"},
- {0x6920, 0x0, "AMD Radeon R9 M395X"},
- {0x6920, 0x1, "AMD Radeon R9 M390X"},
- {0x6921, 0x0, "AMD Radeon R9 M295X"},
- {0x6929, 0x0, "AMD FirePro S7150"},
- {0x692B, 0x0, "AMD FirePro W7100"},
- {0x6938, 0x0, "AMD Radeon R9 200 Series"},
- {0x6938, 0xF0, "AMD Radeon R9 200 Series"},
- {0x6938, 0xF1, "AMD Radeon R9 380 Series"},
- {0x6939, 0xF0, "AMD Radeon R9 200 Series"},
- {0x6939, 0x0, "AMD Radeon R9 200 Series"},
- {0x6939, 0xF1, "AMD Radeon R9 380 Series"},
- {0x7300, 0xC8, "AMD Radeon R9 Fury Series"},
- {0x7300, 0xCB, "AMD Radeon R9 Fury Series"},
- {0x7300, 0xCA, "AMD Radeon R9 Fury Series"},
- {0x9874, 0xC4, "AMD Radeon R7 Graphics"},
- {0x9874, 0xC5, "AMD Radeon R6 Graphics"},
- {0x9874, 0xC6, "AMD Radeon R6 Graphics"},
- {0x9874, 0xC7, "AMD Radeon R5 Graphics"},
- {0x9874, 0x81, "AMD Radeon R6 Graphics"},
- {0x9874, 0x87, "AMD Radeon R5 Graphics"},
- {0x9874, 0x85, "AMD Radeon R6 Graphics"},
- {0x9874, 0x84, "AMD Radeon R7 Graphics"},
-
- {0x0000, 0x0, "\0"},
-};
-#endif
diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
index f473d2d..9d08744 100644
--- a/amdgpu/amdgpu_device.c
+++ b/amdgpu/amdgpu_device.c
@@ -44,7 +44,6 @@
#include "amdgpu_internal.h"
#include "util_hash_table.h"
#include "util_math.h"
-#include "amdgpu_asic_id.h"
#define PTR_TO_UINT(x) ((unsigned)((intptr_t)(x)))
#define UINT_TO_PTR(x) ((void *)((intptr_t)(x)))
@@ -131,6 +130,7 @@ static int amdgpu_get_auth(int fd, int *auth)
static void amdgpu_device_free_internal(amdgpu_device_handle dev)
{
+ const struct amdgpu_asic_id *id;
amdgpu_vamgr_deinit(&dev->vamgr_32);
amdgpu_vamgr_deinit(&dev->vamgr);
util_hash_table_destroy(dev->bo_flink_names);
@@ -140,6 +140,13 @@ static void amdgpu_device_free_internal(amdgpu_device_handle dev)
close(dev->fd);
if ((dev->flink_fd >= 0) && (dev->fd != dev->flink_fd))
close(dev->flink_fd);
+ if (dev->asic_ids) {
+ for (id = dev->asic_ids; id->did; id++) {
+ if (id->marketing_name != NULL)
+ free(id->marketing_name);
+ }
+ free(dev->asic_ids);
+ }
free(dev);
}
@@ -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);
+
*major_version = dev->major_version;
*minor_version = dev->minor_version;
*device_handle = dev;
@@ -297,13 +309,15 @@ int amdgpu_device_deinitialize(amdgpu_device_handle dev)
const char *amdgpu_get_marketing_name(amdgpu_device_handle dev)
{
- const struct amdgpu_asic_id_table_t *t = amdgpu_asic_id_table;
+ const struct amdgpu_asic_id *id;
+
+ if (!dev->asic_ids)
+ return NULL;
- while (t->did) {
- if ((t->did == dev->info.asic_id) &&
- (t->rid == dev->info.pci_rev_id))
- return t->marketing_name;
- t++;
+ for (id = dev->asic_ids; id->did; id++) {
+ if ((id->did == dev->info.asic_id) &&
+ (id->rid == dev->info.pci_rev_id))
+ return id->marketing_name;
}
return NULL;
diff --git a/amdgpu/amdgpu_internal.h b/amdgpu/amdgpu_internal.h
index cf119a5..e68246b 100644
--- a/amdgpu/amdgpu_internal.h
+++ b/amdgpu/amdgpu_internal.h
@@ -69,6 +69,12 @@ struct amdgpu_va {
struct amdgpu_bo_va_mgr *vamgr;
};
+struct amdgpu_asic_id {
+ uint32_t did;
+ uint32_t rid;
+ char *marketing_name;
+};
+
struct amdgpu_device {
atomic_t refcount;
int fd;
@@ -76,6 +82,8 @@ struct amdgpu_device {
unsigned major_version;
unsigned minor_version;
+ /** Lookup table of asic device id, revision id and marketing name */
+ struct amdgpu_asic_id *asic_ids;
/** List of buffer handles. Protected by bo_table_mutex. */
struct util_hash_table *bo_handles;
/** List of buffer GEM flink names. Protected by bo_table_mutex. */
@@ -149,6 +157,8 @@ amdgpu_vamgr_find_va(struct amdgpu_bo_va_mgr *mgr, uint64_t size,
drm_private void
amdgpu_vamgr_free_va(struct amdgpu_bo_va_mgr *mgr, uint64_t va, uint64_t size);
+drm_private int amdgpu_parse_asic_ids(struct amdgpu_asic_id **asic_ids);
+
drm_private int amdgpu_query_gpu_info_init(amdgpu_device_handle dev);
drm_private uint64_t amdgpu_cs_calculate_timeout(uint64_t timeout);
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
@@ -0,0 +1,170 @@
+# List of AMDGPU ID's
+#
+# Syntax:
+# device_id, revision_id, product_name <-- single tab after comma
+
+1.0.0
+6600, 0, AMD Radeon HD 8600/8700M
+6600, 81, AMD Radeon (TM) R7 M370
+6601, 0, AMD Radeon (TM) HD 8500M/8700M
+6604, 0, AMD Radeon R7 M265 Series
+6604, 81, AMD Radeon (TM) R7 M350
+6605, 0, AMD Radeon R7 M260 Series
+6605, 81, AMD Radeon (TM) R7 M340
+6606, 0, AMD Radeon HD 8790M
+6607, 0, AMD Radeon (TM) HD8530M
+6608, 0, AMD FirePro W2100
+6610, 0, AMD Radeon HD 8600 Series
+6610, 81, AMD Radeon (TM) R7 350
+6610, 83, AMD Radeon (TM) R5 340
+6611, 0, AMD Radeon HD 8500 Series
+6613, 0, AMD Radeon HD 8500 series
+6617, C7, AMD Radeon R7 240 Series
+6640, 0, AMD Radeon HD 8950
+6640, 80, AMD Radeon (TM) R9 M380
+6646, 0, AMD Radeon R9 M280X
+6646, 80, AMD Radeon (TM) R9 M470X
+6647, 0, AMD Radeon R9 M270X
+6647, 80, AMD Radeon (TM) R9 M380
+6649, 0, AMD FirePro W5100
+6658, 0, AMD Radeon R7 200 Series
+665C, 0, AMD Radeon HD 7700 Series
+665D, 0, AMD Radeon R7 200 Series
+665F, 81, AMD Radeon (TM) R7 300 Series
+6660, 0, AMD Radeon HD 8600M Series
+6660, 81, AMD Radeon (TM) R5 M335
+6660, 83, AMD Radeon (TM) R5 M330
+6663, 0, AMD Radeon HD 8500M Series
+6663, 83, AMD Radeon (TM) R5 M320
+6664, 0, AMD Radeon R5 M200 Series
+6665, 0, AMD Radeon R5 M200 Series
+6665, 83, AMD Radeon (TM) R5 M320
+6667, 0, AMD Radeon R5 M200 Series
+666F, 0, AMD Radeon HD 8500M
+6780, 0, ATI FirePro V (FireGL V) Graphics Adapter
+678A, 0, ATI FirePro V (FireGL V) Graphics Adapter
+6798, 0, AMD Radeon HD 7900 Series
+679A, 0, AMD Radeon HD 7900 Series
+679B, 0, AMD Radeon HD 7900 Series
+679E, 0, AMD Radeon HD 7800 Series
+67A0, 0, AMD Radeon FirePro W9100
+67A1, 0, AMD Radeon FirePro W8100
+67B0, 0, AMD Radeon R9 200 Series
+67B0, 80, AMD Radeon (TM) R9 390 Series
+67B1, 0, AMD Radeon R9 200 Series
+67B1, 80, AMD Radeon (TM) R9 390 Series
+67B9, 0, AMD Radeon R9 200 Series
+67DF, C1, Radeon RX 580 Series
+67DF, C2, Radeon RX 570 Series
+67DF, C3, Radeon RX 580 Series
+67DF, C4, AMD Radeon (TM) RX 480 Graphics
+67DF, C5, AMD Radeon (TM) RX 470 Graphics
+67DF, C6, Radeon RX 570 Series
+67DF, C7, AMD Radeon (TM) RX 480 Graphics
+67DF, CF, AMD Radeon (TM) RX 470 Graphics
+67DF, E3, Radeon RX Series
+67DF, E7, Radeon RX 580 Series
+67DF, EF, Radeon RX 570 Series
+67C2, 0, 67C2:00
+67C2, 01, AMD Radeon (TM) Pro V7350x2
+67C2, 02, AMD Radeon (TM) Pro V7300X
+67C4, 00, AMD Radeon (TM) Pro WX 7100 Graphics
+67C7, 00, AMD Radeon (TM) Pro WX 5100 Graphics
+67C0, 00, AMD Radeon (TM) Pro WX 7100 Graphics
+67D0, 0, 67D0:00
+67D0, 01, AMD Radeon (TM) Pro V7350x2
+67D0, 02, AMD Radeon (TM) Pro V7300X
+67E0, 00, AMD Radeon (TM) Pro WX Series
+67E3, 00, AMD Radeon (TM) Pro WX 4100
+67E8, 00, AMD Radeon (TM) Pro WX Series
+67E8, 01, AMD Radeon (TM) Pro WX Series
+67E8, 80, AMD Radeon (TM) E9260 Graphics
+67EB, 00, AMD Radeon (TM) Pro V5300X
+67EF, C0, AMD Radeon (TM) RX Graphics
+67EF, C1, AMD Radeon (TM) RX 460 Graphics
+67EF, C3, Radeon RX Series
+67EF, C5, AMD Radeon (TM) RX 460 Graphics
+67EF, C7, AMD Radeon (TM) RX Graphics
+67EF, CF, AMD Radeon (TM) RX 460 Graphics
+67EF, E0, 67EF:E0
+67EF, E1, Radeon RX Series
+67EF, E3, Radeon RX Series
+67EF, E5, 67EF:E5
+67EF, E7, Radeon RX Series
+67EF, EF, AMD Radeon (TM) RX Graphics
+67EF, FF, Radeon RX Series
+67FF, C0, AMD Radeon (TM) RX Graphics
+67FF, C1, AMD Radeon (TM) RX Graphics
+67FF, CF, 67FF:CF
+67FF, EF, 67FF:EF
+67FF, FF, Radeon RX 550 Series
+6800, 0, AMD Radeon HD 7970M
+6801, 0, AMD Radeon(TM) HD8970M
+6808, 0, ATI FirePro V(FireGL V) Graphics Adapter
+6809, 0, ATI FirePro V(FireGL V) Graphics Adapter
+6810, 0, AMD Radeon(TM) HD 8800 Series
+6810, 81, AMD Radeon (TM) R7 370 Series
+6811, 0, AMD Radeon(TM) HD8800 Series
+6811, 81, AMD Radeon (TM) R7 300 Series
+6818, 0, AMD Radeon HD 7800 Series
+6819, 0, AMD Radeon HD 7800 Series
+6820, 0, AMD Radeon HD 8800M Series
+6820, 81, AMD Radeon (TM) R9 M375
+6820, 83, AMD Radeon (TM) R9 M375X
+6821, 0, AMD Radeon HD 8800M Series
+6821, 87, AMD Radeon (TM) R7 M380
+6821, 83, AMD Radeon R9 (TM) M370X
+6822, 0, AMD Radeon E8860
+6823, 0, AMD Radeon HD 8800M Series
+6825, 0, AMD Radeon HD 7800M Series
+6827, 0, AMD Radeon HD 7800M Series
+6828, 0, ATI FirePro V(FireGL V) Graphics Adapter
+682B, 0, AMD Radeon HD 8800M Series
+682B, 87, AMD Radeon (TM) R9 M360
+682C, 0, AMD FirePro W4100
+682D, 0, AMD Radeon HD 7700M Series
+682F, 0, AMD Radeon HD 7700M Series
+6835, 0, AMD Radeon R7 Series / HD 9000 Series
+6837, 0, AMD Radeon HD7700 Series
+683D, 0, AMD Radeon HD 7700 Series
+683F, 0, AMD Radeon HD 7700 Serie6860, 00, Radeon Instinct MI25
+6900, 0, AMD Radeon R7 M260
+6900, 81, AMD Radeon (TM) R7 M360
+6900, 83, AMD Radeon (TM) R7 M340
+6901, 0, AMD Radeon R5 M255
+6907, 0, AMD Radeon R5 M255
+6907, 87, AMD Radeon (TM) R5 M315
+6920, 0, AMD RADEON R9 M395X
+6920, 1, AMD RADEON R9 M390X
+6921, 0, AMD Radeon R9 M295X
+6929, 0, AMD FirePro S7150
+692B, 0, AMD FirePro W7100
+6938, 0, AMD Radeon R9 200 Series
+6938, F0, AMD Radeon R9 200 Series
+6938, F1, AMD Radeon (TM) R9 380 Series
+6939, F0, AMD Radeon R9 200 Series
+6939, 0, AMD Radeon R9 200 Series
+6939, F1, AMD Radeon (TM) R9 380 Series
+6980, 00, 6980:00
+6981, C0, 6981:C0
+6985, 00, AMD Radeon Pro WX3100
+6987, 80, 6987:80
+6995, 00, AMD Radeon Pro WX2100
+699F, 81, 699F:81
+699F, C0, Radeon 500 Series
+699F, C1, 699F:C1
+699F, C3, Radeon 500 Series
+699F, C7, Radeon RX 550 Series
+7300, C1, AMD FirePro (TM) S9300 x2
+7300, C8, AMD Radeon (TM) R9 Fury Series
+7300, C9, Radeon (TM) Pro Duo
+7300, CB, AMD Radeon (TM) R9 Fury Series
+7300, CA, AMD Radeon (TM) R9 Fury Series
+9874, C4, AMD Radeon R7 Graphics
+9874, C5, AMD Radeon R6 Graphics
+9874, C6, AMD Radeon R6 Graphics
+9874, C7, AMD Radeon R5 Graphics
+9874, 81, AMD Radeon R6 Graphics
+9874, 87, AMD Radeon R5 Graphics
+9874, 85, AMD Radeon R6 Graphics
+9874, 84, AMD Radeon R7 Graphics
--
2.7.4
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
[not found] ` <1496092589-10642-1-git-send-email-Samuel.Li-5C7GfCeVMHo@public.gmane.org>
@ 2017-05-30 1:25 ` Michel Dänzer
[not found] ` <79a6b0a4-edb4-0368-1902-90458b577c1f-otUistvHUpPR7s880joybQ@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Michel Dänzer @ 2017-05-30 1:25 UTC (permalink / raw)
To: Samuel Li; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Xiaojie Yuan
On 30/05/17 06:16 AM, Samuel Li wrote:
> From: Xiaojie Yuan <Xiaojie.Yuan@amd.com>
I took a closer look and noticed some details (and some non-details
about the amdgpu.ids file at the end).
> 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.
> +static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
> +{
> + char *buf, *saveptr;
> + char *s_did;
> + char *s_rid;
> + char *s_name;
> + char *endptr;
> + int r = 0;
This function could be simplified slightly by initializing r = -EINVAL
here and only setting it to 0 just before the out label.
> +int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
> +{
[...]
> + /* 1st valid line is file version */
> + while ((n = getline(&line, &len, fp)) != -1) {
> + /* trim trailing newline */
> + if (line[n - 1] == '\n')
> + line[n - 1] = '\0';
Should probably increment line_num here, otherwise the line number in
the error message below might be confusing.
> + fprintf(stderr, "Invalid format: %s: line %d: %s\n",
> + AMDGPU_ASIC_ID_TABLE, line_num, line);
The second line should be indented to align with the opening parenthesis.
> + if (table_size >= table_max_size) {
> + /* double table size */
> + table_max_size *= 2;
> + asic_id_table = realloc(asic_id_table, table_max_size *
> + sizeof(struct amdgpu_asic_id));
Ditto.
> + /* end of table */
> + id = asic_id_table + table_size;
> + memset(id, 0, sizeof(struct amdgpu_asic_id));
Might also want to realloc asic_id_table according to the final table
size, to avoid wasting memory.
> + if (r && asic_id_table) {
> + while (table_size--) {
> + id = asic_id_table + table_size;
> + if (id->marketing_name != NULL)
> + free(id->marketing_name);
free(NULL) works fine (and parse_one_line returns an error for
id->marketing_name == NULL anyway), so this can be simplified to
free(id->marketing_name);
> @@ -140,6 +140,13 @@ static void amdgpu_device_free_internal(amdgpu_device_handle dev)
> close(dev->fd);
> if ((dev->flink_fd >= 0) && (dev->fd != dev->flink_fd))
> close(dev->flink_fd);
> + if (dev->asic_ids) {
> + for (id = dev->asic_ids; id->did; id++) {
> + if (id->marketing_name != NULL)
> + free(id->marketing_name);
> + }
Ditto, this can be simplified to
for (id = dev->asic_ids; id->did; id++)
free(id->marketing_name);
> @@ -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.
> @@ -297,13 +309,15 @@ int amdgpu_device_deinitialize(amdgpu_device_handle dev)
>
> const char *amdgpu_get_marketing_name(amdgpu_device_handle dev)
> {
> - const struct amdgpu_asic_id_table_t *t = amdgpu_asic_id_table;
> + const struct amdgpu_asic_id *id;
> +
> + if (!dev->asic_ids)
> + return NULL;
>
> - while (t->did) {
> - if ((t->did == dev->info.asic_id) &&
> - (t->rid == dev->info.pci_rev_id))
> - return t->marketing_name;
> - t++;
> + for (id = dev->asic_ids; id->did; id++) {
> + if ((id->did == dev->info.asic_id) &&
> + (id->rid == dev->info.pci_rev_id))
The last line is indented incorrectly, should be 2 tabs and 4 spaces.
> 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.
> @@ -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.
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
[not found] ` <79a6b0a4-edb4-0368-1902-90458b577c1f-otUistvHUpPR7s880joybQ@public.gmane.org>
@ 2017-05-30 22:31 ` Li, Samuel
[not found] ` <CY1PR1201MB1033195A941CEB02E9E93DB2F5F00-JBJ/M6OpXY+2VhmsawAdvGrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Li, Samuel @ 2017-05-30 22:31 UTC (permalink / raw)
To: Michel Dänzer
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Yuan, Xiaojie
Please see comments inline.
-----Original Message-----
From: Michel Dänzer [mailto:michel@daenzer.net]
Sent: Monday, May 29, 2017 9:26 PM
To: Li, Samuel <Samuel.Li@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <Xiaojie.Yuan@amd.com>
Subject: Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
On 30/05/17 06:16 AM, Samuel Li wrote:
> From: Xiaojie Yuan <Xiaojie.Yuan@amd.com>
I took a closer look and noticed some details (and some non-details about the amdgpu.ids file at the end).
> 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.
> +static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
> +{
> + char *buf, *saveptr;
> + char *s_did;
> + char *s_rid;
> + char *s_name;
> + char *endptr;
> + int r = 0;
This function could be simplified slightly by initializing r = -EINVAL
here and only setting it to 0 just before the out label.
[Sam]This is likely a personal preference. I am fine with current implementation which is clear.
> +int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
> +{
[...]
> + /* 1st valid line is file version */
> + while ((n = getline(&line, &len, fp)) != -1) {
> + /* trim trailing newline */
> + if (line[n - 1] == '\n')
> + line[n - 1] = '\0';
Should probably increment line_num here, otherwise the line number in
the error message below might be confusing.
[Sam]That is a good catch.
> + fprintf(stderr, "Invalid format: %s: line %d: %s\n",
> + AMDGPU_ASIC_ID_TABLE, line_num, line);
The second line should be indented to align with the opening parenthesis.
[Sam] Can be done.
> + if (table_size >= table_max_size) {
> + /* double table size */
> + table_max_size *= 2;
> + asic_id_table = realloc(asic_id_table, table_max_size *
> + sizeof(struct amdgpu_asic_id));
Ditto.
[Sam] Can be done.
> + /* end of table */
> + id = asic_id_table + table_size;
> + memset(id, 0, sizeof(struct amdgpu_asic_id));
Might also want to realloc asic_id_table according to the final table
size, to avoid wasting memory.
[Sam] Good one.
> + if (r && asic_id_table) {
> + while (table_size--) {
> + id = asic_id_table + table_size;
> + if (id->marketing_name != NULL)
> + free(id->marketing_name);
free(NULL) works fine (and parse_one_line returns an error for
id->marketing_name == NULL anyway), so this can be simplified to
free(id->marketing_name);
[Sam] Can be done.
> @@ -140,6 +140,13 @@ static void amdgpu_device_free_internal(amdgpu_device_handle dev)
> close(dev->fd);
> if ((dev->flink_fd >= 0) && (dev->fd != dev->flink_fd))
> close(dev->flink_fd);
> + if (dev->asic_ids) {
> + for (id = dev->asic_ids; id->did; id++) {
> + if (id->marketing_name != NULL)
> + free(id->marketing_name);
> + }
Ditto, this can be simplified to
[Sam] Can be done.
for (id = dev->asic_ids; id->did; id++)
free(id->marketing_name);
> @@ -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?
> @@ -297,13 +309,15 @@ int amdgpu_device_deinitialize(amdgpu_device_handle dev)
>
> const char *amdgpu_get_marketing_name(amdgpu_device_handle dev)
> {
> - const struct amdgpu_asic_id_table_t *t = amdgpu_asic_id_table;
> + const struct amdgpu_asic_id *id;
> +
> + if (!dev->asic_ids)
> + return NULL;
>
> - while (t->did) {
> - if ((t->did == dev->info.asic_id) &&
> - (t->rid == dev->info.pci_rev_id))
> - return t->marketing_name;
> - t++;
> + for (id = dev->asic_ids; id->did; id++) {
> + if ((id->did == dev->info.asic_id) &&
> + (id->rid == dev->info.pci_rev_id))
The last line is indented incorrectly, should be 2 tabs and 4 spaces.
[Sam] Can be done
> 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.
> @@ -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.
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
[not found] ` <CY1PR1201MB1033195A941CEB02E9E93DB2F5F00-JBJ/M6OpXY+2VhmsawAdvGrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
@ 2017-05-31 5:15 ` Michel Dänzer
[not found] ` <d4f0edae-8adf-50b1-e0b0-520b9e368e9c-otUistvHUpPR7s880joybQ@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Michel Dänzer @ 2017-05-31 5:15 UTC (permalink / raw)
To: Li, Samuel; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Yuan, Xiaojie
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
[not found] ` <d4f0edae-8adf-50b1-e0b0-520b9e368e9c-otUistvHUpPR7s880joybQ@public.gmane.org>
@ 2017-05-31 13:17 ` Alex Deucher
[not found] ` <CADnq5_OZNgmVYeLyDBdzrvqLLKJ+EAKBq00K0WLFw+W1EnhgiQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-05-31 15:32 ` Li, Samuel
1 sibling, 1 reply; 11+ messages in thread
From: Alex Deucher @ 2017-05-31 13:17 UTC (permalink / raw)
To: Michel Dänzer
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Li, Samuel, Yuan, Xiaojie
On Wed, May 31, 2017 at 1:15 AM, Michel Dänzer <michel@daenzer.net> wrote:
> 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).
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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
[not found] ` <d4f0edae-8adf-50b1-e0b0-520b9e368e9c-otUistvHUpPR7s880joybQ@public.gmane.org>
2017-05-31 13:17 ` Alex Deucher
@ 2017-05-31 15:32 ` Li, Samuel
[not found] ` <CY1PR1201MB10336D98A57C23B8C3DF18A9F5F10-JBJ/M6OpXY+2VhmsawAdvGrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
1 sibling, 1 reply; 11+ messages in thread
From: Li, Samuel @ 2017-05-31 15:32 UTC (permalink / raw)
To: Michel Dänzer
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Yuan, Xiaojie
Please see comments incline,
-----Original Message-----
From: Michel Dänzer [mailto:michel@daenzer.net]
Sent: Wednesday, May 31, 2017 1:15 AM
To: Li, Samuel <Samuel.Li@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <Xiaojie.Yuan@amd.com>
Subject: Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
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.
[Sam] Please see here
https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
>>> @@ -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.
[Sam] OK.
>>> 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/.
[Sam] I am planning to do it right after this. README is also located in this directory.
>>> @@ -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).
[Sam] Essentially marketing names are defined by Marketing. They are complicated as I can imagine. If you have questions regarding the names, the thread I forwarded has the contact you can use.
IIRC, the hex format in marketing names has been used for very long time. My preference is to pass the names only, not to audit from a coder's view ... that can make discussions much much more difficult.
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
[not found] ` <CADnq5_OZNgmVYeLyDBdzrvqLLKJ+EAKBq00K0WLFw+W1EnhgiQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-06-01 5:46 ` Michel Dänzer
0 siblings, 0 replies; 11+ messages in thread
From: Michel Dänzer @ 2017-06-01 5:46 UTC (permalink / raw)
To: Alex Deucher
Cc: Li, Samuel, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Yuan, Xiaojie
On 31/05/17 10:17 PM, Alex Deucher wrote:
> On Wed, May 31, 2017 at 1:15 AM, Michel Dänzer <michel@daenzer.net> wrote:
>> 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:
>>>>
>>>>> +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.
Right, by the time a product is released, we should have the final
marketing name and can just add that directly. I really don't see the
point of adding such dummy entries before that in the libdrm repository.
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
[not found] ` <CY1PR1201MB10336D98A57C23B8C3DF18A9F5F10-JBJ/M6OpXY+2VhmsawAdvGrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
@ 2017-06-01 6:09 ` Michel Dänzer
[not found] ` <4cad0af4-94d0-0a74-7bfc-ef2e63227800-otUistvHUpPR7s880joybQ@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Michel Dänzer @ 2017-06-01 6:09 UTC (permalink / raw)
To: Li, Samuel; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Yuan, Xiaojie
On 01/06/17 12:32 AM, Li, Samuel wrote:
>> From: Michel Dänzer [mailto:michel@daenzer.net]
>> 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.
>
> [Sam] Please see here
> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
Thanks, I didn't know different search paths can apply with "" vs <>.
One learns something new every day. :) You can ignore the above then.
>>>>> 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/.
> [Sam] I am planning to do it right after this.
If amdgpu.ids living in the amdgpu directory prevents it from being used
by libdrm_radeon (why?), let's put it in a new toplevel directory, e.g.
"data".
> README is also located in this directory.
Not the same thing. It documents things about the header files, and
doesn't get installed anywhere.
>>>>> @@ -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).
>
> [Sam] Essentially marketing names are defined by Marketing. They are
> complicated as I can imagine. If you have questions regarding the
> names, the thread I forwarded has the contact you can use.
> IIRC, the hex format in marketing names has been used for very long
> time.
It's obviously not a "marketing name" but some kind of placeholder. We
don't need those in libdrm.
> My preference is to pass the names only, not to audit from a coder's
> view ...
That's not how we do things.
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
[not found] ` <4cad0af4-94d0-0a74-7bfc-ef2e63227800-otUistvHUpPR7s880joybQ@public.gmane.org>
@ 2017-06-01 14:27 ` Li, Samuel
[not found] ` <CY1PR1201MB10333653ACF093A7C38341EEF5F60-JBJ/M6OpXY+2VhmsawAdvGrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Li, Samuel @ 2017-06-01 14:27 UTC (permalink / raw)
To: Michel Dänzer
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Yuan, Xiaojie
>If amdgpu.ids living in the amdgpu directory prevents it from being used by libdrm_radeon (why?), let's put it in a new toplevel directory, e.g.
>"data".
>> README is also located in this directory.
> Not the same thing. It documents things about the header files, and doesn't get installed anywhere.
I think that is a personal preference.
>> My preference is to pass the names only, not to audit from a coder's
>> view ...
>That's not how we do things.
It is a data file, not really a part of code. It shall be your preference to decide how much time you would like to spend in auditing the names :)
Sam
-----Original Message-----
From: Michel Dänzer [mailto:michel@daenzer.net]
Sent: Thursday, June 01, 2017 2:09 AM
To: Li, Samuel <Samuel.Li@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <Xiaojie.Yuan@amd.com>
Subject: Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
On 01/06/17 12:32 AM, Li, Samuel wrote:
>> From: Michel Dänzer [mailto:michel@daenzer.net] 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.
>
> [Sam] Please see here
> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
Thanks, I didn't know different search paths can apply with "" vs <>.
One learns something new every day. :) You can ignore the above then.
>>>>> 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/.
> [Sam] I am planning to do it right after this.
If amdgpu.ids living in the amdgpu directory prevents it from being used by libdrm_radeon (why?), let's put it in a new toplevel directory, e.g.
"data".
> README is also located in this directory.
Not the same thing. It documents things about the header files, and doesn't get installed anywhere.
>>>>> @@ -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).
>
> [Sam] Essentially marketing names are defined by Marketing. They are
> complicated as I can imagine. If you have questions regarding the
> names, the thread I forwarded has the contact you can use.
> IIRC, the hex format in marketing names has been used for very long
> time.
It's obviously not a "marketing name" but some kind of placeholder. We don't need those in libdrm.
> My preference is to pass the names only, not to audit from a coder's
> view ...
That's not how we do things.
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
[not found] ` <CY1PR1201MB10333653ACF093A7C38341EEF5F60-JBJ/M6OpXY+2VhmsawAdvGrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
@ 2017-06-02 3:05 ` Michel Dänzer
[not found] ` <b13d7f5c-85ea-aba2-bf1d-66d74f0a6e1b-otUistvHUpPR7s880joybQ@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Michel Dänzer @ 2017-06-02 3:05 UTC (permalink / raw)
To: Li, Samuel; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Yuan, Xiaojie
On 01/06/17 11:27 PM, Li, Samuel wrote:
>> If amdgpu.ids living in the amdgpu directory prevents it from being used by libdrm_radeon (why?), let's put it in a new toplevel directory, e.g.
>> "data".
>>> README is also located in this directory.
>> Not the same thing. It documents things about the header files, and doesn't get installed anywhere.
> I think that is a personal preference.
If you're referring to naming the new directory "data", that's just a
suggestion. Another possibility is to simply put it in the toplevel
directory.
What I wrote about include/drm/README is easily verifiable fact.
>>> My preference is to pass the names only, not to audit from a coder's
>>> view ...
>> That's not how we do things.
> It is a data file, not really a part of code.
There is nothing magic about it. It's subject to the review process just
like any other file in the repository.
BTW, if you put the file outside of the amdgpu directory, you should
send the patch to the dri-devel mailing list for review as well.
> It shall be your preference to decide how much time you would like to
> spend in auditing the names :)
It shouldn't take as much time as we've spent talking about it in this
thread. :}
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
[not found] ` <b13d7f5c-85ea-aba2-bf1d-66d74f0a6e1b-otUistvHUpPR7s880joybQ@public.gmane.org>
@ 2017-06-02 14:25 ` Li, Samuel
0 siblings, 0 replies; 11+ messages in thread
From: Li, Samuel @ 2017-06-02 14:25 UTC (permalink / raw)
To: Michel Dänzer
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Yuan, Xiaojie
> Another possibility is to simply put it in the toplevel directory.
I considered that too, but I prefer not to pollute the toplevel.
> It shouldn't take as much time as we've spent talking about it in this thread. :}
Right, I meant you can go ahead if you would like to verify the names.
Sam
-----Original Message-----
From: Michel Dänzer [mailto:michel@daenzer.net]
Sent: Thursday, June 01, 2017 11:06 PM
To: Li, Samuel <Samuel.Li@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Yuan, Xiaojie <Xiaojie.Yuan@amd.com>
Subject: Re: [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file
On 01/06/17 11:27 PM, Li, Samuel wrote:
>> If amdgpu.ids living in the amdgpu directory prevents it from being used by libdrm_radeon (why?), let's put it in a new toplevel directory, e.g.
>> "data".
>>> README is also located in this directory.
>> Not the same thing. It documents things about the header files, and doesn't get installed anywhere.
> I think that is a personal preference.
If you're referring to naming the new directory "data", that's just a suggestion. Another possibility is to simply put it in the toplevel directory.
What I wrote about include/drm/README is easily verifiable fact.
>>> My preference is to pass the names only, not to audit from a coder's
>>> view ...
>> That's not how we do things.
> It is a data file, not really a part of code.
There is nothing magic about it. It's subject to the review process just like any other file in the repository.
BTW, if you put the file outside of the amdgpu directory, you should send the patch to the dri-devel mailing list for review as well.
> It shall be your preference to decide how much time you would like to
> spend in auditing the names :)
It shouldn't take as much time as we've spent talking about it in this thread. :}
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-06-02 14:25 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-29 21:16 [PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file Samuel Li
[not found] ` <1496092589-10642-1-git-send-email-Samuel.Li-5C7GfCeVMHo@public.gmane.org>
2017-05-30 1:25 ` Michel Dänzer
[not found] ` <79a6b0a4-edb4-0368-1902-90458b577c1f-otUistvHUpPR7s880joybQ@public.gmane.org>
2017-05-30 22:31 ` Li, Samuel
[not found] ` <CY1PR1201MB1033195A941CEB02E9E93DB2F5F00-JBJ/M6OpXY+2VhmsawAdvGrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
2017-05-31 5:15 ` Michel Dänzer
[not found] ` <d4f0edae-8adf-50b1-e0b0-520b9e368e9c-otUistvHUpPR7s880joybQ@public.gmane.org>
2017-05-31 13:17 ` Alex Deucher
[not found] ` <CADnq5_OZNgmVYeLyDBdzrvqLLKJ+EAKBq00K0WLFw+W1EnhgiQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-01 5:46 ` Michel Dänzer
2017-05-31 15:32 ` Li, Samuel
[not found] ` <CY1PR1201MB10336D98A57C23B8C3DF18A9F5F10-JBJ/M6OpXY+2VhmsawAdvGrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
2017-06-01 6:09 ` Michel Dänzer
[not found] ` <4cad0af4-94d0-0a74-7bfc-ef2e63227800-otUistvHUpPR7s880joybQ@public.gmane.org>
2017-06-01 14:27 ` Li, Samuel
[not found] ` <CY1PR1201MB10333653ACF093A7C38341EEF5F60-JBJ/M6OpXY+2VhmsawAdvGrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
2017-06-02 3:05 ` Michel Dänzer
[not found] ` <b13d7f5c-85ea-aba2-bf1d-66d74f0a6e1b-otUistvHUpPR7s880joybQ@public.gmane.org>
2017-06-02 14:25 ` Li, Samuel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.