On 30/05/17 06:16 AM, Samuel Li wrote:
> From: Xiaojie Yuan <Xiaojie.Yuan at 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
