[PATCH] drm/amd: Adjust output for discovery error handling

[Mario Limonciello]

On 6/13/25 10:27 PM, Lazar, Lijo wrote:
> 
> 
> On 6/14/2025 3:11 AM, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello at amd.com>
>>
>> commit 017fbb6690c22 ("drm/amdgpu/discovery: check ip_discovery fw file
>> available") added support for reading an amdgpu IP discovery bin file
>> for some specific products. If it's not found then it will fallback to
>> hardcoded values. However if it's not found there is also a lot of noise
>> about missing files and errors.
>>
>> Adjust the error handling to decrease most messages to DEBUG and to show
>> at most one message to a user about the missing file at INFO level.
>>
>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4312
>> Fixes: 017fbb6690c22 ("drm/amdgpu/discovery: check ip_discovery fw file available")
>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 17 ++++++-----------
>>   1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>> index a0e9bf9b27108..8e4526a8c2600 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>> @@ -321,10 +321,9 @@ static int amdgpu_discovery_read_binary_from_file(struct amdgpu_device *adev,
>>   	const struct firmware *fw;
>>   	int r;
>>   
>> -	r = request_firmware(&fw, fw_name, adev->dev);
>> +	r = firmware_request_nowarn(&fw, fw_name, adev->dev);
>>   	if (r) {
>> -		dev_err(adev->dev, "can't load firmware \"%s\"\n",
>> -			fw_name);
>> +		drm_info(&adev->ddev, "Optional firmware \"%s\" was not found\n", fw_name);
>>   		return r;
>>   	}
>>   
>> @@ -459,16 +458,12 @@ static int amdgpu_discovery_init(struct amdgpu_device *adev)
>>   	/* Read from file if it is the preferred option */
>>   	fw_name = amdgpu_discovery_get_fw_name(adev);
>>   	if (fw_name != NULL) {
>> -		dev_info(adev->dev, "use ip discovery information from file");
>> +		drm_dbg(&adev->ddev, "use ip discovery information from file");
>>   		r = amdgpu_discovery_read_binary_from_file(adev, adev->mman.discovery_bin, fw_name);
>> -
>> -		if (r) {
>> -			dev_err(adev->dev, "failed to read ip discovery binary from file\n");
>> -			r = -EINVAL;
>> +		if (r)
>>   			goto out;
>> -		}
>> -
>>   	} else {
>> +		drm_dbg(&adev->ddev, "use ip discovery information from memory");
>>   		r = amdgpu_discovery_read_binary_from_mem(
>>   			adev, adev->mman.discovery_bin);
>>   		if (r)
>> @@ -1339,7 +1334,7 @@ static int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
>>   
>>   	r = amdgpu_discovery_init(adev);
>>   	if (r) {
>> -		DRM_ERROR("amdgpu_discovery_init failed\n");
>> +		drm_warn(&adev->ddev, "%s failed: %d\n", __func__, r);
> 
> This indeed is an error. Rest of the changes are fine.

How about pushing this down into amdgpu_discovery_set_ip_blocks()?
I was thinking we can put it in the default case only so that the 
fallback path doesn't make a lot of noise for vega/raven/arcturus/aldebaran.

Something like this:

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index 3877b43d9f863..d5bd105de1b31 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -1333,10 +1333,8 @@ static int amdgpu_discovery_reg_base_init(struct 
amdgpu_device *adev)
         int r;

         r = amdgpu_discovery_init(adev);
-       if (r) {
-               DRM_ERROR("amdgpu_discovery_init failed\n");
+       if (r)
                 return r;
-       }

         wafl_ver = 0;
         adev->gfx.xcc_mask = 0;
@@ -2574,8 +2572,10 @@ int amdgpu_discovery_set_ip_blocks(struct 
amdgpu_device *adev)
                 break;
         default:
                 r = amdgpu_discovery_reg_base_init(adev);
-               if (r)
-                       return -EINVAL;
+               if (r) {
+                       drm_err(&adev->ddev, "discovery failed: %d\n", ret);
+                       return r;
+               }

                 amdgpu_discovery_harvest_ip(adev);
                 amdgpu_discovery_get_gfx_info(adev);



> 
> Thanks,
> Lijo
> 
>>   		return r;
>>   	}
>>   
>