Re: [PATCH] drm/panthor: Add defer probe for firmware load

[Steven Price <steven.price@arm.com>]

Hi Javier,

On 25/04/2024 10:22, Javier Martinez Canillas wrote:
> Steven Price <steven.price@arm.com> writes:
> 
> Hello Steven,
> 
>> On 13/04/2024 12:49, Andy Yan wrote:
>>> From: Andy Yan <andy.yan@rock-chips.com>
>>>
>>> The firmware in the rootfs will not be accessible until we
>>> are in the SYSTEM_RUNNING state, so return EPROBE_DEFER until
>>> that point.
>>> This let the driver can load firmware when it is builtin.
>>
>> The usual solution is that the firmware should be placed in the
>> initrd/initramfs if the module is included there (or built-in). The same
>> issue was brought up regarding the powervr driver:
>>
>> https://lore.kernel.org/dri-devel/20240109120604.603700-1-javierm@redhat.com/T/
>>
>> I'm not sure if that ever actually reached a conclusion though. The
>> question was deferred to Greg KH but I didn't see Greg actually getting
>> involved in the thread.
>>
> 
> Correct, there was not conclusion reached in that thread.

So I think we need a conclusion before we start applying point fixes to
individual drivers.

>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>> ---
>>>
>>>  drivers/gpu/drm/panthor/panthor_fw.c | 11 ++++++++++-
>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
>>> index 33c87a59834e..25e375f8333c 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_fw.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
>>> @@ -1336,8 +1336,17 @@ int panthor_fw_init(struct panthor_device *ptdev)
>>>  	}
>>>  
>>>  	ret = panthor_fw_load(ptdev);
>>> -	if (ret)
>>> +	if (ret) {
>>> +		/*
>>> +		 * The firmware in the rootfs will not be accessible until we
>>> +		 * are in the SYSTEM_RUNNING state, so return EPROBE_DEFER until
>>> +		 * that point.
>>> +		 */
>>> +		if (system_state < SYSTEM_RUNNING)
>>
>> This should really only be in the case where ret == -ENOENT - any other
>> error and the firmware is apparently present but broken in some way, so
>> there's no point deferring.
>>
>> I also suspect we'd need some change in panthor_fw_load() to quieten
>> error messages if we're going to defer on this, in which case it might
>> make more sense to move this logic into that function.
>>
> 
> In the thread you referenced I suggested to add that logic in request_firmware()
> (or add a new request_firmware_defer() helper function) that changes the request
> firmare behaviour to return -EPROBE_DEFER instead of -ENOENT.

That would seem like a good feature if it's agreed that deferring on
request_firmware is a good idea.

> Since as you mentioned, this isn't specific to panthor and an issue that I also
> faced with the powervr driver.

I'm not in any way against the idea of deferring the probe until the
firmware is around - indeed it seems like a very sensible idea in many
respects. But I don't want panthor to be 'special' in this way.

If the consensus is that the firmware should live with the module (i.e.
either both in the initramfs or both in the rootfs) then the code is
fine as it is. That seemed to be the view of Sima in that thread and
seems reasonable to me - why put the .ko in the initrd if you can't
actually use it until the rootfs comes along?

The other option is the user fallback mechanisms for firmware loading
which can wait arbitrarily for the firmware to become available. But
that isn't exactly popular these days.

Steve