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

[Javier Martinez Canillas <javierm@redhat.com>]

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.

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

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat