| Message ID | 20240413114938.740631-1-andyshrk@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panthor: Add defer probe for firmware load | expand |
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)
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.
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
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.
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)