> If you add this here, you should add the ids file itself and make libdrm install it too...
? Here the ids file is separate from libdrm. It is passed during compilation so that libdrm knows where to get it.
> You can't use strtok() in a library. Any other thread may call strtok() anytime too and screw you up.
Right, better to call strtok_r() instead.
> The manpage says len must be 0 before the first getline() call.
Right, that shall be safer.
>> + printf("%s version: %s\n", AMDGPU_ASIC_ID_TABLE, line);
>debug leftover?
This is to print out the ids file version.
Thanks,
Sam
-----Original Message-----
From: Grazvydas Ignotas [mailto:notasas@xxxxxxxxx]
Sent: Friday, May 12, 2017 8:16 AM
To: Li, Samuel <Samuel.Li at amd.com>
Cc: amd-gfx at lists.freedesktop.org; Yuan, Xiaojie <Xiaojie.Yuan at amd.com>
Subject: Re: [PATCH libdrm v2 1/1] amdgpu: move asic id table to a separate file
On Fri, May 12, 2017 at 12:19 AM, Samuel Li <Samuel.Li at amd.com> wrote:
> From: Xiaojie Yuan <Xiaojie.Yuan at amd.com>
>
> v2: fix an off by one error and leading white spaces
>
> Change-Id: I12216da14910f5e2b0970bc1fafc2a20b0ef1ba9
> Reviewed-by: Junwei Zhang <Jerry.Zhang at amd.com>
> Signed-off-by: Samuel Li <Samuel.Li at amd.com>
> ---
> amdgpu/Makefile.am | 2 +
> amdgpu/Makefile.sources | 2 +-
> amdgpu/amdgpu_asic_id.c | 198
> +++++++++++++++++++++++++++++++++++++++++++++++
> amdgpu/amdgpu_asic_id.h | 165 ---------------------------------------
> amdgpu/amdgpu_device.c | 28 +++++--
> amdgpu/amdgpu_internal.h | 10 +++
> 6 files changed, 232 insertions(+), 173 deletions(-) create mode
> 100644 amdgpu/amdgpu_asic_id.c delete mode 100644
> amdgpu/amdgpu_asic_id.h
>
> diff --git a/amdgpu/Makefile.am b/amdgpu/Makefile.am index
> cf7bc1b..ecf9e82 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=\"${datadir}/libdrm/amdgpu.ids\"
If you add this here, you should add the ids file itself and make libdrm install it too...
> +
> 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..067f38c
> --- /dev/null
> +++ b/amdgpu/amdgpu_asic_id.c
> @@ -0,0 +1,198 @@
> +/*
> + * 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 "amdgpu_drm.h"
> +#include "amdgpu_internal.h"
> +
> +static int parse_one_line(const char *line, struct amdgpu_asic_id
> +*id) {
> + char *buf;
> + 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(buf, ",");
You can't use strtok() in a library. Any other thread may call
strtok() anytime too and screw you up.
> + 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(NULL, ",");
> + 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(NULL, ",");
> + if (!s_name) {
> + 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;
The manpage says len must be 0 before the first getline() call.
> + 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 line is file version */
> + if ((n = getline(&line, &len, fp)) != -1) {
> + /* trim trailing newline */
> + if (line[n - 1] == '\n')
> + line[n - 1] = '\0';
> + printf("%s version: %s\n", AMDGPU_ASIC_ID_TABLE,
> + line);
debug leftover?
Gražvydas
