[PATCH libdrm v4 1/1] amdgpu: move asic id table to a separate file

[Li, Samuel]

Please see comments inline.

-----Original Message-----
From: Michel Dänzer [mailto:michel at daenzer.net] 
Sent: Monday, May 29, 2017 9:26 PM
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 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 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.

 [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