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

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

[Andy Yan]

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.

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)
+			ret = -EPROBE_DEFER;
+
 		goto err_unplug_fw;
+	}
 
 	ret = panthor_vm_active(fw->vm);
 	if (ret)
-- 
2.34.1

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

[Steven Price]

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.

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

Steve

> +			ret = -EPROBE_DEFER;
> +
>  		goto err_unplug_fw;
> +	}
>  
>  	ret = panthor_vm_active(fw->vm);
>  	if (ret)

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

[Javier Martinez Canillas]

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

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

[Steven Price]

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

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

[Javier Martinez Canillas]

Steven Price <steven.price@arm.com> writes:

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

Agreed.

[...]

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

Yeah. I didn't attempt to type that patch because didn't get an answer
from Greg and didn't want to spent the time writing and testing a patch
to just be nacked.

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

That's indeed a sensible position for me as well and is what I answered to
the user who reported the powervr issue.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat