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

[Lazar, Lijo]

On 12/5/2024 9:36 AM, Mario Limonciello wrote:
> 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?
> 

Yes, this will help.

Thanks,
Lijo

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