[PATCH] drm/amdgpu: Add sysfs file for VBIOS

[PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Kent Russell]

This won't change after initialization, so we can add the information
when we parse the atombios information. This ensures that we can find
out the VBIOS, even when the dmesg buffer fills up, and makes it easier
to associate which VBIOS is for which GPU on mGPU configurations. Set
the size to 20 characters in case of some weird VBIOS suffix that exceeds
the expected 17 character format (3-8-3\0)

Signed-off-by: Kent Russell <kent.russell@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
 drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a1f9424..f40be71 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
 	return r;
 }
 
+static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	struct drm_device *ddev = dev_get_drvdata(dev);
+	struct amdgpu_device *adev = ddev->dev_private;
+	struct atom_context *ctx = adev->mode_info.atom_context;
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version);
+}
+
+static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
+		   NULL);
+
 /**
  * amdgpu_atombios_fini - free the driver info and callbacks for atombios
  *
@@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
 	adev->mode_info.atom_context = NULL;
 	kfree(adev->mode_info.atom_card_info);
 	adev->mode_info.atom_card_info = NULL;
+	device_remove_file(adev->dev, &dev_attr_vbios_version);
 }
 
 /**
@@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
 {
 	struct card_info *atom_card_info =
 	    kzalloc(sizeof(struct card_info), GFP_KERNEL);
+	int ret;
 
 	if (!atom_card_info)
 		return -ENOMEM;
@@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
 		amdgpu_atombios_scratch_regs_init(adev);
 		amdgpu_atombios_allocate_fb_scratch(adev);
 	}
+
+	ret = device_create_file(adev->dev, &dev_attr_vbios_version);
+	if (ret) {
+		DRM_ERROR("Failed to create device file for VBIOS version\n");
+		return ret;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/amdgpu/atom.c
index d69aa2e..69500a8 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.c
+++ b/drivers/gpu/drm/amd/amdgpu/atom.c
@@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
 		idx = 0x80;
 
 	str = CSTR(idx);
-	if (*str != '\0')
+	if (*str != '\0') {
 		pr_info("ATOM BIOS: %s\n", str);
+		strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
+	}
+
 
 	return ctx;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h b/drivers/gpu/drm/amd/amdgpu/atom.h
index ddd8045..a391709 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.h
+++ b/drivers/gpu/drm/amd/amdgpu/atom.h
@@ -140,6 +140,7 @@ struct atom_context {
 	int io_mode;
 	uint32_t *scratch;
 	int scratch_size_bytes;
+	char vbios_version[20];
 };
 
 extern int amdgpu_atom_debug;
-- 
2.7.4

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

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Michel Dänzer]

On 24/08/17 03:12 AM, Kent Russell wrote:
> This won't change after initialization, so we can add the information
> when we parse the atombios information. This ensures that we can find
> out the VBIOS, even when the dmesg buffer fills up, and makes it easier
> to associate which VBIOS is for which GPU on mGPU configurations. Set
> the size to 20 characters in case of some weird VBIOS suffix that exceeds
> the expected 17 character format (3-8-3\0)
> 
> Signed-off-by: Kent Russell <kent.russell@amd.com>

Most if not all instances of "VBIOS" in the commit log should probably
be replaced by "VBIOS version" for clarity.


-- 
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

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Christian König]

Am 23.08.2017 um 20:12 schrieb Kent Russell:
> This won't change after initialization, so we can add the information
> when we parse the atombios information. This ensures that we can find
> out the VBIOS, even when the dmesg buffer fills up, and makes it easier
> to associate which VBIOS is for which GPU on mGPU configurations. Set
> the size to 20 characters in case of some weird VBIOS suffix that exceeds
> the expected 17 character format (3-8-3\0)

Is there any reason that needs to be sysfs? Sounds more like an use case 
for debugfs.

Christian.

>
> Signed-off-by: Kent Russell <kent.russell@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>   drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>   3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a1f9424..f40be71 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>   	return r;
>   }
>   
> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct drm_device *ddev = dev_get_drvdata(dev);
> +	struct amdgpu_device *adev = ddev->dev_private;
> +	struct atom_context *ctx = adev->mode_info.atom_context;
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version);
> +}
> +
> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
> +		   NULL);
> +
>   /**
>    * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>    *
> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>   	adev->mode_info.atom_context = NULL;
>   	kfree(adev->mode_info.atom_card_info);
>   	adev->mode_info.atom_card_info = NULL;
> +	device_remove_file(adev->dev, &dev_attr_vbios_version);
>   }
>   
>   /**
> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>   {
>   	struct card_info *atom_card_info =
>   	    kzalloc(sizeof(struct card_info), GFP_KERNEL);
> +	int ret;
>   
>   	if (!atom_card_info)
>   		return -ENOMEM;
> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>   		amdgpu_atombios_scratch_regs_init(adev);
>   		amdgpu_atombios_allocate_fb_scratch(adev);
>   	}
> +
> +	ret = device_create_file(adev->dev, &dev_attr_vbios_version);
> +	if (ret) {
> +		DRM_ERROR("Failed to create device file for VBIOS version\n");
> +		return ret;
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/amdgpu/atom.c
> index d69aa2e..69500a8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>   		idx = 0x80;
>   
>   	str = CSTR(idx);
> -	if (*str != '\0')
> +	if (*str != '\0') {
>   		pr_info("ATOM BIOS: %s\n", str);
> +		strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
> +	}
> +
>   
>   	return ctx;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h b/drivers/gpu/drm/amd/amdgpu/atom.h
> index ddd8045..a391709 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
> @@ -140,6 +140,7 @@ struct atom_context {
>   	int io_mode;
>   	uint32_t *scratch;
>   	int scratch_size_bytes;
> +	char vbios_version[20];
>   };
>   
>   extern int amdgpu_atom_debug;


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

RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Russell, Kent]

No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!

 Kent

-----Original Message-----
From: Christian König [mailto:deathsimple@vodafone.de] 
Sent: Thursday, August 24, 2017 2:22 AM
To: Russell, Kent; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

Am 23.08.2017 um 20:12 schrieb Kent Russell:
> This won't change after initialization, so we can add the information 
> when we parse the atombios information. This ensures that we can find 
> out the VBIOS, even when the dmesg buffer fills up, and makes it 
> easier to associate which VBIOS is for which GPU on mGPU 
> configurations. Set the size to 20 characters in case of some weird 
> VBIOS suffix that exceeds the expected 17 character format (3-8-3\0)

Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.

Christian.

>
> Signed-off-by: Kent Russell <kent.russell@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>   drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>   3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a1f9424..f40be71 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>   	return r;
>   }
>   
> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct drm_device *ddev = dev_get_drvdata(dev);
> +	struct amdgpu_device *adev = ddev->dev_private;
> +	struct atom_context *ctx = adev->mode_info.atom_context;
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); }
> +
> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
> +		   NULL);
> +
>   /**
>    * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>    *
> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>   	adev->mode_info.atom_context = NULL;
>   	kfree(adev->mode_info.atom_card_info);
>   	adev->mode_info.atom_card_info = NULL;
> +	device_remove_file(adev->dev, &dev_attr_vbios_version);
>   }
>   
>   /**
> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>   {
>   	struct card_info *atom_card_info =
>   	    kzalloc(sizeof(struct card_info), GFP_KERNEL);
> +	int ret;
>   
>   	if (!atom_card_info)
>   		return -ENOMEM;
> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>   		amdgpu_atombios_scratch_regs_init(adev);
>   		amdgpu_atombios_allocate_fb_scratch(adev);
>   	}
> +
> +	ret = device_create_file(adev->dev, &dev_attr_vbios_version);
> +	if (ret) {
> +		DRM_ERROR("Failed to create device file for VBIOS version\n");
> +		return ret;
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c 
> b/drivers/gpu/drm/amd/amdgpu/atom.c
> index d69aa2e..69500a8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>   		idx = 0x80;
>   
>   	str = CSTR(idx);
> -	if (*str != '\0')
> +	if (*str != '\0') {
>   		pr_info("ATOM BIOS: %s\n", str);
> +		strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
> +	}
> +
>   
>   	return ctx;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h 
> b/drivers/gpu/drm/amd/amdgpu/atom.h
> index ddd8045..a391709 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
> @@ -140,6 +140,7 @@ struct atom_context {
>   	int io_mode;
>   	uint32_t *scratch;
>   	int scratch_size_bytes;
> +	char vbios_version[20];
>   };
>   
>   extern int amdgpu_atom_debug;


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

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Alex Deucher]

On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote:
> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!
>

While you are at it, can you expose the vbios binary itself via
debugfs?  That's been on by todo list for a while.

Alex

>  Kent
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Thursday, August 24, 2017 2:22 AM
> To: Russell, Kent; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> Am 23.08.2017 um 20:12 schrieb Kent Russell:
>> This won't change after initialization, so we can add the information
>> when we parse the atombios information. This ensures that we can find
>> out the VBIOS, even when the dmesg buffer fills up, and makes it
>> easier to associate which VBIOS is for which GPU on mGPU
>> configurations. Set the size to 20 characters in case of some weird
>> VBIOS suffix that exceeds the expected 17 character format (3-8-3\0)
>
> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.
>
> Christian.
>
>>
>> Signed-off-by: Kent Russell <kent.russell@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>>   drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>>   3 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a1f9424..f40be71 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>>       return r;
>>   }
>>
>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
>> +                                              struct device_attribute *attr,
>> +                                              char *buf)
>> +{
>> +     struct drm_device *ddev = dev_get_drvdata(dev);
>> +     struct amdgpu_device *adev = ddev->dev_private;
>> +     struct atom_context *ctx = adev->mode_info.atom_context;
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); }
>> +
>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>> +                NULL);
>> +
>>   /**
>>    * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>>    *
>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>>       adev->mode_info.atom_context = NULL;
>>       kfree(adev->mode_info.atom_card_info);
>>       adev->mode_info.atom_card_info = NULL;
>> +     device_remove_file(adev->dev, &dev_attr_vbios_version);
>>   }
>>
>>   /**
>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>   {
>>       struct card_info *atom_card_info =
>>           kzalloc(sizeof(struct card_info), GFP_KERNEL);
>> +     int ret;
>>
>>       if (!atom_card_info)
>>               return -ENOMEM;
>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>               amdgpu_atombios_scratch_regs_init(adev);
>>               amdgpu_atombios_allocate_fb_scratch(adev);
>>       }
>> +
>> +     ret = device_create_file(adev->dev, &dev_attr_vbios_version);
>> +     if (ret) {
>> +             DRM_ERROR("Failed to create device file for VBIOS version\n");
>> +             return ret;
>> +     }
>> +
>>       return 0;
>>   }
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>> index d69aa2e..69500a8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>>               idx = 0x80;
>>
>>       str = CSTR(idx);
>> -     if (*str != '\0')
>> +     if (*str != '\0') {
>>               pr_info("ATOM BIOS: %s\n", str);
>> +             strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
>> +     }
>> +
>>
>>       return ctx;
>>   }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
>> b/drivers/gpu/drm/amd/amdgpu/atom.h
>> index ddd8045..a391709 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
>> @@ -140,6 +140,7 @@ struct atom_context {
>>       int io_mode;
>>       uint32_t *scratch;
>>       int scratch_size_bytes;
>> +     char vbios_version[20];
>>   };
>>
>>   extern int amdgpu_atom_debug;
>
>
> _______________________________________________
> 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] drm/amdgpu: Add sysfs file for VBIOS

[Russell, Kent]

I can definitely add that as well.

 Kent

-----Original Message-----
From: Alex Deucher [mailto:alexdeucher@gmail.com] 
Sent: Thursday, August 24, 2017 8:56 AM
To: Russell, Kent
Cc: Christian König; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote:
> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!
>

While you are at it, can you expose the vbios binary itself via debugfs?  That's been on by todo list for a while.

Alex

>  Kent
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Thursday, August 24, 2017 2:22 AM
> To: Russell, Kent; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> Am 23.08.2017 um 20:12 schrieb Kent Russell:
>> This won't change after initialization, so we can add the information 
>> when we parse the atombios information. This ensures that we can find 
>> out the VBIOS, even when the dmesg buffer fills up, and makes it 
>> easier to associate which VBIOS is for which GPU on mGPU 
>> configurations. Set the size to 20 characters in case of some weird 
>> VBIOS suffix that exceeds the expected 17 character format (3-8-3\0)
>
> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.
>
> Christian.
>
>>
>> Signed-off-by: Kent Russell <kent.russell@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>>   drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>>   3 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a1f9424..f40be71 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>>       return r;
>>   }
>>
>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
>> +                                              struct device_attribute *attr,
>> +                                              char *buf) {
>> +     struct drm_device *ddev = dev_get_drvdata(dev);
>> +     struct amdgpu_device *adev = ddev->dev_private;
>> +     struct atom_context *ctx = adev->mode_info.atom_context;
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); }
>> +
>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>> +                NULL);
>> +
>>   /**
>>    * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>>    *
>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>>       adev->mode_info.atom_context = NULL;
>>       kfree(adev->mode_info.atom_card_info);
>>       adev->mode_info.atom_card_info = NULL;
>> +     device_remove_file(adev->dev, &dev_attr_vbios_version);
>>   }
>>
>>   /**
>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>   {
>>       struct card_info *atom_card_info =
>>           kzalloc(sizeof(struct card_info), GFP_KERNEL);
>> +     int ret;
>>
>>       if (!atom_card_info)
>>               return -ENOMEM;
>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>               amdgpu_atombios_scratch_regs_init(adev);
>>               amdgpu_atombios_allocate_fb_scratch(adev);
>>       }
>> +
>> +     ret = device_create_file(adev->dev, &dev_attr_vbios_version);
>> +     if (ret) {
>> +             DRM_ERROR("Failed to create device file for VBIOS version\n");
>> +             return ret;
>> +     }
>> +
>>       return 0;
>>   }
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>> index d69aa2e..69500a8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>>               idx = 0x80;
>>
>>       str = CSTR(idx);
>> -     if (*str != '\0')
>> +     if (*str != '\0') {
>>               pr_info("ATOM BIOS: %s\n", str);
>> +             strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
>> +     }
>> +
>>
>>       return ctx;
>>   }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
>> b/drivers/gpu/drm/amd/amdgpu/atom.h
>> index ddd8045..a391709 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
>> @@ -140,6 +140,7 @@ struct atom_context {
>>       int io_mode;
>>       uint32_t *scratch;
>>       int scratch_size_bytes;
>> +     char vbios_version[20];
>>   };
>>
>>   extern int amdgpu_atom_debug;
>
>
> _______________________________________________
> 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] drm/amdgpu: Add sysfs file for VBIOS

[Kuehling, Felix]

Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs?

I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi.

Regards,
  Felix
________________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Russell, Kent <Kent.Russell@amd.com>
Sent: Thursday, August 24, 2017 9:06:22 AM
To: Alex Deucher
Cc: Christian König; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

I can definitely add that as well.

 Kent

-----Original Message-----
From: Alex Deucher [mailto:alexdeucher@gmail.com]
Sent: Thursday, August 24, 2017 8:56 AM
To: Russell, Kent
Cc: Christian König; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote:
> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!
>

While you are at it, can you expose the vbios binary itself via debugfs?  That's been on by todo list for a while.

Alex

>  Kent
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Thursday, August 24, 2017 2:22 AM
> To: Russell, Kent; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> Am 23.08.2017 um 20:12 schrieb Kent Russell:
>> This won't change after initialization, so we can add the information
>> when we parse the atombios information. This ensures that we can find
>> out the VBIOS, even when the dmesg buffer fills up, and makes it
>> easier to associate which VBIOS is for which GPU on mGPU
>> configurations. Set the size to 20 characters in case of some weird
>> VBIOS suffix that exceeds the expected 17 character format (3-8-3\0)
>
> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.
>
> Christian.
>
>>
>> Signed-off-by: Kent Russell <kent.russell@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>>   drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>>   3 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a1f9424..f40be71 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>>       return r;
>>   }
>>
>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
>> +                                              struct device_attribute *attr,
>> +                                              char *buf) {
>> +     struct drm_device *ddev = dev_get_drvdata(dev);
>> +     struct amdgpu_device *adev = ddev->dev_private;
>> +     struct atom_context *ctx = adev->mode_info.atom_context;
>> +
>> +     return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); }
>> +
>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>> +                NULL);
>> +
>>   /**
>>    * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>>    *
>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>>       adev->mode_info.atom_context = NULL;
>>       kfree(adev->mode_info.atom_card_info);
>>       adev->mode_info.atom_card_info = NULL;
>> +     device_remove_file(adev->dev, &dev_attr_vbios_version);
>>   }
>>
>>   /**
>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>   {
>>       struct card_info *atom_card_info =
>>           kzalloc(sizeof(struct card_info), GFP_KERNEL);
>> +     int ret;
>>
>>       if (!atom_card_info)
>>               return -ENOMEM;
>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>               amdgpu_atombios_scratch_regs_init(adev);
>>               amdgpu_atombios_allocate_fb_scratch(adev);
>>       }
>> +
>> +     ret = device_create_file(adev->dev, &dev_attr_vbios_version);
>> +     if (ret) {
>> +             DRM_ERROR("Failed to create device file for VBIOS version\n");
>> +             return ret;
>> +     }
>> +
>>       return 0;
>>   }
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>> index d69aa2e..69500a8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>>               idx = 0x80;
>>
>>       str = CSTR(idx);
>> -     if (*str != '\0')
>> +     if (*str != '\0') {
>>               pr_info("ATOM BIOS: %s\n", str);
>> +             strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
>> +     }
>> +
>>
>>       return ctx;
>>   }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
>> b/drivers/gpu/drm/amd/amdgpu/atom.h
>> index ddd8045..a391709 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
>> @@ -140,6 +140,7 @@ struct atom_context {
>>       int io_mode;
>>       uint32_t *scratch;
>>       int scratch_size_bytes;
>> +     char vbios_version[20];
>>   };
>>
>>   extern int amdgpu_atom_debug;
>
>
> _______________________________________________
> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Alex Deucher]

On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix
<Felix.Kuehling@amd.com> wrote:
> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs?
>
> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi.
>

Ah, ok, sysfs is fine in that case.  I thought this was just general
debugging stuff.

Alex

> Regards,
>   Felix
> ________________________________________
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Russell, Kent <Kent.Russell@amd.com>
> Sent: Thursday, August 24, 2017 9:06:22 AM
> To: Alex Deucher
> Cc: Christian König; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> I can definitely add that as well.
>
>  Kent
>
> -----Original Message-----
> From: Alex Deucher [mailto:alexdeucher@gmail.com]
> Sent: Thursday, August 24, 2017 8:56 AM
> To: Russell, Kent
> Cc: Christian König; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote:
>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!
>>
>
> While you are at it, can you expose the vbios binary itself via debugfs?  That's been on by todo list for a while.
>
> Alex
>
>>  Kent
>>
>> -----Original Message-----
>> From: Christian König [mailto:deathsimple@vodafone.de]
>> Sent: Thursday, August 24, 2017 2:22 AM
>> To: Russell, Kent; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> Am 23.08.2017 um 20:12 schrieb Kent Russell:
>>> This won't change after initialization, so we can add the information
>>> when we parse the atombios information. This ensures that we can find
>>> out the VBIOS, even when the dmesg buffer fills up, and makes it
>>> easier to associate which VBIOS is for which GPU on mGPU
>>> configurations. Set the size to 20 characters in case of some weird
>>> VBIOS suffix that exceeds the expected 17 character format (3-8-3\0)
>>
>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.
>>
>> Christian.
>>
>>>
>>> Signed-off-by: Kent Russell <kent.russell@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>>>   drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>>>   drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>>>   3 files changed, 28 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index a1f9424..f40be71 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>>>       return r;
>>>   }
>>>
>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
>>> +                                              struct device_attribute *attr,
>>> +                                              char *buf) {
>>> +     struct drm_device *ddev = dev_get_drvdata(dev);
>>> +     struct amdgpu_device *adev = ddev->dev_private;
>>> +     struct atom_context *ctx = adev->mode_info.atom_context;
>>> +
>>> +     return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); }
>>> +
>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>>> +                NULL);
>>> +
>>>   /**
>>>    * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>>>    *
>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>>>       adev->mode_info.atom_context = NULL;
>>>       kfree(adev->mode_info.atom_card_info);
>>>       adev->mode_info.atom_card_info = NULL;
>>> +     device_remove_file(adev->dev, &dev_attr_vbios_version);
>>>   }
>>>
>>>   /**
>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>   {
>>>       struct card_info *atom_card_info =
>>>           kzalloc(sizeof(struct card_info), GFP_KERNEL);
>>> +     int ret;
>>>
>>>       if (!atom_card_info)
>>>               return -ENOMEM;
>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>               amdgpu_atombios_scratch_regs_init(adev);
>>>               amdgpu_atombios_allocate_fb_scratch(adev);
>>>       }
>>> +
>>> +     ret = device_create_file(adev->dev, &dev_attr_vbios_version);
>>> +     if (ret) {
>>> +             DRM_ERROR("Failed to create device file for VBIOS version\n");
>>> +             return ret;
>>> +     }
>>> +
>>>       return 0;
>>>   }
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>>> index d69aa2e..69500a8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>>>               idx = 0x80;
>>>
>>>       str = CSTR(idx);
>>> -     if (*str != '\0')
>>> +     if (*str != '\0') {
>>>               pr_info("ATOM BIOS: %s\n", str);
>>> +             strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
>>> +     }
>>> +
>>>
>>>       return ctx;
>>>   }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
>>> b/drivers/gpu/drm/amd/amdgpu/atom.h
>>> index ddd8045..a391709 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
>>> @@ -140,6 +140,7 @@ struct atom_context {
>>>       int io_mode;
>>>       uint32_t *scratch;
>>>       int scratch_size_bytes;
>>> +     char vbios_version[20];
>>>   };
>>>
>>>   extern int amdgpu_atom_debug;
>>
>>
>> _______________________________________________
>> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Russell, Kent]

We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that.

 Kent

-----Original Message-----
From: Alex Deucher [mailto:alexdeucher@gmail.com] 
Sent: Thursday, August 24, 2017 11:39 AM
To: Kuehling, Felix
Cc: Russell, Kent; Christian König; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote:
> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs?
>
> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi.
>

Ah, ok, sysfs is fine in that case.  I thought this was just general debugging stuff.

Alex

> Regards,
>   Felix
> ________________________________________
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of 
> Russell, Kent <Kent.Russell@amd.com>
> Sent: Thursday, August 24, 2017 9:06:22 AM
> To: Alex Deucher
> Cc: Christian König; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> I can definitely add that as well.
>
>  Kent
>
> -----Original Message-----
> From: Alex Deucher [mailto:alexdeucher@gmail.com]
> Sent: Thursday, August 24, 2017 8:56 AM
> To: Russell, Kent
> Cc: Christian König; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote:
>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!
>>
>
> While you are at it, can you expose the vbios binary itself via debugfs?  That's been on by todo list for a while.
>
> Alex
>
>>  Kent
>>
>> -----Original Message-----
>> From: Christian König [mailto:deathsimple@vodafone.de]
>> Sent: Thursday, August 24, 2017 2:22 AM
>> To: Russell, Kent; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> Am 23.08.2017 um 20:12 schrieb Kent Russell:
>>> This won't change after initialization, so we can add the 
>>> information when we parse the atombios information. This ensures 
>>> that we can find out the VBIOS, even when the dmesg buffer fills up, 
>>> and makes it easier to associate which VBIOS is for which GPU on 
>>> mGPU configurations. Set the size to 20 characters in case of some 
>>> weird VBIOS suffix that exceeds the expected 17 character format 
>>> (3-8-3\0)
>>
>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.
>>
>> Christian.
>>
>>>
>>> Signed-off-by: Kent Russell <kent.russell@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>>>   drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>>>   drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>>>   3 files changed, 28 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index a1f9424..f40be71 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>>>       return r;
>>>   }
>>>
>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
>>> +                                              struct device_attribute *attr,
>>> +                                              char *buf) {
>>> +     struct drm_device *ddev = dev_get_drvdata(dev);
>>> +     struct amdgpu_device *adev = ddev->dev_private;
>>> +     struct atom_context *ctx = adev->mode_info.atom_context;
>>> +
>>> +     return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); }
>>> +
>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>>> +                NULL);
>>> +
>>>   /**
>>>    * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>>>    *
>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>>>       adev->mode_info.atom_context = NULL;
>>>       kfree(adev->mode_info.atom_card_info);
>>>       adev->mode_info.atom_card_info = NULL;
>>> +     device_remove_file(adev->dev, &dev_attr_vbios_version);
>>>   }
>>>
>>>   /**
>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>   {
>>>       struct card_info *atom_card_info =
>>>           kzalloc(sizeof(struct card_info), GFP_KERNEL);
>>> +     int ret;
>>>
>>>       if (!atom_card_info)
>>>               return -ENOMEM;
>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>               amdgpu_atombios_scratch_regs_init(adev);
>>>               amdgpu_atombios_allocate_fb_scratch(adev);
>>>       }
>>> +
>>> +     ret = device_create_file(adev->dev, &dev_attr_vbios_version);
>>> +     if (ret) {
>>> +             DRM_ERROR("Failed to create device file for VBIOS version\n");
>>> +             return ret;
>>> +     }
>>> +
>>>       return 0;
>>>   }
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>>> index d69aa2e..69500a8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>>>               idx = 0x80;
>>>
>>>       str = CSTR(idx);
>>> -     if (*str != '\0')
>>> +     if (*str != '\0') {
>>>               pr_info("ATOM BIOS: %s\n", str);
>>> +             strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
>>> +     }
>>> +
>>>
>>>       return ctx;
>>>   }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
>>> b/drivers/gpu/drm/amd/amdgpu/atom.h
>>> index ddd8045..a391709 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
>>> @@ -140,6 +140,7 @@ struct atom_context {
>>>       int io_mode;
>>>       uint32_t *scratch;
>>>       int scratch_size_bytes;
>>> +     char vbios_version[20];
>>>   };
>>>
>>>   extern int amdgpu_atom_debug;
>>
>>
>> _______________________________________________
>> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Christian König]

Actually the main difference is not root vs. user, but rather unstable 
vs stable interface.

If you want a stable interface for an userspace tool you should use 
sysfs, if you don't care about that you should use debugfs.

Christian.

Am 24.08.2017 um 18:37 schrieb Russell, Kent:
> We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that.
>
>   Kent
>
> -----Original Message-----
> From: Alex Deucher [mailto:alexdeucher@gmail.com]
> Sent: Thursday, August 24, 2017 11:39 AM
> To: Kuehling, Felix
> Cc: Russell, Kent; Christian König; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote:
>> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs?
>>
>> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi.
>>
> Ah, ok, sysfs is fine in that case.  I thought this was just general debugging stuff.
>
> Alex
>
>> Regards,
>>    Felix
>> ________________________________________
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of
>> Russell, Kent <Kent.Russell@amd.com>
>> Sent: Thursday, August 24, 2017 9:06:22 AM
>> To: Alex Deucher
>> Cc: Christian König; amd-gfx@lists.freedesktop.org
>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> I can definitely add that as well.
>>
>>   Kent
>>
>> -----Original Message-----
>> From: Alex Deucher [mailto:alexdeucher@gmail.com]
>> Sent: Thursday, August 24, 2017 8:56 AM
>> To: Russell, Kent
>> Cc: Christian König; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote:
>>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!
>>>
>> While you are at it, can you expose the vbios binary itself via debugfs?  That's been on by todo list for a while.
>>
>> Alex
>>
>>>   Kent
>>>
>>> -----Original Message-----
>>> From: Christian König [mailto:deathsimple@vodafone.de]
>>> Sent: Thursday, August 24, 2017 2:22 AM
>>> To: Russell, Kent; amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>
>>> Am 23.08.2017 um 20:12 schrieb Kent Russell:
>>>> This won't change after initialization, so we can add the
>>>> information when we parse the atombios information. This ensures
>>>> that we can find out the VBIOS, even when the dmesg buffer fills up,
>>>> and makes it easier to associate which VBIOS is for which GPU on
>>>> mGPU configurations. Set the size to 20 characters in case of some
>>>> weird VBIOS suffix that exceeds the expected 17 character format
>>>> (3-8-3\0)
>>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.
>>>
>>> Christian.
>>>
>>>> Signed-off-by: Kent Russell <kent.russell@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>>>>    drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>>>>    drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>>>>    3 files changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> index a1f9424..f40be71 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>>>>        return r;
>>>>    }
>>>>
>>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
>>>> +                                              struct device_attribute *attr,
>>>> +                                              char *buf) {
>>>> +     struct drm_device *ddev = dev_get_drvdata(dev);
>>>> +     struct amdgpu_device *adev = ddev->dev_private;
>>>> +     struct atom_context *ctx = adev->mode_info.atom_context;
>>>> +
>>>> +     return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); }
>>>> +
>>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>>>> +                NULL);
>>>> +
>>>>    /**
>>>>     * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>>>>     *
>>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>>>>        adev->mode_info.atom_context = NULL;
>>>>        kfree(adev->mode_info.atom_card_info);
>>>>        adev->mode_info.atom_card_info = NULL;
>>>> +     device_remove_file(adev->dev, &dev_attr_vbios_version);
>>>>    }
>>>>
>>>>    /**
>>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>>    {
>>>>        struct card_info *atom_card_info =
>>>>            kzalloc(sizeof(struct card_info), GFP_KERNEL);
>>>> +     int ret;
>>>>
>>>>        if (!atom_card_info)
>>>>                return -ENOMEM;
>>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>>                amdgpu_atombios_scratch_regs_init(adev);
>>>>                amdgpu_atombios_allocate_fb_scratch(adev);
>>>>        }
>>>> +
>>>> +     ret = device_create_file(adev->dev, &dev_attr_vbios_version);
>>>> +     if (ret) {
>>>> +             DRM_ERROR("Failed to create device file for VBIOS version\n");
>>>> +             return ret;
>>>> +     }
>>>> +
>>>>        return 0;
>>>>    }
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>>>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>>>> index d69aa2e..69500a8 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>>>>                idx = 0x80;
>>>>
>>>>        str = CSTR(idx);
>>>> -     if (*str != '\0')
>>>> +     if (*str != '\0') {
>>>>                pr_info("ATOM BIOS: %s\n", str);
>>>> +             strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
>>>> +     }
>>>> +
>>>>
>>>>        return ctx;
>>>>    }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
>>>> b/drivers/gpu/drm/amd/amdgpu/atom.h
>>>> index ddd8045..a391709 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
>>>> @@ -140,6 +140,7 @@ struct atom_context {
>>>>        int io_mode;
>>>>        uint32_t *scratch;
>>>>        int scratch_size_bytes;
>>>> +     char vbios_version[20];
>>>>    };
>>>>
>>>>    extern int amdgpu_atom_debug;
>>>
>>> _______________________________________________
>>> 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
> _______________________________________________
> 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] drm/amdgpu: Add sysfs file for VBIOS

[Russell, Kent]

[-- Attachment #1.1: Type: text/plain, Size: 8751 bytes --]

The plan is for the vbios version to be available through the ROCM-SMI utility. We have the GPU power usage listed in the debugfs currently. If they are both to be used for a userspace utility, should we be moving both of those to sysfs instead?

Kent

KENT RUSSELL
Software Engineer | Vertical Workstation/Compute
1 Commerce Valley Drive East
Markham, ON L3T 7X6
Canada
O +(1) 289-695-2122   | Ext x72122


________________________________
From: Christian König <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
Sent: Thursday, August 24, 2017 2:10:35 PM
To: Russell, Kent; Alex Deucher; Kuehling, Felix
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

Actually the main difference is not root vs. user, but rather unstable
vs stable interface.

If you want a stable interface for an userspace tool you should use
sysfs, if you don't care about that you should use debugfs.

Christian.

Am 24.08.2017 um 18:37 schrieb Russell, Kent:
> We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that.
>
>   Kent
>
> -----Original Message-----
> From: Alex Deucher [mailto:alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> Sent: Thursday, August 24, 2017 11:39 AM
> To: Kuehling, Felix
> Cc: Russell, Kent; Christian König; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org> wrote:
>> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs?
>>
>> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi.
>>
> Ah, ok, sysfs is fine in that case.  I thought this was just general debugging stuff.
>
> Alex
>
>> Regards,
>>    Felix
>> ________________________________________
>> From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of
>> Russell, Kent <Kent.Russell-5C7GfCeVMHo@public.gmane.org>
>> Sent: Thursday, August 24, 2017 9:06:22 AM
>> To: Alex Deucher
>> Cc: Christian König; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> I can definitely add that as well.
>>
>>   Kent
>>
>> -----Original Message-----
>> From: Alex Deucher [mailto:alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
>> Sent: Thursday, August 24, 2017 8:56 AM
>> To: Russell, Kent
>> Cc: Christian König; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell-5C7GfCeVMHo@public.gmane.org> wrote:
>>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!
>>>
>> While you are at it, can you expose the vbios binary itself via debugfs?  That's been on by todo list for a while.
>>
>> Alex
>>
>>>   Kent
>>>
>>> -----Original Message-----
>>> From: Christian König [mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org]
>>> Sent: Thursday, August 24, 2017 2:22 AM
>>> To: Russell, Kent; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>
>>> Am 23.08.2017 um 20:12 schrieb Kent Russell:
>>>> This won't change after initialization, so we can add the
>>>> information when we parse the atombios information. This ensures
>>>> that we can find out the VBIOS, even when the dmesg buffer fills up,
>>>> and makes it easier to associate which VBIOS is for which GPU on
>>>> mGPU configurations. Set the size to 20 characters in case of some
>>>> weird VBIOS suffix that exceeds the expected 17 character format
>>>> (3-8-3\0)
>>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.
>>>
>>> Christian.
>>>
>>>> Signed-off-by: Kent Russell <kent.russell-5C7GfCeVMHo@public.gmane.org>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>>>>    drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>>>>    drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>>>>    3 files changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> index a1f9424..f40be71 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>>>>        return r;
>>>>    }
>>>>
>>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
>>>> +                                              struct device_attribute *attr,
>>>> +                                              char *buf) {
>>>> +     struct drm_device *ddev = dev_get_drvdata(dev);
>>>> +     struct amdgpu_device *adev = ddev->dev_private;
>>>> +     struct atom_context *ctx = adev->mode_info.atom_context;
>>>> +
>>>> +     return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); }
>>>> +
>>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>>>> +                NULL);
>>>> +
>>>>    /**
>>>>     * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>>>>     *
>>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>>>>        adev->mode_info.atom_context = NULL;
>>>>        kfree(adev->mode_info.atom_card_info);
>>>>        adev->mode_info.atom_card_info = NULL;
>>>> +     device_remove_file(adev->dev, &dev_attr_vbios_version);
>>>>    }
>>>>
>>>>    /**
>>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>>    {
>>>>        struct card_info *atom_card_info =
>>>>            kzalloc(sizeof(struct card_info), GFP_KERNEL);
>>>> +     int ret;
>>>>
>>>>        if (!atom_card_info)
>>>>                return -ENOMEM;
>>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>>                amdgpu_atombios_scratch_regs_init(adev);
>>>>                amdgpu_atombios_allocate_fb_scratch(adev);
>>>>        }
>>>> +
>>>> +     ret = device_create_file(adev->dev, &dev_attr_vbios_version);
>>>> +     if (ret) {
>>>> +             DRM_ERROR("Failed to create device file for VBIOS version\n");
>>>> +             return ret;
>>>> +     }
>>>> +
>>>>        return 0;
>>>>    }
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>>>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>>>> index d69aa2e..69500a8 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>>>>                idx = 0x80;
>>>>
>>>>        str = CSTR(idx);
>>>> -     if (*str != '\0')
>>>> +     if (*str != '\0') {
>>>>                pr_info("ATOM BIOS: %s\n", str);
>>>> +             strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
>>>> +     }
>>>> +
>>>>
>>>>        return ctx;
>>>>    }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
>>>> b/drivers/gpu/drm/amd/amdgpu/atom.h
>>>> index ddd8045..a391709 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
>>>> @@ -140,6 +140,7 @@ struct atom_context {
>>>>        int io_mode;
>>>>        uint32_t *scratch;
>>>>        int scratch_size_bytes;
>>>> +     char vbios_version[20];
>>>>    };
>>>>
>>>>    extern int amdgpu_atom_debug;
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[-- Attachment #1.2: Type: text/html, Size: 14812 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Tom St Denis]

Hi Kent,

Loosely following this thread.  Was there a decision on whether to leave 
this in debugfs or sysfs?

I'd like to throw the version string in umr's config output (umr -c) :-)

Cheers,
Tom

On 24/08/17 05:30 PM, Russell, Kent wrote:
> The plan is for the vbios version to be available through the ROCM-SMI 
> utility. We have the GPU power usage listed in the debugfs currently. If 
> they are both to be used for a userspace utility, should we be moving 
> both of those to sysfs instead?
> 
> Kent
> 
> KENT RUSSELL
> Software Engineer | Vertical Workstation/Compute
> 1 Commerce Valley Drive East
> Markham, ON L3T 7X6
> Canada
> O +(1) 289-695-2122   | Ext x72122
> 
> 
> ------------------------------------------------------------------------
> *From:* Christian König <deathsimple@vodafone.de>
> *Sent:* Thursday, August 24, 2017 2:10:35 PM
> *To:* Russell, Kent; Alex Deucher; Kuehling, Felix
> *Cc:* amd-gfx@lists.freedesktop.org
> *Subject:* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> Actually the main difference is not root vs. user, but rather unstable
> vs stable interface.
> 
> If you want a stable interface for an userspace tool you should use
> sysfs, if you don't care about that you should use debugfs.
> 
> Christian.
> 
> Am 24.08.2017 um 18:37 schrieb Russell, Kent:
>> We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that.
>>
>>   Kent
>>
>> -----Original Message-----
>> From: Alex Deucher [mailto:alexdeucher@gmail.com]
>> Sent: Thursday, August 24, 2017 11:39 AM
>> To: Kuehling, Felix
>> Cc: Russell, Kent; Christian König; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote:
>>> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs?
>>>
>>> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi.
>>>
>> Ah, ok, sysfs is fine in that case.  I thought this was just general debugging stuff.
>>
>> Alex
>>
>>> Regards,
>>>    Felix
>>> ________________________________________
>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of
>>> Russell, Kent <Kent.Russell@amd.com>
>>> Sent: Thursday, August 24, 2017 9:06:22 AM
>>> To: Alex Deucher
>>> Cc: Christian König; amd-gfx@lists.freedesktop.org
>>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>
>>> I can definitely add that as well.
>>>
>>>   Kent
>>>
>>> -----Original Message-----
>>> From: Alex Deucher [mailto:alexdeucher@gmail.com]
>>> Sent: Thursday, August 24, 2017 8:56 AM
>>> To: Russell, Kent
>>> Cc: Christian König; amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>
>>> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote:
>>>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!
>>>>
>>> While you are at it, can you expose the vbios binary itself via debugfs?  That's been on by todo list for a while.
>>>
>>> Alex
>>>
>>>>   Kent
>>>>
>>>> -----Original Message-----
>>>> From: Christian König [mailto:deathsimple@vodafone.de]
>>>> Sent: Thursday, August 24, 2017 2:22 AM
>>>> To: Russell, Kent; amd-gfx@lists.freedesktop.org
>>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>>
>>>> Am 23.08.2017 um 20:12 schrieb Kent Russell:
>>>>> This won't change after initialization, so we can add the
>>>>> information when we parse the atombios information. This ensures
>>>>> that we can find out the VBIOS, even when the dmesg buffer fills up,
>>>>> and makes it easier to associate which VBIOS is for which GPU on
>>>>> mGPU configurations. Set the size to 20 characters in case of some
>>>>> weird VBIOS suffix that exceeds the expected 17 character format
>>>>> (3-8-3\0)
>>>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.
>>>>
>>>> Christian.
>>>>
>>>>> Signed-off-by: Kent Russell <kent.russell@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>>>>>    drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>>>>>    drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>>>>>    3 files changed, 28 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> index a1f9424..f40be71 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>>>>>        return r;
>>>>>    }
>>>>>
>>>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
>>>>> +                                              struct device_attribute *attr,
>>>>> +                                              char *buf) {
>>>>> +     struct drm_device *ddev = dev_get_drvdata(dev);
>>>>> +     struct amdgpu_device *adev = ddev->dev_private;
>>>>> +     struct atom_context *ctx = adev->mode_info.atom_context;
>>>>> +
>>>>> +     return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); }
>>>>> +
>>>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>>>>> +                NULL);
>>>>> +
>>>>>    /**
>>>>>     * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>>>>>     *
>>>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>>>>>        adev->mode_info.atom_context = NULL;
>>>>>        kfree(adev->mode_info.atom_card_info);
>>>>>        adev->mode_info.atom_card_info = NULL;
>>>>> +     device_remove_file(adev->dev, &dev_attr_vbios_version);
>>>>>    }
>>>>>
>>>>>    /**
>>>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>>>    {
>>>>>        struct card_info *atom_card_info =
>>>>>            kzalloc(sizeof(struct card_info), GFP_KERNEL);
>>>>> +     int ret;
>>>>>
>>>>>        if (!atom_card_info)
>>>>>                return -ENOMEM;
>>>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>>>                amdgpu_atombios_scratch_regs_init(adev);
>>>>>                amdgpu_atombios_allocate_fb_scratch(adev);
>>>>>        }
>>>>> +
>>>>> +     ret = device_create_file(adev->dev, &dev_attr_vbios_version);
>>>>> +     if (ret) {
>>>>> +             DRM_ERROR("Failed to create device file for VBIOS version\n");
>>>>> +             return ret;
>>>>> +     }
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>> index d69aa2e..69500a8 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>>>>>                idx = 0x80;
>>>>>
>>>>>        str = CSTR(idx);
>>>>> -     if (*str != '\0')
>>>>> +     if (*str != '\0') {
>>>>>                pr_info("ATOM BIOS: %s\n", str);
>>>>> +             strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
>>>>> +     }
>>>>> +
>>>>>
>>>>>        return ctx;
>>>>>    }
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>> b/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>> index ddd8045..a391709 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>> @@ -140,6 +140,7 @@ struct atom_context {
>>>>>        int io_mode;
>>>>>        uint32_t *scratch;
>>>>>        int scratch_size_bytes;
>>>>> +     char vbios_version[20];
>>>>>    };
>>>>>
>>>>>    extern int amdgpu_atom_debug;
>>>>
>>>> _______________________________________________
>>>> 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
>> _______________________________________________
>> 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
> 

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

RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Russell, Kent]

The decision to move the VBIOS version to sysfs has been decided. I didn't get any confirmation regarding moving the GPU power usage, or the actual VBIOS dump, as the dump seems to make more sense in debugfs. I'd like to move power usage to sysfs as the SMI tool needs root privileges just to read the value, which makes people cranky. I just want to make sure that it's alright to do so from the powers-that-be.

 Kent

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Tom St Denis
Sent: Friday, August 25, 2017 8:37 AM
To: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

Hi Kent,

Loosely following this thread.  Was there a decision on whether to leave this in debugfs or sysfs?

I'd like to throw the version string in umr's config output (umr -c) :-)

Cheers,
Tom

On 24/08/17 05:30 PM, Russell, Kent wrote:
> The plan is for the vbios version to be available through the ROCM-SMI 
> utility. We have the GPU power usage listed in the debugfs currently. 
> If they are both to be used for a userspace utility, should we be 
> moving both of those to sysfs instead?
> 
> Kent
> 
> KENT RUSSELL
> Software Engineer | Vertical Workstation/Compute
> 1 Commerce Valley Drive East
> Markham, ON L3T 7X6
> Canada
> O +(1) 289-695-2122   | Ext x72122
> 
> 
> ----------------------------------------------------------------------
> --
> *From:* Christian König <deathsimple@vodafone.de>
> *Sent:* Thursday, August 24, 2017 2:10:35 PM
> *To:* Russell, Kent; Alex Deucher; Kuehling, Felix
> *Cc:* amd-gfx@lists.freedesktop.org
> *Subject:* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS Actually 
> the main difference is not root vs. user, but rather unstable vs 
> stable interface.
> 
> If you want a stable interface for an userspace tool you should use 
> sysfs, if you don't care about that you should use debugfs.
> 
> Christian.
> 
> Am 24.08.2017 um 18:37 schrieb Russell, Kent:
>> We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that.
>>
>>   Kent
>>
>> -----Original Message-----
>> From: Alex Deucher [mailto:alexdeucher@gmail.com]
>> Sent: Thursday, August 24, 2017 11:39 AM
>> To: Kuehling, Felix
>> Cc: Russell, Kent; Christian König; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote:
>>> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs?
>>>
>>> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi.
>>>
>> Ah, ok, sysfs is fine in that case.  I thought this was just general debugging stuff.
>>
>> Alex
>>
>>> Regards,
>>>    Felix
>>> ________________________________________
>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of 
>>> Russell, Kent <Kent.Russell@amd.com>
>>> Sent: Thursday, August 24, 2017 9:06:22 AM
>>> To: Alex Deucher
>>> Cc: Christian König; amd-gfx@lists.freedesktop.org
>>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>
>>> I can definitely add that as well.
>>>
>>>   Kent
>>>
>>> -----Original Message-----
>>> From: Alex Deucher [mailto:alexdeucher@gmail.com]
>>> Sent: Thursday, August 24, 2017 8:56 AM
>>> To: Russell, Kent
>>> Cc: Christian König; amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>
>>> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote:
>>>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!
>>>>
>>> While you are at it, can you expose the vbios binary itself via debugfs?  That's been on by todo list for a while.
>>>
>>> Alex
>>>
>>>>   Kent
>>>>
>>>> -----Original Message-----
>>>> From: Christian König [mailto:deathsimple@vodafone.de]
>>>> Sent: Thursday, August 24, 2017 2:22 AM
>>>> To: Russell, Kent; amd-gfx@lists.freedesktop.org
>>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>>
>>>> Am 23.08.2017 um 20:12 schrieb Kent Russell:
>>>>> This won't change after initialization, so we can add the 
>>>>> information when we parse the atombios information. This ensures 
>>>>> that we can find out the VBIOS, even when the dmesg buffer fills 
>>>>> up, and makes it easier to associate which VBIOS is for which GPU 
>>>>> on mGPU configurations. Set the size to 20 characters in case of 
>>>>> some weird VBIOS suffix that exceeds the expected 17 character 
>>>>> format
>>>>> (3-8-3\0)
>>>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.
>>>>
>>>> Christian.
>>>>
>>>>> Signed-off-by: Kent Russell <kent.russell@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>>>>>    drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>>>>>    drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>>>>>    3 files changed, 28 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> index a1f9424..f40be71 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>>>>>        return r;
>>>>>    }
>>>>>
>>>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
>>>>> +                                              struct device_attribute *attr,
>>>>> +                                              char *buf) {
>>>>> +     struct drm_device *ddev = dev_get_drvdata(dev);
>>>>> +     struct amdgpu_device *adev = ddev->dev_private;
>>>>> +     struct atom_context *ctx = adev->mode_info.atom_context;
>>>>> +
>>>>> +     return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); 
>>>>> + }
>>>>> +
>>>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>>>>> +                NULL);
>>>>> +
>>>>>    /**
>>>>>     * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>>>>>     *
>>>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>>>>>        adev->mode_info.atom_context = NULL;
>>>>>        kfree(adev->mode_info.atom_card_info);
>>>>>        adev->mode_info.atom_card_info = NULL;
>>>>> +     device_remove_file(adev->dev, &dev_attr_vbios_version);
>>>>>    }
>>>>>
>>>>>    /**
>>>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>>>    {
>>>>>        struct card_info *atom_card_info =
>>>>>            kzalloc(sizeof(struct card_info), GFP_KERNEL);
>>>>> +     int ret;
>>>>>
>>>>>        if (!atom_card_info)
>>>>>                return -ENOMEM;
>>>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>>>                amdgpu_atombios_scratch_regs_init(adev);
>>>>>                amdgpu_atombios_allocate_fb_scratch(adev);
>>>>>        }
>>>>> +
>>>>> +     ret = device_create_file(adev->dev, &dev_attr_vbios_version);
>>>>> +     if (ret) {
>>>>> +             DRM_ERROR("Failed to create device file for VBIOS version\n");
>>>>> +             return ret;
>>>>> +     }
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>> index d69aa2e..69500a8 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>>>>>                idx = 0x80;
>>>>>
>>>>>        str = CSTR(idx);
>>>>> -     if (*str != '\0')
>>>>> +     if (*str != '\0') {
>>>>>                pr_info("ATOM BIOS: %s\n", str);
>>>>> +             strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
>>>>> +     }
>>>>> +
>>>>>
>>>>>        return ctx;
>>>>>    }
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>> b/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>> index ddd8045..a391709 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>> @@ -140,6 +140,7 @@ struct atom_context {
>>>>>        int io_mode;
>>>>>        uint32_t *scratch;
>>>>>        int scratch_size_bytes;
>>>>> +     char vbios_version[20];
>>>>>    };
>>>>>
>>>>>    extern int amdgpu_atom_debug;
>>>>
>>>> _______________________________________________
>>>> 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
>> _______________________________________________
>> 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
> 

_______________________________________________
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] drm/amdgpu: Add sysfs file for VBIOS

[Christian König]

Hi Kent,

agree on the VBIOS dump file, that clearly belongs to debugsf.

The power usage stuff I can't say much about cause I'm not deeply into 
this, but keep in mind the restriction for sysfs:
1. It's a stable interface. So it must be very well designed.
2. Only one value per file. I think the power stuff doesn't fulfill that 
requirement at the moment.

Regards,
Christian.

Am 25.08.2017 um 14:40 schrieb Russell, Kent:
> The decision to move the VBIOS version to sysfs has been decided. I didn't get any confirmation regarding moving the GPU power usage, or the actual VBIOS dump, as the dump seems to make more sense in debugfs. I'd like to move power usage to sysfs as the SMI tool needs root privileges just to read the value, which makes people cranky. I just want to make sure that it's alright to do so from the powers-that-be.
>
>   Kent
>
> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Tom St Denis
> Sent: Friday, August 25, 2017 8:37 AM
> To: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> Hi Kent,
>
> Loosely following this thread.  Was there a decision on whether to leave this in debugfs or sysfs?
>
> I'd like to throw the version string in umr's config output (umr -c) :-)
>
> Cheers,
> Tom
>
> On 24/08/17 05:30 PM, Russell, Kent wrote:
>> The plan is for the vbios version to be available through the ROCM-SMI
>> utility. We have the GPU power usage listed in the debugfs currently.
>> If they are both to be used for a userspace utility, should we be
>> moving both of those to sysfs instead?
>>
>> Kent
>>
>> KENT RUSSELL
>> Software Engineer | Vertical Workstation/Compute
>> 1 Commerce Valley Drive East
>> Markham, ON L3T 7X6
>> Canada
>> O +(1) 289-695-2122   | Ext x72122
>>
>>
>> ----------------------------------------------------------------------
>> --
>> *From:* Christian König <deathsimple@vodafone.de>
>> *Sent:* Thursday, August 24, 2017 2:10:35 PM
>> *To:* Russell, Kent; Alex Deucher; Kuehling, Felix
>> *Cc:* amd-gfx@lists.freedesktop.org
>> *Subject:* Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS Actually
>> the main difference is not root vs. user, but rather unstable vs
>> stable interface.
>>
>> If you want a stable interface for an userspace tool you should use
>> sysfs, if you don't care about that you should use debugfs.
>>
>> Christian.
>>
>> Am 24.08.2017 um 18:37 schrieb Russell, Kent:
>>> We already access the GPU Power usage via debugfs, which is why I didn't think it was a huge deal to switch it over, since we already need root access for the SMI due to that.
>>>
>>>    Kent
>>>
>>> -----Original Message-----
>>> From: Alex Deucher [mailto:alexdeucher@gmail.com]
>>> Sent: Thursday, August 24, 2017 11:39 AM
>>> To: Kuehling, Felix
>>> Cc: Russell, Kent; Christian König; amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>
>>> On Thu, Aug 24, 2017 at 11:35 AM, Kuehling, Felix <Felix.Kuehling@amd.com> wrote:
>>>> Debugfs is only accessible by Root. The BIOS version is already reported in the kernel log, which is visible to everyone. So why hide this away in debugfs?
>>>>
>>>> I think Kent's intention is to add VBIOS version reporting to the rocm-smi tool. I'd prefer using a stable interface like sysfs, and one that doesn't require root access in a tool like rocm-smi.
>>>>
>>> Ah, ok, sysfs is fine in that case.  I thought this was just general debugging stuff.
>>>
>>> Alex
>>>
>>>> Regards,
>>>>     Felix
>>>> ________________________________________
>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of
>>>> Russell, Kent <Kent.Russell@amd.com>
>>>> Sent: Thursday, August 24, 2017 9:06:22 AM
>>>> To: Alex Deucher
>>>> Cc: Christian König; amd-gfx@lists.freedesktop.org
>>>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>>
>>>> I can definitely add that as well.
>>>>
>>>>    Kent
>>>>
>>>> -----Original Message-----
>>>> From: Alex Deucher [mailto:alexdeucher@gmail.com]
>>>> Sent: Thursday, August 24, 2017 8:56 AM
>>>> To: Russell, Kent
>>>> Cc: Christian König; amd-gfx@lists.freedesktop.org
>>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>>
>>>> On Thu, Aug 24, 2017 at 5:58 AM, Russell, Kent <Kent.Russell@amd.com> wrote:
>>>>> No real reason for sysfs instead of debugfs, I just picked the one that was more familiar with. I can definitely move it to debugfs instead. I will also clean up the commit message per Michel's comments. Thank you!
>>>>>
>>>> While you are at it, can you expose the vbios binary itself via debugfs?  That's been on by todo list for a while.
>>>>
>>>> Alex
>>>>
>>>>>    Kent
>>>>>
>>>>> -----Original Message-----
>>>>> From: Christian König [mailto:deathsimple@vodafone.de]
>>>>> Sent: Thursday, August 24, 2017 2:22 AM
>>>>> To: Russell, Kent; amd-gfx@lists.freedesktop.org
>>>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>>>
>>>>> Am 23.08.2017 um 20:12 schrieb Kent Russell:
>>>>>> This won't change after initialization, so we can add the
>>>>>> information when we parse the atombios information. This ensures
>>>>>> that we can find out the VBIOS, even when the dmesg buffer fills
>>>>>> up, and makes it easier to associate which VBIOS is for which GPU
>>>>>> on mGPU configurations. Set the size to 20 characters in case of
>>>>>> some weird VBIOS suffix that exceeds the expected 17 character
>>>>>> format
>>>>>> (3-8-3\0)
>>>>> Is there any reason that needs to be sysfs? Sounds more like an use case for debugfs.
>>>>>
>>>>> Christian.
>>>>>
>>>>>> Signed-off-by: Kent Russell <kent.russell@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 23 +++++++++++++++++++++++
>>>>>>     drivers/gpu/drm/amd/amdgpu/atom.c          |  5 ++++-
>>>>>>     drivers/gpu/drm/amd/amdgpu/atom.h          |  1 +
>>>>>>     3 files changed, 28 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> index a1f9424..f40be71 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>>> @@ -888,6 +888,20 @@ static uint32_t cail_ioreg_read(struct card_info *info, uint32_t reg)
>>>>>>         return r;
>>>>>>     }
>>>>>>
>>>>>> +static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev,
>>>>>> +                                              struct device_attribute *attr,
>>>>>> +                                              char *buf) {
>>>>>> +     struct drm_device *ddev = dev_get_drvdata(dev);
>>>>>> +     struct amdgpu_device *adev = ddev->dev_private;
>>>>>> +     struct atom_context *ctx = adev->mode_info.atom_context;
>>>>>> +
>>>>>> +     return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version);
>>>>>> + }
>>>>>> +
>>>>>> +static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version,
>>>>>> +                NULL);
>>>>>> +
>>>>>>     /**
>>>>>>      * amdgpu_atombios_fini - free the driver info and callbacks for atombios
>>>>>>      *
>>>>>> @@ -907,6 +921,7 @@ static void amdgpu_atombios_fini(struct amdgpu_device *adev)
>>>>>>         adev->mode_info.atom_context = NULL;
>>>>>>         kfree(adev->mode_info.atom_card_info);
>>>>>>         adev->mode_info.atom_card_info = NULL;
>>>>>> +     device_remove_file(adev->dev, &dev_attr_vbios_version);
>>>>>>     }
>>>>>>
>>>>>>     /**
>>>>>> @@ -923,6 +938,7 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>>>>     {
>>>>>>         struct card_info *atom_card_info =
>>>>>>             kzalloc(sizeof(struct card_info), GFP_KERNEL);
>>>>>> +     int ret;
>>>>>>
>>>>>>         if (!atom_card_info)
>>>>>>                 return -ENOMEM;
>>>>>> @@ -959,6 +975,13 @@ static int amdgpu_atombios_init(struct amdgpu_device *adev)
>>>>>>                 amdgpu_atombios_scratch_regs_init(adev);
>>>>>>                 amdgpu_atombios_allocate_fb_scratch(adev);
>>>>>>         }
>>>>>> +
>>>>>> +     ret = device_create_file(adev->dev, &dev_attr_vbios_version);
>>>>>> +     if (ret) {
>>>>>> +             DRM_ERROR("Failed to create device file for VBIOS version\n");
>>>>>> +             return ret;
>>>>>> +     }
>>>>>> +
>>>>>>         return 0;
>>>>>>     }
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>>> index d69aa2e..69500a8 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>>>>>> @@ -1343,8 +1343,11 @@ struct atom_context *amdgpu_atom_parse(struct card_info *card, void *bios)
>>>>>>                 idx = 0x80;
>>>>>>
>>>>>>         str = CSTR(idx);
>>>>>> -     if (*str != '\0')
>>>>>> +     if (*str != '\0') {
>>>>>>                 pr_info("ATOM BIOS: %s\n", str);
>>>>>> +             strlcpy(ctx->vbios_version, str, sizeof(ctx->vbios_version));
>>>>>> +     }
>>>>>> +
>>>>>>
>>>>>>         return ctx;
>>>>>>     }
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>>> b/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>>> index ddd8045..a391709 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
>>>>>> @@ -140,6 +140,7 @@ struct atom_context {
>>>>>>         int io_mode;
>>>>>>         uint32_t *scratch;
>>>>>>         int scratch_size_bytes;
>>>>>> +     char vbios_version[20];
>>>>>>     };
>>>>>>
>>>>>>     extern int amdgpu_atom_debug;
>>>>> _______________________________________________
>>>>> 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
>>> _______________________________________________
>>> 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
>>
> _______________________________________________
> 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


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

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Tom St Denis]

On 25/08/17 08:56 AM, Christian König wrote:
> Hi Kent,
> 
> agree on the VBIOS dump file, that clearly belongs to debugsf.
> 
> The power usage stuff I can't say much about cause I'm not deeply into 
> this, but keep in mind the restriction for sysfs:
> 1. It's a stable interface. So it must be very well designed.
> 2. Only one value per file. I think the power stuff doesn't fulfill that 
> requirement at the moment.

What "power" stuff are we talking about?  The sensors interface or the 
pm_info or something else?

Keep in mind umr uses the sensors debugfs file in --top mode.

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

RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Russell, Kent]

There is GPU Power usage reported through amdgpu_pm_info, which also has some other information as well. I'd like that in sysfs, but I am unsure if we are  allowed to due to the other information reported there.

 Kent

-----Original Message-----
From: StDenis, Tom 
Sent: Friday, August 25, 2017 8:58 AM
To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

On 25/08/17 08:56 AM, Christian König wrote:
> Hi Kent,
> 
> agree on the VBIOS dump file, that clearly belongs to debugsf.
> 
> The power usage stuff I can't say much about cause I'm not deeply into 
> this, but keep in mind the restriction for sysfs:
> 1. It's a stable interface. So it must be very well designed.
> 2. Only one value per file. I think the power stuff doesn't fulfill 
> that requirement at the moment.

What "power" stuff are we talking about?  The sensors interface or the pm_info or something else?

Keep in mind umr uses the sensors debugfs file in --top mode.

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

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Tom St Denis]

On 25/08/17 08:59 AM, Russell, Kent wrote:
> There is GPU Power usage reported through amdgpu_pm_info, which also has some other information as well. I'd like that in sysfs, but I am unsure if we are  allowed to due to the other information reported there.

I thought the sensors were added to the DRM ioctl but apparently not.

Seems like that might be a bit easier place to put them than creating 
sysfs entries for every single sensor (it also means as new sensors are 
added you have to circle around back to add yet more sysfs entries).

Tom



> 
>   Kent
> 
> -----Original Message-----
> From: StDenis, Tom
> Sent: Friday, August 25, 2017 8:58 AM
> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> 
> On 25/08/17 08:56 AM, Christian König wrote:
>> Hi Kent,
>>
>> agree on the VBIOS dump file, that clearly belongs to debugsf.
>>
>> The power usage stuff I can't say much about cause I'm not deeply into
>> this, but keep in mind the restriction for sysfs:
>> 1. It's a stable interface. So it must be very well designed.
>> 2. Only one value per file. I think the power stuff doesn't fulfill
>> that requirement at the moment.
> 
> What "power" stuff are we talking about?  The sensors interface or the pm_info or something else?
> 
> Keep in mind umr uses the sensors debugfs file in --top mode.
> 
> Tom
> 

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

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Tom St Denis]

On 25/08/17 09:05 AM, Tom St Denis wrote:
> On 25/08/17 08:59 AM, Russell, Kent wrote:
>> There is GPU Power usage reported through amdgpu_pm_info, which also 
>> has some other information as well. I'd like that in sysfs, but I am 
>> unsure if we are  allowed to due to the other information reported there.
> 
> I thought the sensors were added to the DRM ioctl but apparently not.

No it was, under AMDGPU_INFO you request info->query == 
AMDGPU_INFO_SENSOR and then specify which sensor you want in 
info->sensor_info.type (see amdgpu_kms.c).

So you can already read this from a normal user process who has an open 
DRM file handle.

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

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Tom St Denis]

On 25/08/17 09:05 AM, Tom St Denis wrote:
> On 25/08/17 08:59 AM, Russell, Kent wrote:
>> There is GPU Power usage reported through amdgpu_pm_info, which also 
>> has some other information as well. I'd like that in sysfs, but I am 
>> unsure if we are  allowed to due to the other information reported there.
> 
> I thought the sensors were added to the DRM ioctl but apparently not.

Nope, it was actually (see amdgpu_kms.c).  Under an AMDGPU_INFO command 
you ask for AMDGPU_INFO_SENSOR and then fill out sensor_info.type.

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

RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Russell, Kent]

Right. And according to Christian, it doesn't fit the format anyways, so power usage can stay. I'm working on the patch to move the vbios version to sysfs from debugfs. Should be up shortly. Maybe I can just make a new sysfs file for Average GPU usage by itself. Another task for another day. But I won't be moving the sensors over. Just vbios version for now, maybe average GPU power usage later.

 Kent

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Tom St Denis
Sent: Friday, August 25, 2017 9:34 AM
To: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

On 25/08/17 09:05 AM, Tom St Denis wrote:
> On 25/08/17 08:59 AM, Russell, Kent wrote:
>> There is GPU Power usage reported through amdgpu_pm_info, which also 
>> has some other information as well. I'd like that in sysfs, but I am 
>> unsure if we are  allowed to due to the other information reported there.
> 
> I thought the sensors were added to the DRM ioctl but apparently not.

Nope, it was actually (see amdgpu_kms.c).  Under an AMDGPU_INFO command you ask for AMDGPU_INFO_SENSOR and then fill out sensor_info.type.

Tom
_______________________________________________
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] drm/amdgpu: Add sysfs file for VBIOS

[Alex Deucher]

On Fri, Aug 25, 2017 at 8:59 AM, Russell, Kent <Kent.Russell@amd.com> wrote:
> There is GPU Power usage reported through amdgpu_pm_info, which also has some other information as well. I'd like that in sysfs, but I am unsure if we are  allowed to due to the other information reported there.
>

We can add a sysfs file for power info, but I'd like the keep the
debugfs one as well since we regularly change the format as we add
features and such.  Depending on what info you want to get, you may
already have what you need via the sysfs pp files.

Alex


>  Kent
>
> -----Original Message-----
> From: StDenis, Tom
> Sent: Friday, August 25, 2017 8:58 AM
> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>
> On 25/08/17 08:56 AM, Christian König wrote:
>> Hi Kent,
>>
>> agree on the VBIOS dump file, that clearly belongs to debugsf.
>>
>> The power usage stuff I can't say much about cause I'm not deeply into
>> this, but keep in mind the restriction for sysfs:
>> 1. It's a stable interface. So it must be very well designed.
>> 2. Only one value per file. I think the power stuff doesn't fulfill
>> that requirement at the moment.
>
> What "power" stuff are we talking about?  The sensors interface or the pm_info or something else?
>
> Keep in mind umr uses the sensors debugfs file in --top mode.
>
> Tom
> _______________________________________________
> 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] drm/amdgpu: Add sysfs file for VBIOS

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Russell, Kent
> Sent: Friday, August 25, 2017 9:00 AM
> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> 
> There is GPU Power usage reported through amdgpu_pm_info, which also
> has some other information as well. I'd like that in sysfs, but I am unsure if
> we are  allowed to due to the other information reported there.

For power and voltage, I believe there are standard hwmon interfaces.  It would probably be best to expose it via those.  For clocks/pcie, I think you can already determine them via the existing pp sysfs interfaces.

Alex

> 
>  Kent
> 
> -----Original Message-----
> From: StDenis, Tom
> Sent: Friday, August 25, 2017 8:58 AM
> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> 
> On 25/08/17 08:56 AM, Christian König wrote:
> > Hi Kent,
> >
> > agree on the VBIOS dump file, that clearly belongs to debugsf.
> >
> > The power usage stuff I can't say much about cause I'm not deeply into
> > this, but keep in mind the restriction for sysfs:
> > 1. It's a stable interface. So it must be very well designed.
> > 2. Only one value per file. I think the power stuff doesn't fulfill
> > that requirement at the moment.
> 
> What "power" stuff are we talking about?  The sensors interface or the
> pm_info or something else?
> 
> Keep in mind umr uses the sensors debugfs file in --top mode.
> 
> Tom
> _______________________________________________
> 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] drm/amdgpu: Add sysfs file for VBIOS

[Russell, Kent]

That sounds good. There's already the sclk/mclk/pcieclk in the device sysfs tree, it was just power that we had to obtain in the debugfs. Thanks, I'll work on that now. And I'll add voltage because I am sure someone will ask for it soon enough. hwmon makes perfect sense though, I'll add it in there.

 Kent

-----Original Message-----
From: Deucher, Alexander 
Sent: Friday, August 25, 2017 11:56 AM
To: Russell, Kent; StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf 
> Of Russell, Kent
> Sent: Friday, August 25, 2017 9:00 AM
> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> 
> There is GPU Power usage reported through amdgpu_pm_info, which also 
> has some other information as well. I'd like that in sysfs, but I am 
> unsure if we are  allowed to due to the other information reported there.

For power and voltage, I believe there are standard hwmon interfaces.  It would probably be best to expose it via those.  For clocks/pcie, I think you can already determine them via the existing pp sysfs interfaces.

Alex

> 
>  Kent
> 
> -----Original Message-----
> From: StDenis, Tom
> Sent: Friday, August 25, 2017 8:58 AM
> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> 
> On 25/08/17 08:56 AM, Christian König wrote:
> > Hi Kent,
> >
> > agree on the VBIOS dump file, that clearly belongs to debugsf.
> >
> > The power usage stuff I can't say much about cause I'm not deeply 
> > into this, but keep in mind the restriction for sysfs:
> > 1. It's a stable interface. So it must be very well designed.
> > 2. Only one value per file. I think the power stuff doesn't fulfill 
> > that requirement at the moment.
> 
> What "power" stuff are we talking about?  The sensors interface or the 
> pm_info or something else?
> 
> Keep in mind umr uses the sensors debugfs file in --top mode.
> 
> Tom
> _______________________________________________
> 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] drm/amdgpu: Add sysfs file for VBIOS

[Felix Kuehling]

I think the power measurement is a bit of a hack right now. It requires
pinging the SMU twice to start and stop the measurement, and waiting for
some time (but not too long) in between. I think if we want to expose
this via hwmon, we'll need to have some kind of timer that polls it in
regular intervals in the background and updates a value that can be
queried through HWMon without delay.

Regards,
  Felix


On 2017-08-25 11:56 AM, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Russell, Kent
>> Sent: Friday, August 25, 2017 9:00 AM
>> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org
>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> There is GPU Power usage reported through amdgpu_pm_info, which also
>> has some other information as well. I'd like that in sysfs, but I am unsure if
>> we are  allowed to due to the other information reported there.
> For power and voltage, I believe there are standard hwmon interfaces.  It would probably be best to expose it via those.  For clocks/pcie, I think you can already determine them via the existing pp sysfs interfaces.
>
> Alex
>
>>  Kent
>>
>> -----Original Message-----
>> From: StDenis, Tom
>> Sent: Friday, August 25, 2017 8:58 AM
>> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> On 25/08/17 08:56 AM, Christian König wrote:
>>> Hi Kent,
>>>
>>> agree on the VBIOS dump file, that clearly belongs to debugsf.
>>>
>>> The power usage stuff I can't say much about cause I'm not deeply into
>>> this, but keep in mind the restriction for sysfs:
>>> 1. It's a stable interface. So it must be very well designed.
>>> 2. Only one value per file. I think the power stuff doesn't fulfill
>>> that requirement at the moment.
>> What "power" stuff are we talking about?  The sensors interface or the
>> pm_info or something else?
>>
>> Keep in mind umr uses the sensors debugfs file in --top mode.
>>
>> Tom
>> _______________________________________________
>> 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

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

Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Tom St Denis]

On 25/08/17 03:33 PM, Felix Kuehling wrote:
> I think the power measurement is a bit of a hack right now. It requires
> pinging the SMU twice to start and stop the measurement, and waiting for
> some time (but not too long) in between. I think if we want to expose
> this via hwmon, we'll need to have some kind of timer that polls it in
> regular intervals in the background and updates a value that can be
> queried through HWMon without delay.

We had this debate about 8 months ago :-)

I don't know if adding an ever present kthread just for this though is a 
super use of resources.

In umr when I read it I read the sensors in a separate thread from the 
main UI so that it doesn't mess with things.

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

RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Felix Kuehling
> Sent: Friday, August 25, 2017 3:34 PM
> To: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> 
> I think the power measurement is a bit of a hack right now. It requires
> pinging the SMU twice to start and stop the measurement, and waiting for
> some time (but not too long) in between. I think if we want to expose
> this via hwmon, we'll need to have some kind of timer that polls it in
> regular intervals in the background and updates a value that can be
> queried through HWMon without delay.

I don't know that hwmon has any requirements with respect to delay.  I think a slight delay in reading it back through hwmon is preferable to a background thread polling it. Regular polling may also have negative impacts on performance or stability, depending on how it was validated.

Alex

> 
> Regards,
>   Felix
> 
> 
> On 2017-08-25 11:56 AM, Deucher, Alexander wrote:
> >> -----Original Message-----
> >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> Behalf
> >> Of Russell, Kent
> >> Sent: Friday, August 25, 2017 9:00 AM
> >> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org
> >> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> >>
> >> There is GPU Power usage reported through amdgpu_pm_info, which
> also
> >> has some other information as well. I'd like that in sysfs, but I am unsure if
> >> we are  allowed to due to the other information reported there.
> > For power and voltage, I believe there are standard hwmon interfaces.  It
> would probably be best to expose it via those.  For clocks/pcie, I think you
> can already determine them via the existing pp sysfs interfaces.
> >
> > Alex
> >
> >>  Kent
> >>
> >> -----Original Message-----
> >> From: StDenis, Tom
> >> Sent: Friday, August 25, 2017 8:58 AM
> >> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org
> >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> >>
> >> On 25/08/17 08:56 AM, Christian König wrote:
> >>> Hi Kent,
> >>>
> >>> agree on the VBIOS dump file, that clearly belongs to debugsf.
> >>>
> >>> The power usage stuff I can't say much about cause I'm not deeply into
> >>> this, but keep in mind the restriction for sysfs:
> >>> 1. It's a stable interface. So it must be very well designed.
> >>> 2. Only one value per file. I think the power stuff doesn't fulfill
> >>> that requirement at the moment.
> >> What "power" stuff are we talking about?  The sensors interface or the
> >> pm_info or something else?
> >>
> >> Keep in mind umr uses the sensors debugfs file in --top mode.
> >>
> >> Tom
> >> _______________________________________________
> >> 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
> 
> _______________________________________________
> 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] drm/amdgpu: Add sysfs file for VBIOS

[Felix Kuehling]

On 2017-08-25 03:40 PM, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Felix Kuehling
>> Sent: Friday, August 25, 2017 3:34 PM
>> To: amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>
>> I think the power measurement is a bit of a hack right now. It requires
>> pinging the SMU twice to start and stop the measurement, and waiting for
>> some time (but not too long) in between. I think if we want to expose
>> this via hwmon, we'll need to have some kind of timer that polls it in
>> regular intervals in the background and updates a value that can be
>> queried through HWMon without delay.
> I don't know that hwmon has any requirements with respect to delay.  I think a slight delay in reading it back through hwmon is preferable to a background thread polling it. Regular polling may also have negative impacts on performance or stability, depending on how it was validated.

If an application is reading hwmon data from 4 GPUs and each query for
power usage from each GPU takes one second, that would seriously limit
the update frequency of the application.

Maybe the first read could block long enough to get useful data. But
subsequent reads could probably avoid blocking if we keep track of the
time stamp of the last query. If it was very recently, we can return the
last value. If it was long enough ago, we can get an updated value from
the SMU. If the last query was too long ago, we start over and block for
a second.

Regards,
  Felix

> Alex
>
>> Regards,
>>   Felix
>>
>>
>> On 2017-08-25 11:56 AM, Deucher, Alexander wrote:
>>>> -----Original Message-----
>>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
>> Behalf
>>>> Of Russell, Kent
>>>> Sent: Friday, August 25, 2017 9:00 AM
>>>> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org
>>>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>>
>>>> There is GPU Power usage reported through amdgpu_pm_info, which
>> also
>>>> has some other information as well. I'd like that in sysfs, but I am unsure if
>>>> we are  allowed to due to the other information reported there.
>>> For power and voltage, I believe there are standard hwmon interfaces.  It
>> would probably be best to expose it via those.  For clocks/pcie, I think you
>> can already determine them via the existing pp sysfs interfaces.
>>> Alex
>>>
>>>>  Kent
>>>>
>>>> -----Original Message-----
>>>> From: StDenis, Tom
>>>> Sent: Friday, August 25, 2017 8:58 AM
>>>> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org
>>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
>>>>
>>>> On 25/08/17 08:56 AM, Christian König wrote:
>>>>> Hi Kent,
>>>>>
>>>>> agree on the VBIOS dump file, that clearly belongs to debugsf.
>>>>>
>>>>> The power usage stuff I can't say much about cause I'm not deeply into
>>>>> this, but keep in mind the restriction for sysfs:
>>>>> 1. It's a stable interface. So it must be very well designed.
>>>>> 2. Only one value per file. I think the power stuff doesn't fulfill
>>>>> that requirement at the moment.
>>>> What "power" stuff are we talking about?  The sensors interface or the
>>>> pm_info or something else?
>>>>
>>>> Keep in mind umr uses the sensors debugfs file in --top mode.
>>>>
>>>> Tom
>>>> _______________________________________________
>>>> 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
>> _______________________________________________
>> 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] drm/amdgpu: Add sysfs file for VBIOS

[Deucher, Alexander]

> -----Original Message-----
> From: Kuehling, Felix
> Sent: Friday, August 25, 2017 3:59 PM
> To: Deucher, Alexander; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> 
> On 2017-08-25 03:40 PM, Deucher, Alexander wrote:
> >> -----Original Message-----
> >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> Behalf
> >> Of Felix Kuehling
> >> Sent: Friday, August 25, 2017 3:34 PM
> >> To: amd-gfx@lists.freedesktop.org
> >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> >>
> >> I think the power measurement is a bit of a hack right now. It requires
> >> pinging the SMU twice to start and stop the measurement, and waiting
> for
> >> some time (but not too long) in between. I think if we want to expose
> >> this via hwmon, we'll need to have some kind of timer that polls it in
> >> regular intervals in the background and updates a value that can be
> >> queried through HWMon without delay.
> > I don't know that hwmon has any requirements with respect to delay.  I
> think a slight delay in reading it back through hwmon is preferable to a
> background thread polling it. Regular polling may also have negative impacts
> on performance or stability, depending on how it was validated.
> 
> If an application is reading hwmon data from 4 GPUs and each query for
> power usage from each GPU takes one second, that would seriously limit
> the update frequency of the application.
> 
> Maybe the first read could block long enough to get useful data. But
> subsequent reads could probably avoid blocking if we keep track of the
> time stamp of the last query. If it was very recently, we can return the
> last value. If it was long enough ago, we can get an updated value from
> the SMU. If the last query was too long ago, we start over and block for
> a second.

Could we just set the sampling period down?  If the application is polling for instantaneous power we'd probably want a short smu sampling period anyway.  Otherwise, the application shouldn’t be polling so frequently in the first place.

Alex

> 
> Regards,
>   Felix
> 
> > Alex
> >
> >> Regards,
> >>   Felix
> >>
> >>
> >> On 2017-08-25 11:56 AM, Deucher, Alexander wrote:
> >>>> -----Original Message-----
> >>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> >> Behalf
> >>>> Of Russell, Kent
> >>>> Sent: Friday, August 25, 2017 9:00 AM
> >>>> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org
> >>>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> >>>>
> >>>> There is GPU Power usage reported through amdgpu_pm_info, which
> >> also
> >>>> has some other information as well. I'd like that in sysfs, but I am
> unsure if
> >>>> we are  allowed to due to the other information reported there.
> >>> For power and voltage, I believe there are standard hwmon interfaces.
> It
> >> would probably be best to expose it via those.  For clocks/pcie, I think you
> >> can already determine them via the existing pp sysfs interfaces.
> >>> Alex
> >>>
> >>>>  Kent
> >>>>
> >>>> -----Original Message-----
> >>>> From: StDenis, Tom
> >>>> Sent: Friday, August 25, 2017 8:58 AM
> >>>> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org
> >>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> >>>>
> >>>> On 25/08/17 08:56 AM, Christian König wrote:
> >>>>> Hi Kent,
> >>>>>
> >>>>> agree on the VBIOS dump file, that clearly belongs to debugsf.
> >>>>>
> >>>>> The power usage stuff I can't say much about cause I'm not deeply
> into
> >>>>> this, but keep in mind the restriction for sysfs:
> >>>>> 1. It's a stable interface. So it must be very well designed.
> >>>>> 2. Only one value per file. I think the power stuff doesn't fulfill
> >>>>> that requirement at the moment.
> >>>> What "power" stuff are we talking about?  The sensors interface or the
> >>>> pm_info or something else?
> >>>>
> >>>> Keep in mind umr uses the sensors debugfs file in --top mode.
> >>>>
> >>>> Tom
> >>>> _______________________________________________
> >>>> 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
> >> _______________________________________________
> >> 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] drm/amdgpu: Add sysfs file for VBIOS

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Deucher, Alexander
> Sent: Friday, August 25, 2017 4:07 PM
> To: Kuehling, Felix; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> 
> > -----Original Message-----
> > From: Kuehling, Felix
> > Sent: Friday, August 25, 2017 3:59 PM
> > To: Deucher, Alexander; amd-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> >
> > On 2017-08-25 03:40 PM, Deucher, Alexander wrote:
> > >> -----Original Message-----
> > >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> > Behalf
> > >> Of Felix Kuehling
> > >> Sent: Friday, August 25, 2017 3:34 PM
> > >> To: amd-gfx@lists.freedesktop.org
> > >> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> > >>
> > >> I think the power measurement is a bit of a hack right now. It requires
> > >> pinging the SMU twice to start and stop the measurement, and waiting
> > for
> > >> some time (but not too long) in between. I think if we want to expose
> > >> this via hwmon, we'll need to have some kind of timer that polls it in
> > >> regular intervals in the background and updates a value that can be
> > >> queried through HWMon without delay.
> > > I don't know that hwmon has any requirements with respect to delay.  I
> > think a slight delay in reading it back through hwmon is preferable to a
> > background thread polling it. Regular polling may also have negative
> impacts
> > on performance or stability, depending on how it was validated.
> >
> > If an application is reading hwmon data from 4 GPUs and each query for
> > power usage from each GPU takes one second, that would seriously limit
> > the update frequency of the application.
> >
> > Maybe the first read could block long enough to get useful data. But
> > subsequent reads could probably avoid blocking if we keep track of the
> > time stamp of the last query. If it was very recently, we can return the
> > last value. If it was long enough ago, we can get an updated value from
> > the SMU. If the last query was too long ago, we start over and block for
> > a second.
> 
> Could we just set the sampling period down?  If the application is polling for
> instantaneous power we'd probably want a short smu sampling period
> anyway.  Otherwise, the application shouldn’t be polling so frequently in the
> first place.

Another alternative would be to expose the SMU interface directly.  So getting the power would be something like:

echo 1 > power
#wait
echo 0 > power
cat power

or:
echo 2 > power # seconds to sample
cat power #returns -EBUSY until the sampling period is done

Alex

> 
> Alex
> 
> >
> > Regards,
> >   Felix
> >
> > > Alex
> > >
> > >> Regards,
> > >>   Felix
> > >>
> > >>
> > >> On 2017-08-25 11:56 AM, Deucher, Alexander wrote:
> > >>>> -----Original Message-----
> > >>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> > >> Behalf
> > >>>> Of Russell, Kent
> > >>>> Sent: Friday, August 25, 2017 9:00 AM
> > >>>> To: StDenis, Tom; Christian König; amd-gfx@lists.freedesktop.org
> > >>>> Subject: RE: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> > >>>>
> > >>>> There is GPU Power usage reported through amdgpu_pm_info,
> which
> > >> also
> > >>>> has some other information as well. I'd like that in sysfs, but I am
> > unsure if
> > >>>> we are  allowed to due to the other information reported there.
> > >>> For power and voltage, I believe there are standard hwmon interfaces.
> > It
> > >> would probably be best to expose it via those.  For clocks/pcie, I think
> you
> > >> can already determine them via the existing pp sysfs interfaces.
> > >>> Alex
> > >>>
> > >>>>  Kent
> > >>>>
> > >>>> -----Original Message-----
> > >>>> From: StDenis, Tom
> > >>>> Sent: Friday, August 25, 2017 8:58 AM
> > >>>> To: Christian König; Russell, Kent; amd-gfx@lists.freedesktop.org
> > >>>> Subject: Re: [PATCH] drm/amdgpu: Add sysfs file for VBIOS
> > >>>>
> > >>>> On 25/08/17 08:56 AM, Christian König wrote:
> > >>>>> Hi Kent,
> > >>>>>
> > >>>>> agree on the VBIOS dump file, that clearly belongs to debugsf.
> > >>>>>
> > >>>>> The power usage stuff I can't say much about cause I'm not deeply
> > into
> > >>>>> this, but keep in mind the restriction for sysfs:
> > >>>>> 1. It's a stable interface. So it must be very well designed.
> > >>>>> 2. Only one value per file. I think the power stuff doesn't fulfill
> > >>>>> that requirement at the moment.
> > >>>> What "power" stuff are we talking about?  The sensors interface or
> the
> > >>>> pm_info or something else?
> > >>>>
> > >>>> Keep in mind umr uses the sensors debugfs file in --top mode.
> > >>>>
> > >>>> Tom
> > >>>> _______________________________________________
> > >>>> 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
> > >> _______________________________________________
> > >> 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx