[PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[Luben Tuikov]

Add the "harvest" field to the IP attributes in
the IP discovery sysfs visualization, as this
field is present in the binary data.
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index c8dbdb78988ce0..0496d369504641 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -393,6 +393,7 @@ struct ip_hw_instance {
 	int hw_id;
 	u8  num_instance;
 	u8  major, minor, revision;
+	u8  harvest;
 
 	int num_base_addresses;
 	u32 base_addr[];
@@ -440,6 +441,11 @@ static ssize_t revision_show(struct ip_hw_instance *ip_hw_instance, char *buf)
 	return sysfs_emit(buf, "%d\n", ip_hw_instance->revision);
 }
 
+static ssize_t harvest_show(struct ip_hw_instance *ip_hw_instance, char *buf)
+{
+	return sysfs_emit(buf, "0x%01X\n", ip_hw_instance->harvest);
+}
+
 static ssize_t num_base_addresses_show(struct ip_hw_instance *ip_hw_instance, char *buf)
 {
 	return sysfs_emit(buf, "%d\n", ip_hw_instance->num_base_addresses);
@@ -471,6 +477,7 @@ static struct ip_hw_instance_attr ip_hw_attr[] = {
 	__ATTR_RO(major),
 	__ATTR_RO(minor),
 	__ATTR_RO(revision),
+	__ATTR_RO(harvest),
 	__ATTR_RO(num_base_addresses),
 	__ATTR_RO(base_addr),
 };
@@ -708,6 +715,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev,
 			ip_hw_instance->major = ip->major;
 			ip_hw_instance->minor = ip->minor;
 			ip_hw_instance->revision = ip->revision;
+			ip_hw_instance->harvest = ip->harvest;
 			ip_hw_instance->num_base_addresses = ip->num_base_address;
 
 			for (kk = 0; kk < ip_hw_instance->num_base_addresses; kk++)

base-commit: d8604f1d237a145db48bae4ea60b85a5875df307
-- 
2.35.1.102.g2b9c120970

[PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[Luben Tuikov]

Add the "harvest" field to the IP attributes in
the IP discovery sysfs visualization, as this
field is present in the binary data.

Cc: Alex Deucher <Alexander.Deucher@amd.com>
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index c8dbdb78988ce0..0496d369504641 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -393,6 +393,7 @@ struct ip_hw_instance {
 	int hw_id;
 	u8  num_instance;
 	u8  major, minor, revision;
+	u8  harvest;
 
 	int num_base_addresses;
 	u32 base_addr[];
@@ -440,6 +441,11 @@ static ssize_t revision_show(struct ip_hw_instance *ip_hw_instance, char *buf)
 	return sysfs_emit(buf, "%d\n", ip_hw_instance->revision);
 }
 
+static ssize_t harvest_show(struct ip_hw_instance *ip_hw_instance, char *buf)
+{
+	return sysfs_emit(buf, "0x%01X\n", ip_hw_instance->harvest);
+}
+
 static ssize_t num_base_addresses_show(struct ip_hw_instance *ip_hw_instance, char *buf)
 {
 	return sysfs_emit(buf, "%d\n", ip_hw_instance->num_base_addresses);
@@ -471,6 +477,7 @@ static struct ip_hw_instance_attr ip_hw_attr[] = {
 	__ATTR_RO(major),
 	__ATTR_RO(minor),
 	__ATTR_RO(revision),
+	__ATTR_RO(harvest),
 	__ATTR_RO(num_base_addresses),
 	__ATTR_RO(base_addr),
 };
@@ -708,6 +715,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev,
 			ip_hw_instance->major = ip->major;
 			ip_hw_instance->minor = ip->minor;
 			ip_hw_instance->revision = ip->revision;
+			ip_hw_instance->harvest = ip->harvest;
 			ip_hw_instance->num_base_addresses = ip->num_base_address;
 
 			for (kk = 0; kk < ip_hw_instance->num_base_addresses; kk++)

base-commit: d8604f1d237a145db48bae4ea60b85a5875df307
-- 
2.35.1.102.g2b9c120970

RE: [PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[Chen, Guchun]

[Public]

Hi Luben,

I suggest holding on this pls. Harvest bit per IP data structure from VBIOS is not consistently correct. Exposing it to use via sysfs may confuse users.

Regards,
Guchun

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Luben Tuikov
Sent: Tuesday, February 15, 2022 7:22 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Tuikov, Luben <Luben.Tuikov@amd.com>
Subject: [PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

Add the "harvest" field to the IP attributes in the IP discovery sysfs visualization, as this field is present in the binary data.

Cc: Alex Deucher <Alexander.Deucher@amd.com>
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index c8dbdb78988ce0..0496d369504641 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -393,6 +393,7 @@ struct ip_hw_instance {
 	int hw_id;
 	u8  num_instance;
 	u8  major, minor, revision;
+	u8  harvest;
 
 	int num_base_addresses;
 	u32 base_addr[];
@@ -440,6 +441,11 @@ static ssize_t revision_show(struct ip_hw_instance *ip_hw_instance, char *buf)
 	return sysfs_emit(buf, "%d\n", ip_hw_instance->revision);  }
 
+static ssize_t harvest_show(struct ip_hw_instance *ip_hw_instance, char 
+*buf) {
+	return sysfs_emit(buf, "0x%01X\n", ip_hw_instance->harvest); }
+
 static ssize_t num_base_addresses_show(struct ip_hw_instance *ip_hw_instance, char *buf)  {
 	return sysfs_emit(buf, "%d\n", ip_hw_instance->num_base_addresses);
@@ -471,6 +477,7 @@ static struct ip_hw_instance_attr ip_hw_attr[] = {
 	__ATTR_RO(major),
 	__ATTR_RO(minor),
 	__ATTR_RO(revision),
+	__ATTR_RO(harvest),
 	__ATTR_RO(num_base_addresses),
 	__ATTR_RO(base_addr),
 };
@@ -708,6 +715,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev,
 			ip_hw_instance->major = ip->major;
 			ip_hw_instance->minor = ip->minor;
 			ip_hw_instance->revision = ip->revision;
+			ip_hw_instance->harvest = ip->harvest;
 			ip_hw_instance->num_base_addresses = ip->num_base_address;
 
 			for (kk = 0; kk < ip_hw_instance->num_base_addresses; kk++)

base-commit: d8604f1d237a145db48bae4ea60b85a5875df307
--
2.35.1.102.g2b9c120970

Re: [PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[Luben Tuikov]

At the moment it's 0 across all IPs.
I doubt there are any consumers of the harvest data at the moment.
Since this will be fixed in VBIOS eventually, I suggest we add it now for completeness and it'll be there when this is fixed in VBIOS.

Regards,
Luben


On 2022-02-15 03:58, Chen, Guchun wrote:
> [Public]
> 
> Hi Luben,
> 
> I suggest holding on this pls. Harvest bit per IP data structure from VBIOS is not consistently correct. Exposing it to use via sysfs may confuse users.
> 
> Regards,
> Guchun
> 
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Luben Tuikov
> Sent: Tuesday, February 15, 2022 7:22 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Tuikov, Luben <Luben.Tuikov@amd.com>
> Subject: [PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs
> 
> Add the "harvest" field to the IP attributes in the IP discovery sysfs visualization, as this field is present in the binary data.
> 
> Cc: Alex Deucher <Alexander.Deucher@amd.com>
> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index c8dbdb78988ce0..0496d369504641 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -393,6 +393,7 @@ struct ip_hw_instance {
>  	int hw_id;
>  	u8  num_instance;
>  	u8  major, minor, revision;
> +	u8  harvest;
>  
>  	int num_base_addresses;
>  	u32 base_addr[];
> @@ -440,6 +441,11 @@ static ssize_t revision_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>  	return sysfs_emit(buf, "%d\n", ip_hw_instance->revision);  }
>  
> +static ssize_t harvest_show(struct ip_hw_instance *ip_hw_instance, char 
> +*buf) {
> +	return sysfs_emit(buf, "0x%01X\n", ip_hw_instance->harvest); }
> +
>  static ssize_t num_base_addresses_show(struct ip_hw_instance *ip_hw_instance, char *buf)  {
>  	return sysfs_emit(buf, "%d\n", ip_hw_instance->num_base_addresses);
> @@ -471,6 +477,7 @@ static struct ip_hw_instance_attr ip_hw_attr[] = {
>  	__ATTR_RO(major),
>  	__ATTR_RO(minor),
>  	__ATTR_RO(revision),
> +	__ATTR_RO(harvest),
>  	__ATTR_RO(num_base_addresses),
>  	__ATTR_RO(base_addr),
>  };
> @@ -708,6 +715,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev,
>  			ip_hw_instance->major = ip->major;
>  			ip_hw_instance->minor = ip->minor;
>  			ip_hw_instance->revision = ip->revision;
> +			ip_hw_instance->harvest = ip->harvest;
>  			ip_hw_instance->num_base_addresses = ip->num_base_address;
>  
>  			for (kk = 0; kk < ip_hw_instance->num_base_addresses; kk++)
> 
> base-commit: d8604f1d237a145db48bae4ea60b85a5875df307
> --
> 2.35.1.102.g2b9c120970

Regards,
-- 
Luben

Re: [PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[Christian König]

Am 15.02.22 um 00:21 schrieb Luben Tuikov:
> Add the "harvest" field to the IP attributes in
> the IP discovery sysfs visualization, as this
> field is present in the binary data.
>
> Cc: Alex Deucher <Alexander.Deucher@amd.com>
> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index c8dbdb78988ce0..0496d369504641 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -393,6 +393,7 @@ struct ip_hw_instance {
>   	int hw_id;
>   	u8  num_instance;
>   	u8  major, minor, revision;
> +	u8  harvest;

Should we maybe use bool here instead?

Apart from that looks good to me.

Regards,
Christian.

>   
>   	int num_base_addresses;
>   	u32 base_addr[];
> @@ -440,6 +441,11 @@ static ssize_t revision_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>   	return sysfs_emit(buf, "%d\n", ip_hw_instance->revision);
>   }
>   
> +static ssize_t harvest_show(struct ip_hw_instance *ip_hw_instance, char *buf)
> +{
> +	return sysfs_emit(buf, "0x%01X\n", ip_hw_instance->harvest);
> +}
> +
>   static ssize_t num_base_addresses_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>   {
>   	return sysfs_emit(buf, "%d\n", ip_hw_instance->num_base_addresses);
> @@ -471,6 +477,7 @@ static struct ip_hw_instance_attr ip_hw_attr[] = {
>   	__ATTR_RO(major),
>   	__ATTR_RO(minor),
>   	__ATTR_RO(revision),
> +	__ATTR_RO(harvest),
>   	__ATTR_RO(num_base_addresses),
>   	__ATTR_RO(base_addr),
>   };
> @@ -708,6 +715,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev,
>   			ip_hw_instance->major = ip->major;
>   			ip_hw_instance->minor = ip->minor;
>   			ip_hw_instance->revision = ip->revision;
> +			ip_hw_instance->harvest = ip->harvest;
>   			ip_hw_instance->num_base_addresses = ip->num_base_address;
>   
>   			for (kk = 0; kk < ip_hw_instance->num_base_addresses; kk++)
>
> base-commit: d8604f1d237a145db48bae4ea60b85a5875df307

Re: [PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[Luben Tuikov]

On 2022-02-15 09:25, Christian König wrote:
> Am 15.02.22 um 00:21 schrieb Luben Tuikov:
>> Add the "harvest" field to the IP attributes in
>> the IP discovery sysfs visualization, as this
>> field is present in the binary data.
>>
>> Cc: Alex Deucher <Alexander.Deucher@amd.com>
>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>> index c8dbdb78988ce0..0496d369504641 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>> @@ -393,6 +393,7 @@ struct ip_hw_instance {
>>   	int hw_id;
>>   	u8  num_instance;
>>   	u8  major, minor, revision;
>> +	u8  harvest;
> 
> Should we maybe use bool here instead?
> 
> Apart from that looks good to me.

Thanks Christian.

I don't mind using bool here.

I saw the field in the binary data is 4 bits and represented that.

Is the field actually bool in the IP binary data?
I can change the patch and resubmit.

Regards,
Luben

> 
> Regards,
> Christian.
> 
>>   
>>   	int num_base_addresses;
>>   	u32 base_addr[];
>> @@ -440,6 +441,11 @@ static ssize_t revision_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>>   	return sysfs_emit(buf, "%d\n", ip_hw_instance->revision);
>>   }
>>   
>> +static ssize_t harvest_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>> +{
>> +	return sysfs_emit(buf, "0x%01X\n", ip_hw_instance->harvest);
>> +}
>> +
>>   static ssize_t num_base_addresses_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>>   {
>>   	return sysfs_emit(buf, "%d\n", ip_hw_instance->num_base_addresses);
>> @@ -471,6 +477,7 @@ static struct ip_hw_instance_attr ip_hw_attr[] = {
>>   	__ATTR_RO(major),
>>   	__ATTR_RO(minor),
>>   	__ATTR_RO(revision),
>> +	__ATTR_RO(harvest),
>>   	__ATTR_RO(num_base_addresses),
>>   	__ATTR_RO(base_addr),
>>   };
>> @@ -708,6 +715,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev,
>>   			ip_hw_instance->major = ip->major;
>>   			ip_hw_instance->minor = ip->minor;
>>   			ip_hw_instance->revision = ip->revision;
>> +			ip_hw_instance->harvest = ip->harvest;
>>   			ip_hw_instance->num_base_addresses = ip->num_base_address;
>>   
>>   			for (kk = 0; kk < ip_hw_instance->num_base_addresses; kk++)
>>
>> base-commit: d8604f1d237a145db48bae4ea60b85a5875df307
> 

Regards,
-- 
Luben

Re: [PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[Christian König]

Am 15.02.22 um 15:33 schrieb Luben Tuikov:
> On 2022-02-15 09:25, Christian König wrote:
>> Am 15.02.22 um 00:21 schrieb Luben Tuikov:
>>> Add the "harvest" field to the IP attributes in
>>> the IP discovery sysfs visualization, as this
>>> field is present in the binary data.
>>>
>>> Cc: Alex Deucher <Alexander.Deucher@amd.com>
>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>>> index c8dbdb78988ce0..0496d369504641 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>>> @@ -393,6 +393,7 @@ struct ip_hw_instance {
>>>    	int hw_id;
>>>    	u8  num_instance;
>>>    	u8  major, minor, revision;
>>> +	u8  harvest;
>> Should we maybe use bool here instead?
>>
>> Apart from that looks good to me.
> Thanks Christian.
>
> I don't mind using bool here.
>
> I saw the field in the binary data is 4 bits and represented that.
>
> Is the field actually bool in the IP binary data?

I'm not sure either.

I would have expected it to be a single bit flag in the binary, but 
4bits sounds like it serves some more purpose.

Probably best to stick to u8 for now in this case.

Regards,
Christian.

> I can change the patch and resubmit.
>
> Regards,
> Luben
>
>> Regards,
>> Christian.
>>
>>>    
>>>    	int num_base_addresses;
>>>    	u32 base_addr[];
>>> @@ -440,6 +441,11 @@ static ssize_t revision_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>>>    	return sysfs_emit(buf, "%d\n", ip_hw_instance->revision);
>>>    }
>>>    
>>> +static ssize_t harvest_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>>> +{
>>> +	return sysfs_emit(buf, "0x%01X\n", ip_hw_instance->harvest);
>>> +}
>>> +
>>>    static ssize_t num_base_addresses_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>>>    {
>>>    	return sysfs_emit(buf, "%d\n", ip_hw_instance->num_base_addresses);
>>> @@ -471,6 +477,7 @@ static struct ip_hw_instance_attr ip_hw_attr[] = {
>>>    	__ATTR_RO(major),
>>>    	__ATTR_RO(minor),
>>>    	__ATTR_RO(revision),
>>> +	__ATTR_RO(harvest),
>>>    	__ATTR_RO(num_base_addresses),
>>>    	__ATTR_RO(base_addr),
>>>    };
>>> @@ -708,6 +715,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev,
>>>    			ip_hw_instance->major = ip->major;
>>>    			ip_hw_instance->minor = ip->minor;
>>>    			ip_hw_instance->revision = ip->revision;
>>> +			ip_hw_instance->harvest = ip->harvest;
>>>    			ip_hw_instance->num_base_addresses = ip->num_base_address;
>>>    
>>>    			for (kk = 0; kk < ip_hw_instance->num_base_addresses; kk++)
>>>
>>> base-commit: d8604f1d237a145db48bae4ea60b85a5875df307
> Regards,

Re: [PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[Luben Tuikov]

On 2022-02-15 09:37, Christian König wrote:
> 
> 
> Am 15.02.22 um 15:33 schrieb Luben Tuikov:
>> On 2022-02-15 09:25, Christian König wrote:
>>> Am 15.02.22 um 00:21 schrieb Luben Tuikov:
>>>> Add the "harvest" field to the IP attributes in
>>>> the IP discovery sysfs visualization, as this
>>>> field is present in the binary data.
>>>>
>>>> Cc: Alex Deucher <Alexander.Deucher@amd.com>
>>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>>>> index c8dbdb78988ce0..0496d369504641 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>>>> @@ -393,6 +393,7 @@ struct ip_hw_instance {
>>>>    	int hw_id;
>>>>    	u8  num_instance;
>>>>    	u8  major, minor, revision;
>>>> +	u8  harvest;
>>> Should we maybe use bool here instead?
>>>
>>> Apart from that looks good to me.
>> Thanks Christian.
>>
>> I don't mind using bool here.
>>
>> I saw the field in the binary data is 4 bits and represented that.
>>
>> Is the field actually bool in the IP binary data?
> 
> I'm not sure either.
> 
> I would have expected it to be a single bit flag in the binary, but 
> 4bits sounds like it serves some more purpose.
> 
> Probably best to stick to u8 for now in this case.

Okay. Can I get a R-B then?

Regards,
Luben

> 
> Regards,
> Christian.
> 
>> I can change the patch and resubmit.
>>
>> Regards,
>> Luben
>>
>>> Regards,
>>> Christian.
>>>
>>>>    
>>>>    	int num_base_addresses;
>>>>    	u32 base_addr[];
>>>> @@ -440,6 +441,11 @@ static ssize_t revision_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>>>>    	return sysfs_emit(buf, "%d\n", ip_hw_instance->revision);
>>>>    }
>>>>    
>>>> +static ssize_t harvest_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>>>> +{
>>>> +	return sysfs_emit(buf, "0x%01X\n", ip_hw_instance->harvest);
>>>> +}
>>>> +
>>>>    static ssize_t num_base_addresses_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>>>>    {
>>>>    	return sysfs_emit(buf, "%d\n", ip_hw_instance->num_base_addresses);
>>>> @@ -471,6 +477,7 @@ static struct ip_hw_instance_attr ip_hw_attr[] = {
>>>>    	__ATTR_RO(major),
>>>>    	__ATTR_RO(minor),
>>>>    	__ATTR_RO(revision),
>>>> +	__ATTR_RO(harvest),
>>>>    	__ATTR_RO(num_base_addresses),
>>>>    	__ATTR_RO(base_addr),
>>>>    };
>>>> @@ -708,6 +715,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev,
>>>>    			ip_hw_instance->major = ip->major;
>>>>    			ip_hw_instance->minor = ip->minor;
>>>>    			ip_hw_instance->revision = ip->revision;
>>>> +			ip_hw_instance->harvest = ip->harvest;
>>>>    			ip_hw_instance->num_base_addresses = ip->num_base_address;
>>>>    
>>>>    			for (kk = 0; kk < ip_hw_instance->num_base_addresses; kk++)
>>>>
>>>> base-commit: d8604f1d237a145db48bae4ea60b85a5875df307
>> Regards,
> 

Regards,
-- 
Luben

Re: [PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[Christian König]

Am 15.02.22 um 00:21 schrieb Luben Tuikov:
> Add the "harvest" field to the IP attributes in
> the IP discovery sysfs visualization, as this
> field is present in the binary data.
>
> Cc: Alex Deucher <Alexander.Deucher@amd.com>
> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index c8dbdb78988ce0..0496d369504641 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -393,6 +393,7 @@ struct ip_hw_instance {
>   	int hw_id;
>   	u8  num_instance;
>   	u8  major, minor, revision;
> +	u8  harvest;
>   
>   	int num_base_addresses;
>   	u32 base_addr[];
> @@ -440,6 +441,11 @@ static ssize_t revision_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>   	return sysfs_emit(buf, "%d\n", ip_hw_instance->revision);
>   }
>   
> +static ssize_t harvest_show(struct ip_hw_instance *ip_hw_instance, char *buf)
> +{
> +	return sysfs_emit(buf, "0x%01X\n", ip_hw_instance->harvest);
> +}
> +

Maybe add a comment regarding Guchun's concern. With that done feel free 
to add a Reviewed-by: Christian König <christian.koenig@amd.com>

Regards,
Christian.

>   static ssize_t num_base_addresses_show(struct ip_hw_instance *ip_hw_instance, char *buf)
>   {
>   	return sysfs_emit(buf, "%d\n", ip_hw_instance->num_base_addresses);
> @@ -471,6 +477,7 @@ static struct ip_hw_instance_attr ip_hw_attr[] = {
>   	__ATTR_RO(major),
>   	__ATTR_RO(minor),
>   	__ATTR_RO(revision),
> +	__ATTR_RO(harvest),
>   	__ATTR_RO(num_base_addresses),
>   	__ATTR_RO(base_addr),
>   };
> @@ -708,6 +715,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev,
>   			ip_hw_instance->major = ip->major;
>   			ip_hw_instance->minor = ip->minor;
>   			ip_hw_instance->revision = ip->revision;
> +			ip_hw_instance->harvest = ip->harvest;
>   			ip_hw_instance->num_base_addresses = ip->num_base_address;
>   
>   			for (kk = 0; kk < ip_hw_instance->num_base_addresses; kk++)
>
> base-commit: d8604f1d237a145db48bae4ea60b85a5875df307

Re: [PATCH] drm/amdgpu: Add "harvest" to IP discovery sysfs

[Alex Deucher]

yeah, I think it's better to keep the additional data there.  While
there are some faulty vbioses out there, I think the goal is to have
it correct in the long term, so I think it is fine to just expose what
is in the ip discovery table as is for now.  Maybe add a comment in
the code as Christian suggested.  With that:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

On Tue, Feb 15, 2022 at 11:21 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 15.02.22 um 00:21 schrieb Luben Tuikov:
> > Add the "harvest" field to the IP attributes in
> > the IP discovery sysfs visualization, as this
> > field is present in the binary data.
> >
> > Cc: Alex Deucher <Alexander.Deucher@amd.com>
> > Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > index c8dbdb78988ce0..0496d369504641 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > @@ -393,6 +393,7 @@ struct ip_hw_instance {
> >       int hw_id;
> >       u8  num_instance;
> >       u8  major, minor, revision;
> > +     u8  harvest;
> >
> >       int num_base_addresses;
> >       u32 base_addr[];
> > @@ -440,6 +441,11 @@ static ssize_t revision_show(struct ip_hw_instance *ip_hw_instance, char *buf)
> >       return sysfs_emit(buf, "%d\n", ip_hw_instance->revision);
> >   }
> >
> > +static ssize_t harvest_show(struct ip_hw_instance *ip_hw_instance, char *buf)
> > +{
> > +     return sysfs_emit(buf, "0x%01X\n", ip_hw_instance->harvest);
> > +}
> > +
>
> Maybe add a comment regarding Guchun's concern. With that done feel free
> to add a Reviewed-by: Christian König <christian.koenig@amd.com>
>
> Regards,
> Christian.
>
> >   static ssize_t num_base_addresses_show(struct ip_hw_instance *ip_hw_instance, char *buf)
> >   {
> >       return sysfs_emit(buf, "%d\n", ip_hw_instance->num_base_addresses);
> > @@ -471,6 +477,7 @@ static struct ip_hw_instance_attr ip_hw_attr[] = {
> >       __ATTR_RO(major),
> >       __ATTR_RO(minor),
> >       __ATTR_RO(revision),
> > +     __ATTR_RO(harvest),
> >       __ATTR_RO(num_base_addresses),
> >       __ATTR_RO(base_addr),
> >   };
> > @@ -708,6 +715,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev,
> >                       ip_hw_instance->major = ip->major;
> >                       ip_hw_instance->minor = ip->minor;
> >                       ip_hw_instance->revision = ip->revision;
> > +                     ip_hw_instance->harvest = ip->harvest;
> >                       ip_hw_instance->num_base_addresses = ip->num_base_address;
> >
> >                       for (kk = 0; kk < ip_hw_instance->num_base_addresses; kk++)
> >
> > base-commit: d8604f1d237a145db48bae4ea60b85a5875df307
>