[PATCH libdrm 0/3] amdgpu: amdgpu_get_marketing_name cleanups

[PATCH libdrm 0/3] amdgpu: amdgpu_get_marketing_name cleanups

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

The first two patches are preparatory for patch 3, which is the meat of
the series.

Michel Dänzer (3):
  amdgpu: Clean up amdgpu_parse_asic_ids error handling
  amdgpu: Simplify error handling in parse_one_line
  amdgpu: Only remember the device's marketing name

 amdgpu/Android.mk        |   3 +-
 amdgpu/Makefile.am       |   5 +-
 amdgpu/amdgpu_asic_id.c  | 139 ++++++++++++++---------------------------------
 amdgpu/amdgpu_device.c   |  27 +--------
 amdgpu/amdgpu_internal.h |  11 +---
 5 files changed, 49 insertions(+), 136 deletions(-)

-- 
2.15.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 1/3] amdgpu: Clean up amdgpu_parse_asic_ids error handling

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

* Move error message printing into amdgpu_parse_asic_ids and make it
  return void
* Print only "Invalid format" error message if parse_one_line returns
  -EINVAL
* Use strerror instead of printing the (negative) error code in hex

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 amdgpu/amdgpu_asic_id.c  | 16 ++++++++++------
 amdgpu/amdgpu_device.c   |  6 +-----
 amdgpu/amdgpu_internal.h |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c
index e8218974..eb42bbc2 100644
--- a/amdgpu/amdgpu_asic_id.c
+++ b/amdgpu/amdgpu_asic_id.c
@@ -109,7 +109,7 @@ out:
 	return r;
 }
 
-int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
+void amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
 {
 	struct amdgpu_asic_id *asic_id_table;
 	struct amdgpu_asic_id *id;
@@ -126,7 +126,7 @@ int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
 	if (!fp) {
 		fprintf(stderr, "%s: %s\n", AMDGPU_ASIC_ID_TABLE,
 			strerror(errno));
-		return -EINVAL;
+		return;
 	}
 
 	asic_id_table = calloc(table_max_size + 1,
@@ -177,8 +177,6 @@ int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
 				line_num++;
 				continue;
 			}
-			fprintf(stderr, "Invalid format: %s: line %d: %s\n",
-				AMDGPU_ASIC_ID_TABLE, line_num, line);
 			goto free;
 		}
 
@@ -201,6 +199,14 @@ int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
 	memset(id, 0, sizeof(struct amdgpu_asic_id));
 
 free:
+	if (r == -EINVAL) {
+		fprintf(stderr, "Invalid format: %s: line %d: %s\n",
+			AMDGPU_ASIC_ID_TABLE, line_num, line);
+	} else if (r) {
+		fprintf(stderr, "%s: Cannot parse ASIC IDs: %s\n",
+			__func__, strerror(-r));
+	}
+
 	free(line);
 
 	if (r && asic_id_table) {
@@ -215,6 +221,4 @@ close:
 	fclose(fp);
 
 	*p_asic_id_table = asic_id_table;
-
-	return r;
 }
diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
index 344e87ed..e7aaf4fc 100644
--- a/amdgpu/amdgpu_device.c
+++ b/amdgpu/amdgpu_device.c
@@ -280,11 +280,7 @@ int amdgpu_device_initialize(int fd,
 	amdgpu_vamgr_init(&dev->vamgr, start, max,
 			  dev->dev_info.virtual_address_alignment);
 
-	r = amdgpu_parse_asic_ids(&dev->asic_ids);
-	if (r) {
-		fprintf(stderr, "%s: Cannot parse ASIC IDs, 0x%x.",
-			__func__, r);
-	}
+	amdgpu_parse_asic_ids(&dev->asic_ids);
 
 	*major_version = dev->major_version;
 	*minor_version = dev->minor_version;
diff --git a/amdgpu/amdgpu_internal.h b/amdgpu/amdgpu_internal.h
index e26e5190..1aff7f8e 100644
--- a/amdgpu/amdgpu_internal.h
+++ b/amdgpu/amdgpu_internal.h
@@ -148,7 +148,7 @@ drm_private void amdgpu_vamgr_init(struct amdgpu_bo_va_mgr *mgr, uint64_t start,
 
 drm_private void amdgpu_vamgr_deinit(struct amdgpu_bo_va_mgr *mgr);
 
-drm_private int amdgpu_parse_asic_ids(struct amdgpu_asic_id **asic_ids);
+drm_private void amdgpu_parse_asic_ids(struct amdgpu_asic_id **asic_ids);
 
 drm_private int amdgpu_query_gpu_info_init(amdgpu_device_handle dev);
 
-- 
2.15.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 2/3] amdgpu: Simplify error handling in parse_one_line

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

* Move empty/commented line check before the strdup and return -EAGAIN
  directly
* Initialize r = -EAGAIN and remove redundant assignments
* Set r = -ENOMEM if last strdup fails, and remove redundant goto

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 amdgpu/amdgpu_asic_id.c | 45 ++++++++++++++++-----------------------------
 1 file changed, 16 insertions(+), 29 deletions(-)

diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c
index eb42bbc2..0b5f2962 100644
--- a/amdgpu/amdgpu_asic_id.c
+++ b/amdgpu/amdgpu_asic_id.c
@@ -45,63 +45,50 @@ static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
 	char *s_rid;
 	char *s_name;
 	char *endptr;
-	int r = 0;
+	int r = -EINVAL;
+
+	/* ignore empty line and commented line */
+	if (strlen(line) == 0 || line[0] == '#')
+		return -EAGAIN;
 
 	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;
+	if (!s_did)
 		goto out;
-	}
 
 	id->did = strtol(s_did, &endptr, 16);
-	if (*endptr) {
-		r = -EINVAL;
+	if (*endptr)
 		goto out;
-	}
 
 	/* revision id */
 	s_rid = strtok_r(NULL, ",", &saveptr);
-	if (!s_rid) {
-		r = -EINVAL;
+	if (!s_rid)
 		goto out;
-	}
 
 	id->rid = strtol(s_rid, &endptr, 16);
-	if (*endptr) {
-		r = -EINVAL;
+	if (*endptr)
 		goto out;
-	}
 
 	/* marketing name */
 	s_name = strtok_r(NULL, ",", &saveptr);
-	if (!s_name) {
-		r = -EINVAL;
+	if (!s_name)
 		goto out;
-	}
+
 	/* trim leading whitespaces or tabs */
 	while (isblank(*s_name))
 		s_name++;
-	if (strlen(s_name) == 0) {
-		r = -EINVAL;
+	if (strlen(s_name) == 0)
 		goto out;
-	}
 
 	id->marketing_name = strdup(s_name);
-	if (id->marketing_name == NULL) {
-		r = -EINVAL;
-		goto out;
-	}
+	if (id->marketing_name)
+		r = 0;
+	else
+		r = -ENOMEM;
 
 out:
 	free(buf);
-- 
2.15.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 3/3] amdgpu: Only remember the device's marketing name

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

There's no point in keeping around the full table of marketing names,
when amdgpu_get_marketing_name only ever returns the device's marketing
name.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 amdgpu/Android.mk        |  3 +-
 amdgpu/Makefile.am       |  5 +--
 amdgpu/amdgpu_asic_id.c  | 88 ++++++++++++------------------------------------
 amdgpu/amdgpu_device.c   | 23 ++-----------
 amdgpu/amdgpu_internal.h | 11 ++----
 5 files changed, 28 insertions(+), 102 deletions(-)

diff --git a/amdgpu/Android.mk b/amdgpu/Android.mk
index ce273019..1f028d0b 100644
--- a/amdgpu/Android.mk
+++ b/amdgpu/Android.mk
@@ -11,8 +11,7 @@ LOCAL_SHARED_LIBRARIES := libdrm
 LOCAL_SRC_FILES := $(LIBDRM_AMDGPU_FILES)
 
 LOCAL_CFLAGS := \
-	-DAMDGPU_ASIC_ID_TABLE=\"/vendor/etc/hwdata/amdgpu.ids\" \
-	-DAMDGPU_ASIC_ID_TABLE_NUM_ENTRIES=$(shell egrep -ci '^[0-9a-f]{4},.*[0-9a-f]+,' $(LIBDRM_TOP)/data/amdgpu.ids)
+	-DAMDGPU_ASIC_ID_TABLE=\"/vendor/etc/hwdata/amdgpu.ids\"
 
 LOCAL_REQUIRED_MODULES := amdgpu.ids
 
diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am
index 66f6f676..a1b0d05c 100644
--- a/amdgpu/Makefile.am
+++ b/amdgpu/Makefile.am
@@ -31,10 +31,7 @@ AM_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 = -DAMDGPU_ASIC_ID_TABLE=\"${libdrmdatadir}/amdgpu.ids\" \
-	-DAMDGPU_ASIC_ID_TABLE_NUM_ENTRIES=$(ASIC_ID_TABLE_NUM_ENTRIES)
+AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${libdrmdatadir}/amdgpu.ids\"
 
 libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la
 libdrm_amdgpu_ladir = $(libdir)
diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c
index 0b5f2962..0c8925e5 100644
--- a/amdgpu/amdgpu_asic_id.c
+++ b/amdgpu/amdgpu_asic_id.c
@@ -38,11 +38,13 @@
 #include "amdgpu_drm.h"
 #include "amdgpu_internal.h"
 
-static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
+static int parse_one_line(struct amdgpu_device *dev, const char *line)
 {
 	char *buf, *saveptr;
 	char *s_did;
+	uint32_t did;
 	char *s_rid;
+	uint32_t rid;
 	char *s_name;
 	char *endptr;
 	int r = -EINVAL;
@@ -60,19 +62,29 @@ static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
 	if (!s_did)
 		goto out;
 
-	id->did = strtol(s_did, &endptr, 16);
+	did = strtol(s_did, &endptr, 16);
 	if (*endptr)
 		goto out;
 
+	if (did != dev->info.asic_id) {
+		r = -EAGAIN;
+		goto out;
+	}
+
 	/* revision id */
 	s_rid = strtok_r(NULL, ",", &saveptr);
 	if (!s_rid)
 		goto out;
 
-	id->rid = strtol(s_rid, &endptr, 16);
+	rid = strtol(s_rid, &endptr, 16);
 	if (*endptr)
 		goto out;
 
+	if (rid != dev->info.pci_rev_id) {
+		r = -EAGAIN;
+		goto out;
+	}
+
 	/* marketing name */
 	s_name = strtok_r(NULL, ",", &saveptr);
 	if (!s_name)
@@ -84,8 +96,8 @@ static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
 	if (strlen(s_name) == 0)
 		goto out;
 
-	id->marketing_name = strdup(s_name);
-	if (id->marketing_name)
+	dev->marketing_name = strdup(s_name);
+	if (dev->marketing_name)
 		r = 0;
 	else
 		r = -ENOMEM;
@@ -96,17 +108,13 @@ out:
 	return r;
 }
 
-void amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
+void amdgpu_parse_asic_ids(struct amdgpu_device *dev)
 {
-	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 = AMDGPU_ASIC_ID_TABLE_NUM_ENTRIES;
 	int r = 0;
 
 	fp = fopen(AMDGPU_ASIC_ID_TABLE, "r");
@@ -116,13 +124,6 @@ void amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
 		return;
 	}
 
-	asic_id_table = calloc(table_max_size + 1,
-			       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 */
@@ -140,52 +141,17 @@ void amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
 	}
 
 	while ((n = getline(&line, &len, fp)) != -1) {
-		if (table_size > table_max_size) {
-			/* double table size */
-			table_max_size *= 2;
-			id = realloc(asic_id_table, (table_max_size + 1) *
-				     sizeof(struct amdgpu_asic_id));
-			if (!id) {
-				r = -ENOMEM;
-				goto free;
-			}
-                        asic_id_table = id;
-		}
-
-		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;
-			}
-			goto free;
-		}
+		r = parse_one_line(dev, line);
+		if (r != -EAGAIN)
+			break;
 
 		line_num++;
-		table_size++;
 	}
 
-	if (table_size != table_max_size) {
-		id = realloc(asic_id_table, (table_size + 1) *
-			     sizeof(struct amdgpu_asic_id));
-		if (!id) {
-			r = -ENOMEM;
-			goto free;
-		}
-		asic_id_table = id;
-        }
-
-	/* end of table */
-	id = asic_id_table + table_size;
-	memset(id, 0, sizeof(struct amdgpu_asic_id));
-
-free:
 	if (r == -EINVAL) {
 		fprintf(stderr, "Invalid format: %s: line %d: %s\n",
 			AMDGPU_ASIC_ID_TABLE, line_num, line);
@@ -195,17 +161,5 @@ free:
 	}
 
 	free(line);
-
-	if (r && asic_id_table) {
-		while (table_size--) {
-			id = asic_id_table + table_size;
-			free(id->marketing_name);
-		}
-		free(asic_id_table);
-		asic_id_table = NULL;
-	}
-close:
 	fclose(fp);
-
-	*p_asic_id_table = asic_id_table;
 }
diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
index e7aaf4fc..eb4b2745 100644
--- a/amdgpu/amdgpu_device.c
+++ b/amdgpu/amdgpu_device.c
@@ -130,7 +130,6 @@ 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,12 +139,7 @@ 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++)
-			free(id->marketing_name);
-
-		free(dev->asic_ids);
-	}
+	free(dev->marketing_name);
 	free(dev);
 }
 
@@ -280,7 +274,7 @@ int amdgpu_device_initialize(int fd,
 	amdgpu_vamgr_init(&dev->vamgr, start, max,
 			  dev->dev_info.virtual_address_alignment);
 
-	amdgpu_parse_asic_ids(&dev->asic_ids);
+	amdgpu_parse_asic_ids(dev);
 
 	*major_version = dev->major_version;
 	*minor_version = dev->minor_version;
@@ -306,16 +300,5 @@ int amdgpu_device_deinitialize(amdgpu_device_handle dev)
 
 const char *amdgpu_get_marketing_name(amdgpu_device_handle dev)
 {
-	const struct amdgpu_asic_id *id;
-
-	if (!dev->asic_ids)
-		return NULL;
-
-	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;
+	return dev->marketing_name;
 }
diff --git a/amdgpu/amdgpu_internal.h b/amdgpu/amdgpu_internal.h
index 1aff7f8e..3e044f11 100644
--- a/amdgpu/amdgpu_internal.h
+++ b/amdgpu/amdgpu_internal.h
@@ -69,12 +69,6 @@ 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;
@@ -82,8 +76,7 @@ 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;
+	char *marketing_name;
 	/** 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. */
@@ -148,7 +141,7 @@ drm_private void amdgpu_vamgr_init(struct amdgpu_bo_va_mgr *mgr, uint64_t start,
 
 drm_private void amdgpu_vamgr_deinit(struct amdgpu_bo_va_mgr *mgr);
 
-drm_private void amdgpu_parse_asic_ids(struct amdgpu_asic_id **asic_ids);
+drm_private void amdgpu_parse_asic_ids(struct amdgpu_device *dev);
 
 drm_private int amdgpu_query_gpu_info_init(amdgpu_device_handle dev);
 
-- 
2.15.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 0/3] amdgpu: amdgpu_get_marketing_name cleanups

[Slava Abramov]

On 2017-12-01 11:56 AM, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> The first two patches are preparatory for patch 3, which is the meat of
> the series.
>
> Michel Dänzer (3):
>    amdgpu: Clean up amdgpu_parse_asic_ids error handling
>    amdgpu: Simplify error handling in parse_one_line
>    amdgpu: Only remember the device's marketing name
>
>   amdgpu/Android.mk        |   3 +-
>   amdgpu/Makefile.am       |   5 +-
>   amdgpu/amdgpu_asic_id.c  | 139 ++++++++++++++---------------------------------
>   amdgpu/amdgpu_device.c   |  27 +--------
>   amdgpu/amdgpu_internal.h |  11 +---
>   5 files changed, 49 insertions(+), 136 deletions(-)
>

Series
Acked-by: Slava Abramov <slava.abramov@amd.com>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 3/3] amdgpu: Only remember the device's marketing name

[Alex Deucher]

On Fri, Dec 1, 2017 at 11:56 AM, Michel Dänzer <michel@daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> There's no point in keeping around the full table of marketing names,
> when amdgpu_get_marketing_name only ever returns the device's marketing
> name.
>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

Series is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  amdgpu/Android.mk        |  3 +-
>  amdgpu/Makefile.am       |  5 +--
>  amdgpu/amdgpu_asic_id.c  | 88 ++++++++++++------------------------------------
>  amdgpu/amdgpu_device.c   | 23 ++-----------
>  amdgpu/amdgpu_internal.h | 11 ++----
>  5 files changed, 28 insertions(+), 102 deletions(-)
>
> diff --git a/amdgpu/Android.mk b/amdgpu/Android.mk
> index ce273019..1f028d0b 100644
> --- a/amdgpu/Android.mk
> +++ b/amdgpu/Android.mk
> @@ -11,8 +11,7 @@ LOCAL_SHARED_LIBRARIES := libdrm
>  LOCAL_SRC_FILES := $(LIBDRM_AMDGPU_FILES)
>
>  LOCAL_CFLAGS := \
> -       -DAMDGPU_ASIC_ID_TABLE=\"/vendor/etc/hwdata/amdgpu.ids\" \
> -       -DAMDGPU_ASIC_ID_TABLE_NUM_ENTRIES=$(shell egrep -ci '^[0-9a-f]{4},.*[0-9a-f]+,' $(LIBDRM_TOP)/data/amdgpu.ids)
> +       -DAMDGPU_ASIC_ID_TABLE=\"/vendor/etc/hwdata/amdgpu.ids\"
>
>  LOCAL_REQUIRED_MODULES := amdgpu.ids
>
> diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am
> index 66f6f676..a1b0d05c 100644
> --- a/amdgpu/Makefile.am
> +++ b/amdgpu/Makefile.am
> @@ -31,10 +31,7 @@ AM_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 = -DAMDGPU_ASIC_ID_TABLE=\"${libdrmdatadir}/amdgpu.ids\" \
> -       -DAMDGPU_ASIC_ID_TABLE_NUM_ENTRIES=$(ASIC_ID_TABLE_NUM_ENTRIES)
> +AM_CPPFLAGS = -DAMDGPU_ASIC_ID_TABLE=\"${libdrmdatadir}/amdgpu.ids\"
>
>  libdrm_amdgpu_la_LTLIBRARIES = libdrm_amdgpu.la
>  libdrm_amdgpu_ladir = $(libdir)
> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c
> index 0b5f2962..0c8925e5 100644
> --- a/amdgpu/amdgpu_asic_id.c
> +++ b/amdgpu/amdgpu_asic_id.c
> @@ -38,11 +38,13 @@
>  #include "amdgpu_drm.h"
>  #include "amdgpu_internal.h"
>
> -static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
> +static int parse_one_line(struct amdgpu_device *dev, const char *line)
>  {
>         char *buf, *saveptr;
>         char *s_did;
> +       uint32_t did;
>         char *s_rid;
> +       uint32_t rid;
>         char *s_name;
>         char *endptr;
>         int r = -EINVAL;
> @@ -60,19 +62,29 @@ static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
>         if (!s_did)
>                 goto out;
>
> -       id->did = strtol(s_did, &endptr, 16);
> +       did = strtol(s_did, &endptr, 16);
>         if (*endptr)
>                 goto out;
>
> +       if (did != dev->info.asic_id) {
> +               r = -EAGAIN;
> +               goto out;
> +       }
> +
>         /* revision id */
>         s_rid = strtok_r(NULL, ",", &saveptr);
>         if (!s_rid)
>                 goto out;
>
> -       id->rid = strtol(s_rid, &endptr, 16);
> +       rid = strtol(s_rid, &endptr, 16);
>         if (*endptr)
>                 goto out;
>
> +       if (rid != dev->info.pci_rev_id) {
> +               r = -EAGAIN;
> +               goto out;
> +       }
> +
>         /* marketing name */
>         s_name = strtok_r(NULL, ",", &saveptr);
>         if (!s_name)
> @@ -84,8 +96,8 @@ static int parse_one_line(const char *line, struct amdgpu_asic_id *id)
>         if (strlen(s_name) == 0)
>                 goto out;
>
> -       id->marketing_name = strdup(s_name);
> -       if (id->marketing_name)
> +       dev->marketing_name = strdup(s_name);
> +       if (dev->marketing_name)
>                 r = 0;
>         else
>                 r = -ENOMEM;
> @@ -96,17 +108,13 @@ out:
>         return r;
>  }
>
> -void amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
> +void amdgpu_parse_asic_ids(struct amdgpu_device *dev)
>  {
> -       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 = AMDGPU_ASIC_ID_TABLE_NUM_ENTRIES;
>         int r = 0;
>
>         fp = fopen(AMDGPU_ASIC_ID_TABLE, "r");
> @@ -116,13 +124,6 @@ void amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
>                 return;
>         }
>
> -       asic_id_table = calloc(table_max_size + 1,
> -                              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 */
> @@ -140,52 +141,17 @@ void amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
>         }
>
>         while ((n = getline(&line, &len, fp)) != -1) {
> -               if (table_size > table_max_size) {
> -                       /* double table size */
> -                       table_max_size *= 2;
> -                       id = realloc(asic_id_table, (table_max_size + 1) *
> -                                    sizeof(struct amdgpu_asic_id));
> -                       if (!id) {
> -                               r = -ENOMEM;
> -                               goto free;
> -                       }
> -                        asic_id_table = id;
> -               }
> -
> -               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;
> -                       }
> -                       goto free;
> -               }
> +               r = parse_one_line(dev, line);
> +               if (r != -EAGAIN)
> +                       break;
>
>                 line_num++;
> -               table_size++;
>         }
>
> -       if (table_size != table_max_size) {
> -               id = realloc(asic_id_table, (table_size + 1) *
> -                            sizeof(struct amdgpu_asic_id));
> -               if (!id) {
> -                       r = -ENOMEM;
> -                       goto free;
> -               }
> -               asic_id_table = id;
> -        }
> -
> -       /* end of table */
> -       id = asic_id_table + table_size;
> -       memset(id, 0, sizeof(struct amdgpu_asic_id));
> -
> -free:
>         if (r == -EINVAL) {
>                 fprintf(stderr, "Invalid format: %s: line %d: %s\n",
>                         AMDGPU_ASIC_ID_TABLE, line_num, line);
> @@ -195,17 +161,5 @@ free:
>         }
>
>         free(line);
> -
> -       if (r && asic_id_table) {
> -               while (table_size--) {
> -                       id = asic_id_table + table_size;
> -                       free(id->marketing_name);
> -               }
> -               free(asic_id_table);
> -               asic_id_table = NULL;
> -       }
> -close:
>         fclose(fp);
> -
> -       *p_asic_id_table = asic_id_table;
>  }
> diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
> index e7aaf4fc..eb4b2745 100644
> --- a/amdgpu/amdgpu_device.c
> +++ b/amdgpu/amdgpu_device.c
> @@ -130,7 +130,6 @@ 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,12 +139,7 @@ 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++)
> -                       free(id->marketing_name);
> -
> -               free(dev->asic_ids);
> -       }
> +       free(dev->marketing_name);
>         free(dev);
>  }
>
> @@ -280,7 +274,7 @@ int amdgpu_device_initialize(int fd,
>         amdgpu_vamgr_init(&dev->vamgr, start, max,
>                           dev->dev_info.virtual_address_alignment);
>
> -       amdgpu_parse_asic_ids(&dev->asic_ids);
> +       amdgpu_parse_asic_ids(dev);
>
>         *major_version = dev->major_version;
>         *minor_version = dev->minor_version;
> @@ -306,16 +300,5 @@ int amdgpu_device_deinitialize(amdgpu_device_handle dev)
>
>  const char *amdgpu_get_marketing_name(amdgpu_device_handle dev)
>  {
> -       const struct amdgpu_asic_id *id;
> -
> -       if (!dev->asic_ids)
> -               return NULL;
> -
> -       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;
> +       return dev->marketing_name;
>  }
> diff --git a/amdgpu/amdgpu_internal.h b/amdgpu/amdgpu_internal.h
> index 1aff7f8e..3e044f11 100644
> --- a/amdgpu/amdgpu_internal.h
> +++ b/amdgpu/amdgpu_internal.h
> @@ -69,12 +69,6 @@ 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;
> @@ -82,8 +76,7 @@ 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;
> +       char *marketing_name;
>         /** 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. */
> @@ -148,7 +141,7 @@ drm_private void amdgpu_vamgr_init(struct amdgpu_bo_va_mgr *mgr, uint64_t start,
>
>  drm_private void amdgpu_vamgr_deinit(struct amdgpu_bo_va_mgr *mgr);
>
> -drm_private void amdgpu_parse_asic_ids(struct amdgpu_asic_id **asic_ids);
> +drm_private void amdgpu_parse_asic_ids(struct amdgpu_device *dev);
>
>  drm_private int amdgpu_query_gpu_info_init(amdgpu_device_handle dev);
>
> --
> 2.15.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 1/3] amdgpu: Clean up amdgpu_parse_asic_ids error handling

[Marek Olšák]

On Fri, Dec 1, 2017 at 5:56 PM, Michel Dänzer <michel@daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> * Move error message printing into amdgpu_parse_asic_ids and make it
>   return void
> * Print only "Invalid format" error message if parse_one_line returns
>   -EINVAL
> * Use strerror instead of printing the (negative) error code in hex
>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  amdgpu/amdgpu_asic_id.c  | 16 ++++++++++------
>  amdgpu/amdgpu_device.c   |  6 +-----
>  amdgpu/amdgpu_internal.h |  2 +-
>  3 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c
> index e8218974..eb42bbc2 100644
> --- a/amdgpu/amdgpu_asic_id.c
> +++ b/amdgpu/amdgpu_asic_id.c
> @@ -109,7 +109,7 @@ out:
>         return r;
>  }
>
> -int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
> +void amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
>  {
>         struct amdgpu_asic_id *asic_id_table;
>         struct amdgpu_asic_id *id;
> @@ -126,7 +126,7 @@ int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
>         if (!fp) {
>                 fprintf(stderr, "%s: %s\n", AMDGPU_ASIC_ID_TABLE,
>                         strerror(errno));
> -               return -EINVAL;
> +               return;
>         }
>
>         asic_id_table = calloc(table_max_size + 1,
> @@ -177,8 +177,6 @@ int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
>                                 line_num++;
>                                 continue;
>                         }
> -                       fprintf(stderr, "Invalid format: %s: line %d: %s\n",
> -                               AMDGPU_ASIC_ID_TABLE, line_num, line);
>                         goto free;
>                 }
>
> @@ -201,6 +199,14 @@ int amdgpu_parse_asic_ids(struct amdgpu_asic_id **p_asic_id_table)
>         memset(id, 0, sizeof(struct amdgpu_asic_id));
>
>  free:
> +       if (r == -EINVAL) {
> +               fprintf(stderr, "Invalid format: %s: line %d: %s\n",
> +                       AMDGPU_ASIC_ID_TABLE, line_num, line);
> +       } else if (r) {
> +               fprintf(stderr, "%s: Cannot parse ASIC IDs: %s\n",
> +                       __func__, strerror(-r));

This is a good example how strerror should not be used in graphics
drivers. It makes no sense:
"amdgpu_parse_asic_ids: Cannot parse ASIC IDs: Resource temporarily unavailable"

Is my graphics card temporary unavailable?

Marek
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm] amdgpu: Don't print error message if parse_one_line returned -EAGAIN

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

It means it just didn't find an entry for the GPU in the amdgpu.ids file.

Fixes spurious

 amdgpu_parse_asic_ids: Cannot parse ASIC IDs: Resource temporarily unavailable

error messages in that case.

Reported-by: Marek Olšák <marek.olsak@amd.com>
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 amdgpu/amdgpu_asic_id.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c
index 0c8925e5..62459c09 100644
--- a/amdgpu/amdgpu_asic_id.c
+++ b/amdgpu/amdgpu_asic_id.c
@@ -155,7 +155,7 @@ void amdgpu_parse_asic_ids(struct amdgpu_device *dev)
 	if (r == -EINVAL) {
 		fprintf(stderr, "Invalid format: %s: line %d: %s\n",
 			AMDGPU_ASIC_ID_TABLE, line_num, line);
-	} else if (r) {
+	} else if (r && r != -EAGAIN) {
 		fprintf(stderr, "%s: Cannot parse ASIC IDs: %s\n",
 			__func__, strerror(-r));
 	}
-- 
2.15.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: Don't print error message if parse_one_line returned -EAGAIN

[Marek Olšák]

Reviewed-by: Marek Olšák <marek.olsak@amd.com>

Marek

On Mon, Jan 8, 2018 at 11:24 AM, Michel Dänzer <michel@daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> It means it just didn't find an entry for the GPU in the amdgpu.ids file.
>
> Fixes spurious
>
>  amdgpu_parse_asic_ids: Cannot parse ASIC IDs: Resource temporarily unavailable
>
> error messages in that case.
>
> Reported-by: Marek Olšák <marek.olsak@amd.com>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  amdgpu/amdgpu_asic_id.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/amdgpu/amdgpu_asic_id.c b/amdgpu/amdgpu_asic_id.c
> index 0c8925e5..62459c09 100644
> --- a/amdgpu/amdgpu_asic_id.c
> +++ b/amdgpu/amdgpu_asic_id.c
> @@ -155,7 +155,7 @@ void amdgpu_parse_asic_ids(struct amdgpu_device *dev)
>         if (r == -EINVAL) {
>                 fprintf(stderr, "Invalid format: %s: line %d: %s\n",
>                         AMDGPU_ASIC_ID_TABLE, line_num, line);
> -       } else if (r) {
> +       } else if (r && r != -EAGAIN) {
>                 fprintf(stderr, "%s: Cannot parse ASIC IDs: %s\n",
>                         __func__, strerror(-r));
>         }
> --
> 2.15.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx