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

[Lazar, Lijo]

On 6/16/2025 10:35 PM, Mario Limonciello wrote:
> 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;
> +               }
> 

Yes, looks fine.

Thanks,
Lijo

>                 amdgpu_discovery_harvest_ip(adev);
>                 amdgpu_discovery_get_gfx_info(adev);
> 
> 
> 
>>
>> Thanks,
>> Lijo
>>
>>>           return r;
>>>       }
>>>   
>>
>