[PATCH] drm/amd: Add the capability to mark certain firmware as "required"

[Mario Limonciello]

On 12/4/2024 21:59, Lazar, Lijo wrote:
> 
> 
> On 12/4/2024 10:15 PM, Alex Deucher wrote:
>> On Wed, Dec 4, 2024 at 11:18 AM Lazar, Lijo <lijo.lazar at amd.com> wrote:
>>>
>>>
>>>
>>> On 12/4/2024 9:30 PM, Alex Deucher wrote:
>>>> On Wed, Dec 4, 2024 at 10:56 AM Lazar, Lijo <lijo.lazar at amd.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 12/4/2024 7:51 PM, Alex Deucher wrote:
>>>>>> On Wed, Dec 4, 2024 at 12:47 AM Lazar, Lijo <lijo.lazar at amd.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 12/4/2024 10:44 AM, Mario Limonciello wrote:
>>>>>>>>
>>>>>>>>>> +enum amdgpu_ucode_required {
>>>>>>>>>> +    AMDGPU_UCODE_NOT_REQUIRED,
>>>>>>>>>> +    AMDGPU_UCODE_REQUIRED,
>>>>>>>>>
>>>>>>>>> Couldn't this be handled in another API instead of having to flag every
>>>>>>>>> load? By default, every ucode is required and if optional may be skipped
>>>>>>>>> with amdgpu_ucode_request_optional() API?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I guess this would be a smaller patch, but 6 eggs one hand, half dozen
>>>>>>>> in the other?
>>>>>>>>
>>>>>>>
>>>>>>> I thought only ISP and gpu_info (no longer there for newer SOCs) fall
>>>>>>> into the optional ones so far. The usage is rare, similar to the
>>>>>>> nowarn() API usage.
>>>>>>>
>>>>>>> Also, as far as I know, the cap microcode is a must whenever used. That
>>>>>>> is not optional.
>>>>>>>
>>>>>>
>>>>>> The cap firmware is definitely optional.  Some customers use it, some don't.
>>>>>>
>>>>>
>>>>> I thought optional is something that can be ignored even if FW is not
>>>>> found and then driver load proceeds.
>>>>>
>>>>> What is the expected driver action if we classify cap firmware as
>>>>> optional and then it fails on a customer system that expects it?
>>>>
>>>> I guess if the customer expects it, they can make sure it's there.
>>>
>>> I don't think customer really can do that without any diagnostic message
>>> from the driver. Driver has to show the right message. If it passes that
>>> silently and fails at some other point, it could be a totally different
>>> signature.
>>
>> yeah, I haven't seen any bug reports about the cap firmware so the
>> current behavior seems to be fine.
>>
> 
> In this case, need to have a info level message when a firmware
> classified as optional is not found. As it is only during driver load, I
> don't think that message will be an annoyance. On the other hand, it
> gives useful info if it runs into trouble at a later point during load.

This series stemmed from concerns being raised about the WARN level 
message from the core but there is no way to message to the user from 
the core it's optional.

Do you think something like:

drm_info(adev->dev, "Optional firmware %s not found\n", name);

In the failure path for the optional is fine?

> 
> Thanks,
> Lijo
> 
>> Alex
>>
>>>
>>>> I'm not sure how you can have both without it being optional.  For
>>>> customers that don't use it, requiring it would break them if it
>>>> wasn't present.
>>>>
>>>
>>> It's working so far. Having all is better as long as loading that is
>>> harmless.
>>>
>>> Thanks,
>>> Lijo
>>>
>>>> Alex
>>>>
>>>>>
>>>>> Thanks,
>>>>> Lijo
>>>>>
>>>>>
>>>>>> Alex
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> Lijo
>>>>>>>
>>>>>>>> Alex - what's your take?
>>>>>>>
>>>>>
>>>
>