| Message ID | 9b7a9e9b88ad8c7489ee1b4c70b8751eeb5cf6f9.1720049413.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/rockchip: cdn-dp: Remove redundant workarounds for firmware loading | expand |
Hi Dragan， At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> wrote: >After the additional firmware-related module information was introduced by >the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add MODULE_FIRMWARE >macro"), there's no longer need for the firmware-loading workarounds whose >sole purpose was to prevent the missing firmware blob in an initial ramdisk >from causing driver initialization to fail. Thus, delete the workarounds, >which removes a sizable chunk of redundant code. What would happen if there was no ramdisk? And the firmware is in rootfs ？ For example： A buildroot based tiny embedded system。 > >Various utilities used by Linux distributions to generate initial ramdisks >need to obey the firmware-related module information, so we can rely on the >firmware blob being present in the generated initial ramdisks. > >Signed-off-by: Dragan Simic <dsimic@manjaro.org> >--- > drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 +++++--------------------- > 1 file changed, 10 insertions(+), 43 deletions(-) > >diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c >index bd7aa891b839..e1a7c6a1172b 100644 >--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c >+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c >@@ -44,9 +44,9 @@ static inline struct cdn_dp_device *encoder_to_dp(struct drm_encoder *encoder) > #define DPTX_HPD_DEL (2 << 12) > #define DPTX_HPD_SEL_MASK (3 << 28) > >-#define CDN_FW_TIMEOUT_MS (64 * 1000) > #define CDN_DPCD_TIMEOUT_MS 5000 > #define CDN_DP_FIRMWARE "rockchip/dptx.bin" >+ > MODULE_FIRMWARE(CDN_DP_FIRMWARE); > > struct cdn_dp_data { >@@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct cdn_dp_device *dp, > return PTR_ERR_OR_ZERO(dp->audio_pdev); > } > >-static int cdn_dp_request_firmware(struct cdn_dp_device *dp) >-{ >- int ret; >- unsigned long timeout = jiffies + msecs_to_jiffies(CDN_FW_TIMEOUT_MS); >- unsigned long sleep = 1000; >- >- WARN_ON(!mutex_is_locked(&dp->lock)); >- >- if (dp->fw_loaded) >- return 0; >- >- /* Drop the lock before getting the firmware to avoid blocking boot */ >- mutex_unlock(&dp->lock); >- >- while (time_before(jiffies, timeout)) { >- ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >- if (ret == -ENOENT) { >- msleep(sleep); >- sleep *= 2; >- continue; >- } else if (ret) { >- DRM_DEV_ERROR(dp->dev, >- "failed to request firmware: %d\n", ret); >- goto out; >- } >- >- dp->fw_loaded = true; >- ret = 0; >- goto out; >- } >- >- DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n"); >- ret = -ETIMEDOUT; >-out: >- mutex_lock(&dp->lock); >- return ret; >-} >- > static void cdn_dp_pd_event_work(struct work_struct *work) > { > struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device, > event_work); > struct drm_connector *connector = &dp->connector; > enum drm_connector_status old_status; >- > int ret; > > mutex_lock(&dp->lock); > > if (dp->suspended) > goto out; > >- ret = cdn_dp_request_firmware(dp); >- if (ret) >- goto out; >+ if (!dp->fw_loaded) { >+ ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >+ if (ret) { >+ DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", ret); >+ goto out; >+ } >+ >+ dp->fw_loaded = true; >+ } > > dp->connected = true; >
Hello Andy, On 2024-07-04 04:10, Andy Yan wrote: > At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> wrote: >> After the additional firmware-related module information was >> introduced by >> the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add >> MODULE_FIRMWARE >> macro"), there's no longer need for the firmware-loading workarounds >> whose >> sole purpose was to prevent the missing firmware blob in an initial >> ramdisk >> from causing driver initialization to fail. Thus, delete the >> workarounds, >> which removes a sizable chunk of redundant code. > > What would happen if there was no ramdisk? And the firmware is in > rootfs ？ > > For example： A buildroot based tiny embedded system。 Good point, let me explain, please. In general, if a driver is built into the kernel, there should also be an initial ramdisk that contains the related firmware blobs, because it's unknown is the root filesystem available when the driver is probed. If a driver is built as a module and there's no initial ramdisk, having the related firmware blobs on the root filesystem should be fine, because the firmware blobs and the kernel module become available at the same time, through the root filesystem. [1] Another option for a driver built statically into the kernel, when there's no initial ramdisk, is to build the required firmware blobs into the kernel image. [2] Of course, that's feasible only when a kernel image is built specificially for some device, because otherwise it would become too large because of too many drivers and their firmware blobs becoming included, but that seems to fit the Buildroot-based example. To sum it up, mechanisms already exist in the kernel for various scenarios when it comes to loading firmware blobs. Even if the deleted workaround attempts to solve some issue specific to some environment, that isn't the right place or the right way for solving any issues of that kind. While preparing this patch, I even tried to find another kernel driver that also implements some similar workarounds for firmware loading, to justify the existence of such workarounds and to possibly move them into the kernel's firmware-loading interface. Alas, I was unable to find such workarounds in other drivers, which solidified my reasoning behind classifying the removed code as out-of-place and redundant. [1] https://www.kernel.org/doc/Documentation/driver-api/firmware/direct-fs-lookup.rst [2] https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst >> Various utilities used by Linux distributions to generate initial >> ramdisks >> need to obey the firmware-related module information, so we can rely >> on the >> firmware blob being present in the generated initial ramdisks. >> >> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >> --- >> drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 +++++--------------------- >> 1 file changed, 10 insertions(+), 43 deletions(-) >> >> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c >> b/drivers/gpu/drm/rockchip/cdn-dp-core.c >> index bd7aa891b839..e1a7c6a1172b 100644 >> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c >> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c >> @@ -44,9 +44,9 @@ static inline struct cdn_dp_device >> *encoder_to_dp(struct drm_encoder *encoder) >> #define DPTX_HPD_DEL (2 << 12) >> #define DPTX_HPD_SEL_MASK (3 << 28) >> >> -#define CDN_FW_TIMEOUT_MS (64 * 1000) >> #define CDN_DPCD_TIMEOUT_MS 5000 >> #define CDN_DP_FIRMWARE "rockchip/dptx.bin" >> + >> MODULE_FIRMWARE(CDN_DP_FIRMWARE); >> >> struct cdn_dp_data { >> @@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct >> cdn_dp_device *dp, >> return PTR_ERR_OR_ZERO(dp->audio_pdev); >> } >> >> -static int cdn_dp_request_firmware(struct cdn_dp_device *dp) >> -{ >> - int ret; >> - unsigned long timeout = jiffies + >> msecs_to_jiffies(CDN_FW_TIMEOUT_MS); >> - unsigned long sleep = 1000; >> - >> - WARN_ON(!mutex_is_locked(&dp->lock)); >> - >> - if (dp->fw_loaded) >> - return 0; >> - >> - /* Drop the lock before getting the firmware to avoid blocking boot >> */ >> - mutex_unlock(&dp->lock); >> - >> - while (time_before(jiffies, timeout)) { >> - ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >> - if (ret == -ENOENT) { >> - msleep(sleep); >> - sleep *= 2; >> - continue; >> - } else if (ret) { >> - DRM_DEV_ERROR(dp->dev, >> - "failed to request firmware: %d\n", ret); >> - goto out; >> - } >> - >> - dp->fw_loaded = true; >> - ret = 0; >> - goto out; >> - } >> - >> - DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n"); >> - ret = -ETIMEDOUT; >> -out: >> - mutex_lock(&dp->lock); >> - return ret; >> -} >> - >> static void cdn_dp_pd_event_work(struct work_struct *work) >> { >> struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device, >> event_work); >> struct drm_connector *connector = &dp->connector; >> enum drm_connector_status old_status; >> - >> int ret; >> >> mutex_lock(&dp->lock); >> >> if (dp->suspended) >> goto out; >> >> - ret = cdn_dp_request_firmware(dp); >> - if (ret) >> - goto out; >> + if (!dp->fw_loaded) { >> + ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >> + if (ret) { >> + DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", ret); >> + goto out; >> + } >> + >> + dp->fw_loaded = true; >> + } >> >> dp->connected = true; >>
Hi Dragan， At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@manjaro.org> wrote: >Hello Andy, > >On 2024-07-04 04:10, Andy Yan wrote: >> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> wrote: >>> After the additional firmware-related module information was >>> introduced by >>> the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add >>> MODULE_FIRMWARE >>> macro"), there's no longer need for the firmware-loading workarounds >>> whose >>> sole purpose was to prevent the missing firmware blob in an initial >>> ramdisk >>> from causing driver initialization to fail. Thus, delete the >>> workarounds, >>> which removes a sizable chunk of redundant code. >> >> What would happen if there was no ramdisk? And the firmware is in >> rootfs ？ >> >> For example： A buildroot based tiny embedded system。 > >Good point, let me explain, please. > >In general, if a driver is built into the kernel, there should also be >an initial ramdisk that contains the related firmware blobs, because >it's >unknown is the root filesystem available when the driver is probed. If >a driver is built as a module and there's no initial ramdisk, having >the related firmware blobs on the root filesystem should be fine, >because >the firmware blobs and the kernel module become available at the same >time, through the root filesystem. [1] > >Another option for a driver built statically into the kernel, when >there's >no initial ramdisk, is to build the required firmware blobs into the >kernel >image. [2] Of course, that's feasible only when a kernel image is built >specificially for some device, because otherwise it would become too >large >because of too many drivers and their firmware blobs becoming included, >but that seems to fit the Buildroot-based example. > >To sum it up, mechanisms already exist in the kernel for various >scenarios >when it comes to loading firmware blobs. Even if the deleted workaround >attempts to solve some issue specific to some environment, that isn't >the >right place or the right way for solving any issues of that kind. > >While preparing this patch, I even tried to find another kernel driver >that >also implements some similar workarounds for firmware loading, to >justify >the existence of such workarounds and to possibly move them into the >kernel's >firmware-loading interface. Alas, I was unable to find such workarounds >in >other drivers, which solidified my reasoning behind classifying the >removed >code as out-of-place and redundant. For some tiny embedded system，there is no such ramdisk，for example： a buildroot based rootfs，the buildroot only generate rootfs。 And FYI， there are mainline drivers try to fix such issue by defer_probe，for example： smc_abc[0] There are also some other similar scenario in gpu driver{1}[2] [0]https://elixir.bootlin.com/linux/latest/source/drivers/tee/optee/smc_abi.c#L1518 [1]https://patchwork.kernel.org/project/dri-devel/patch/20240109120604.603700-1-javierm@redhat.com/ [2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@minerva.mail-host-address-is-not-set/T/ > >[1] >https://www.kernel.org/doc/Documentation/driver-api/firmware/direct-fs-lookup.rst >[2] >https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst > >>> Various utilities used by Linux distributions to generate initial >>> ramdisks >>> need to obey the firmware-related module information, so we can rely >>> on the >>> firmware blob being present in the generated initial ramdisks. >>> >>> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >>> --- >>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 +++++--------------------- >>> 1 file changed, 10 insertions(+), 43 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>> b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>> index bd7aa891b839..e1a7c6a1172b 100644 >>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>> @@ -44,9 +44,9 @@ static inline struct cdn_dp_device >>> *encoder_to_dp(struct drm_encoder *encoder) >>> #define DPTX_HPD_DEL (2 << 12) >>> #define DPTX_HPD_SEL_MASK (3 << 28) >>> >>> -#define CDN_FW_TIMEOUT_MS (64 * 1000) >>> #define CDN_DPCD_TIMEOUT_MS 5000 >>> #define CDN_DP_FIRMWARE "rockchip/dptx.bin" >>> + >>> MODULE_FIRMWARE(CDN_DP_FIRMWARE); >>> >>> struct cdn_dp_data { >>> @@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct >>> cdn_dp_device *dp, >>> return PTR_ERR_OR_ZERO(dp->audio_pdev); >>> } >>> >>> -static int cdn_dp_request_firmware(struct cdn_dp_device *dp) >>> -{ >>> - int ret; >>> - unsigned long timeout = jiffies + >>> msecs_to_jiffies(CDN_FW_TIMEOUT_MS); >>> - unsigned long sleep = 1000; >>> - >>> - WARN_ON(!mutex_is_locked(&dp->lock)); >>> - >>> - if (dp->fw_loaded) >>> - return 0; >>> - >>> - /* Drop the lock before getting the firmware to avoid blocking boot >>> */ >>> - mutex_unlock(&dp->lock); >>> - >>> - while (time_before(jiffies, timeout)) { >>> - ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>> - if (ret == -ENOENT) { >>> - msleep(sleep); >>> - sleep *= 2; >>> - continue; >>> - } else if (ret) { >>> - DRM_DEV_ERROR(dp->dev, >>> - "failed to request firmware: %d\n", ret); >>> - goto out; >>> - } >>> - >>> - dp->fw_loaded = true; >>> - ret = 0; >>> - goto out; >>> - } >>> - >>> - DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n"); >>> - ret = -ETIMEDOUT; >>> -out: >>> - mutex_lock(&dp->lock); >>> - return ret; >>> -} >>> - >>> static void cdn_dp_pd_event_work(struct work_struct *work) >>> { >>> struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device, >>> event_work); >>> struct drm_connector *connector = &dp->connector; >>> enum drm_connector_status old_status; >>> - >>> int ret; >>> >>> mutex_lock(&dp->lock); >>> >>> if (dp->suspended) >>> goto out; >>> >>> - ret = cdn_dp_request_firmware(dp); >>> - if (ret) >>> - goto out; >>> + if (!dp->fw_loaded) { >>> + ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>> + if (ret) { >>> + DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", ret); >>> + goto out; >>> + } >>> + >>> + dp->fw_loaded = true; >>> + } >>> >>> dp->connected = true; >>>
On Mon, Jul 08, 2024 at 03:46:16PM GMT, Andy Yan wrote: > > Hi Dragan， > At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@manjaro.org> wrote: > >Hello Andy, > > > >On 2024-07-04 04:10, Andy Yan wrote: > >> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> wrote: > >>> After the additional firmware-related module information was > >>> introduced by > >>> the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add > >>> MODULE_FIRMWARE > >>> macro"), there's no longer need for the firmware-loading workarounds > >>> whose > >>> sole purpose was to prevent the missing firmware blob in an initial > >>> ramdisk > >>> from causing driver initialization to fail. Thus, delete the > >>> workarounds, > >>> which removes a sizable chunk of redundant code. > >> > >> What would happen if there was no ramdisk? And the firmware is in > >> rootfs ？ > >> > >> For example： A buildroot based tiny embedded system。 > > > >Good point, let me explain, please. > > > >In general, if a driver is built into the kernel, there should also be > >an initial ramdisk that contains the related firmware blobs, because > >it's > >unknown is the root filesystem available when the driver is probed. If > >a driver is built as a module and there's no initial ramdisk, having > >the related firmware blobs on the root filesystem should be fine, > >because > >the firmware blobs and the kernel module become available at the same > >time, through the root filesystem. [1] > > > >Another option for a driver built statically into the kernel, when > >there's > >no initial ramdisk, is to build the required firmware blobs into the > >kernel > >image. [2] Of course, that's feasible only when a kernel image is built > >specificially for some device, because otherwise it would become too > >large > >because of too many drivers and their firmware blobs becoming included, > >but that seems to fit the Buildroot-based example. > > > >To sum it up, mechanisms already exist in the kernel for various > >scenarios > >when it comes to loading firmware blobs. Even if the deleted workaround > >attempts to solve some issue specific to some environment, that isn't > >the > >right place or the right way for solving any issues of that kind. > > > >While preparing this patch, I even tried to find another kernel driver > >that > >also implements some similar workarounds for firmware loading, to > >justify > >the existence of such workarounds and to possibly move them into the > >kernel's > >firmware-loading interface. Alas, I was unable to find such workarounds > >in > >other drivers, which solidified my reasoning behind classifying the > >removed > >code as out-of-place and redundant. > > For some tiny embedded system，there is no such ramdisk，for example： > a buildroot based rootfs，the buildroot only generate rootfs。 I'm not sure why you think ramdisks are an issue. Modules and firmwares work just the same with or without ramdisks, so Buildroot can work just fine too. Maxime
Hello Andy, On 2024-07-08 09:46, Andy Yan wrote: > At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@manjaro.org> wrote: >> On 2024-07-04 04:10, Andy Yan wrote: >>> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> wrote: >>>> After the additional firmware-related module information was >>>> introduced by >>>> the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add >>>> MODULE_FIRMWARE >>>> macro"), there's no longer need for the firmware-loading workarounds >>>> whose >>>> sole purpose was to prevent the missing firmware blob in an initial >>>> ramdisk >>>> from causing driver initialization to fail. Thus, delete the >>>> workarounds, >>>> which removes a sizable chunk of redundant code. >>> >>> What would happen if there was no ramdisk? And the firmware is in >>> rootfs ？ >>> >>> For example： A buildroot based tiny embedded system。 >> >> Good point, let me explain, please. >> >> In general, if a driver is built into the kernel, there should also be >> an initial ramdisk that contains the related firmware blobs, because >> it's >> unknown is the root filesystem available when the driver is probed. >> If >> a driver is built as a module and there's no initial ramdisk, having >> the related firmware blobs on the root filesystem should be fine, >> because >> the firmware blobs and the kernel module become available at the same >> time, through the root filesystem. [1] >> >> Another option for a driver built statically into the kernel, when >> there's >> no initial ramdisk, is to build the required firmware blobs into the >> kernel >> image. [2] Of course, that's feasible only when a kernel image is >> built >> specificially for some device, because otherwise it would become too >> large >> because of too many drivers and their firmware blobs becoming >> included, >> but that seems to fit the Buildroot-based example. >> >> To sum it up, mechanisms already exist in the kernel for various >> scenarios >> when it comes to loading firmware blobs. Even if the deleted >> workaround >> attempts to solve some issue specific to some environment, that isn't >> the >> right place or the right way for solving any issues of that kind. >> >> While preparing this patch, I even tried to find another kernel driver >> that >> also implements some similar workarounds for firmware loading, to >> justify >> the existence of such workarounds and to possibly move them into the >> kernel's >> firmware-loading interface. Alas, I was unable to find such >> workarounds >> in >> other drivers, which solidified my reasoning behind classifying the >> removed >> code as out-of-place and redundant. > > For some tiny embedded system，there is no such ramdisk，for example： > a buildroot based rootfs，the buildroot only generate rootfs。 > > And FYI， there are mainline drivers try to fix such issue by > defer_probe，for example： > smc_abc[0] > There are also some other similar scenario in gpu driver{1}[2] > > [0]https://elixir.bootlin.com/linux/latest/source/drivers/tee/optee/smc_abi.c#L1518 > [1]https://patchwork.kernel.org/project/dri-devel/patch/20240109120604.603700-1-javierm@redhat.com/ > [2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@minerva.mail-host-address-is-not-set/T/ Thanks for providing these examples. Before I continue thinking about the possible systemic solution, could you please clarify the way Buildroot builds the kernel and prepares the root filesystem? I'm not familiar with Buildroot, but it seems to me that it builds the drivers statically into the produced kernel image, while it places the related firmware blobs into the produced root filesystem. Am I right there? As I already wrote earlier, and as the above-linked discussions conclude, solving these issues doesn't belong to any specific driver. It should be resolved within the kernel's firmware loading mechanism instead, and no driver should be specific in that regard. >> [1] >> https://www.kernel.org/doc/Documentation/driver-api/firmware/direct-fs-lookup.rst >> [2] >> https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst >> >>>> Various utilities used by Linux distributions to generate initial >>>> ramdisks >>>> need to obey the firmware-related module information, so we can rely >>>> on the >>>> firmware blob being present in the generated initial ramdisks. >>>> >>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >>>> --- >>>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 >>>> +++++--------------------- >>>> 1 file changed, 10 insertions(+), 43 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>> index bd7aa891b839..e1a7c6a1172b 100644 >>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>> @@ -44,9 +44,9 @@ static inline struct cdn_dp_device >>>> *encoder_to_dp(struct drm_encoder *encoder) >>>> #define DPTX_HPD_DEL (2 << 12) >>>> #define DPTX_HPD_SEL_MASK (3 << 28) >>>> >>>> -#define CDN_FW_TIMEOUT_MS (64 * 1000) >>>> #define CDN_DPCD_TIMEOUT_MS 5000 >>>> #define CDN_DP_FIRMWARE "rockchip/dptx.bin" >>>> + >>>> MODULE_FIRMWARE(CDN_DP_FIRMWARE); >>>> >>>> struct cdn_dp_data { >>>> @@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct >>>> cdn_dp_device *dp, >>>> return PTR_ERR_OR_ZERO(dp->audio_pdev); >>>> } >>>> >>>> -static int cdn_dp_request_firmware(struct cdn_dp_device *dp) >>>> -{ >>>> - int ret; >>>> - unsigned long timeout = jiffies + >>>> msecs_to_jiffies(CDN_FW_TIMEOUT_MS); >>>> - unsigned long sleep = 1000; >>>> - >>>> - WARN_ON(!mutex_is_locked(&dp->lock)); >>>> - >>>> - if (dp->fw_loaded) >>>> - return 0; >>>> - >>>> - /* Drop the lock before getting the firmware to avoid blocking >>>> boot >>>> */ >>>> - mutex_unlock(&dp->lock); >>>> - >>>> - while (time_before(jiffies, timeout)) { >>>> - ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>>> - if (ret == -ENOENT) { >>>> - msleep(sleep); >>>> - sleep *= 2; >>>> - continue; >>>> - } else if (ret) { >>>> - DRM_DEV_ERROR(dp->dev, >>>> - "failed to request firmware: %d\n", ret); >>>> - goto out; >>>> - } >>>> - >>>> - dp->fw_loaded = true; >>>> - ret = 0; >>>> - goto out; >>>> - } >>>> - >>>> - DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n"); >>>> - ret = -ETIMEDOUT; >>>> -out: >>>> - mutex_lock(&dp->lock); >>>> - return ret; >>>> -} >>>> - >>>> static void cdn_dp_pd_event_work(struct work_struct *work) >>>> { >>>> struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device, >>>> event_work); >>>> struct drm_connector *connector = &dp->connector; >>>> enum drm_connector_status old_status; >>>> - >>>> int ret; >>>> >>>> mutex_lock(&dp->lock); >>>> >>>> if (dp->suspended) >>>> goto out; >>>> >>>> - ret = cdn_dp_request_firmware(dp); >>>> - if (ret) >>>> - goto out; >>>> + if (!dp->fw_loaded) { >>>> + ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>>> + if (ret) { >>>> + DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", ret); >>>> + goto out; >>>> + } >>>> + >>>> + dp->fw_loaded = true; >>>> + } >>>> >>>> dp->connected = true; >>>>
Hi Draqan, At 2024-07-09 16:17:06, "Dragan Simic" <dsimic@manjaro.org> wrote: >Hello Andy, > >On 2024-07-08 09:46, Andy Yan wrote: >> At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@manjaro.org> wrote: >>> On 2024-07-04 04:10, Andy Yan wrote: >>>> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> wrote: >>>>> After the additional firmware-related module information was >>>>> introduced by >>>>> the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add >>>>> MODULE_FIRMWARE >>>>> macro"), there's no longer need for the firmware-loading workarounds >>>>> whose >>>>> sole purpose was to prevent the missing firmware blob in an initial >>>>> ramdisk >>>>> from causing driver initialization to fail. Thus, delete the >>>>> workarounds, >>>>> which removes a sizable chunk of redundant code. >>>> >>>> What would happen if there was no ramdisk? And the firmware is in >>>> rootfs ？ >>>> >>>> For example： A buildroot based tiny embedded system。 >>> >>> Good point, let me explain, please. >>> >>> In general, if a driver is built into the kernel, there should also be >>> an initial ramdisk that contains the related firmware blobs, because >>> it's >>> unknown is the root filesystem available when the driver is probed. >>> If >>> a driver is built as a module and there's no initial ramdisk, having >>> the related firmware blobs on the root filesystem should be fine, >>> because >>> the firmware blobs and the kernel module become available at the same >>> time, through the root filesystem. [1] >>> >>> Another option for a driver built statically into the kernel, when >>> there's >>> no initial ramdisk, is to build the required firmware blobs into the >>> kernel >>> image. [2] Of course, that's feasible only when a kernel image is >>> built >>> specificially for some device, because otherwise it would become too >>> large >>> because of too many drivers and their firmware blobs becoming >>> included, >>> but that seems to fit the Buildroot-based example. >>> >>> To sum it up, mechanisms already exist in the kernel for various >>> scenarios >>> when it comes to loading firmware blobs. Even if the deleted >>> workaround >>> attempts to solve some issue specific to some environment, that isn't >>> the >>> right place or the right way for solving any issues of that kind. >>> >>> While preparing this patch, I even tried to find another kernel driver >>> that >>> also implements some similar workarounds for firmware loading, to >>> justify >>> the existence of such workarounds and to possibly move them into the >>> kernel's >>> firmware-loading interface. Alas, I was unable to find such >>> workarounds >>> in >>> other drivers, which solidified my reasoning behind classifying the >>> removed >>> code as out-of-place and redundant. >> >> For some tiny embedded system，there is no such ramdisk，for example： >> a buildroot based rootfs，the buildroot only generate rootfs。 >> >> And FYI， there are mainline drivers try to fix such issue by >> defer_probe，for example： >> smc_abc[0] >> There are also some other similar scenario in gpu driver{1}[2] >> >> [0]https://elixir.bootlin.com/linux/latest/source/drivers/tee/optee/smc_abi.c#L1518 >> [1]https://patchwork.kernel.org/project/dri-devel/patch/20240109120604.603700-1-javierm@redhat.com/ >> [2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@minerva.mail-host-address-is-not-set/T/ > >Thanks for providing these examples. > >Before I continue thinking about the possible systemic solution, >could you please clarify the way Buildroot builds the kernel and >prepares the root filesystem? I'm not familiar with Buildroot, >but it seems to me that it builds the drivers statically into the >produced kernel image, while it places the related firmware blobs >into the produced root filesystem. Am I right there? in practice we can chose build the drivers statically into the kernel， we can also build it as a module。 And in both case， the firmware blobs are put in rootfs。 If the drivers is built as a module， the module will also put in rootfs， so its fine。 But if a drivers is built into the kernel ，it maybe can't access the firmware blob before the rootfs is mounted. So we can see some drivers try to use DEFER_PROBE to fix this issue. > >As I already wrote earlier, and as the above-linked discussions >conclude, solving these issues doesn't belong to any specific driver. >It should be resolved within the kernel's firmware loading mechanism >instead, and no driver should be specific in that regard. IT would be good if it can be resolved within the kernel's firmware loading mechanism. > >>> [1] >>> https://www.kernel.org/doc/Documentation/driver-api/firmware/direct-fs-lookup.rst >>> [2] >>> https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst >>> >>>>> Various utilities used by Linux distributions to generate initial >>>>> ramdisks >>>>> need to obey the firmware-related module information, so we can rely >>>>> on the >>>>> firmware blob being present in the generated initial ramdisks. >>>>> >>>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >>>>> --- >>>>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 >>>>> +++++--------------------- >>>>> 1 file changed, 10 insertions(+), 43 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>> index bd7aa891b839..e1a7c6a1172b 100644 >>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>> @@ -44,9 +44,9 @@ static inline struct cdn_dp_device >>>>> *encoder_to_dp(struct drm_encoder *encoder) >>>>> #define DPTX_HPD_DEL (2 << 12) >>>>> #define DPTX_HPD_SEL_MASK (3 << 28) >>>>> >>>>> -#define CDN_FW_TIMEOUT_MS (64 * 1000) >>>>> #define CDN_DPCD_TIMEOUT_MS 5000 >>>>> #define CDN_DP_FIRMWARE "rockchip/dptx.bin" >>>>> + >>>>> MODULE_FIRMWARE(CDN_DP_FIRMWARE); >>>>> >>>>> struct cdn_dp_data { >>>>> @@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct >>>>> cdn_dp_device *dp, >>>>> return PTR_ERR_OR_ZERO(dp->audio_pdev); >>>>> } >>>>> >>>>> -static int cdn_dp_request_firmware(struct cdn_dp_device *dp) >>>>> -{ >>>>> - int ret; >>>>> - unsigned long timeout = jiffies + >>>>> msecs_to_jiffies(CDN_FW_TIMEOUT_MS); >>>>> - unsigned long sleep = 1000; >>>>> - >>>>> - WARN_ON(!mutex_is_locked(&dp->lock)); >>>>> - >>>>> - if (dp->fw_loaded) >>>>> - return 0; >>>>> - >>>>> - /* Drop the lock before getting the firmware to avoid blocking >>>>> boot >>>>> */ >>>>> - mutex_unlock(&dp->lock); >>>>> - >>>>> - while (time_before(jiffies, timeout)) { >>>>> - ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>>>> - if (ret == -ENOENT) { >>>>> - msleep(sleep); >>>>> - sleep *= 2; >>>>> - continue; >>>>> - } else if (ret) { >>>>> - DRM_DEV_ERROR(dp->dev, >>>>> - "failed to request firmware: %d\n", ret); >>>>> - goto out; >>>>> - } >>>>> - >>>>> - dp->fw_loaded = true; >>>>> - ret = 0; >>>>> - goto out; >>>>> - } >>>>> - >>>>> - DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n"); >>>>> - ret = -ETIMEDOUT; >>>>> -out: >>>>> - mutex_lock(&dp->lock); >>>>> - return ret; >>>>> -} >>>>> - >>>>> static void cdn_dp_pd_event_work(struct work_struct *work) >>>>> { >>>>> struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device, >>>>> event_work); >>>>> struct drm_connector *connector = &dp->connector; >>>>> enum drm_connector_status old_status; >>>>> - >>>>> int ret; >>>>> >>>>> mutex_lock(&dp->lock); >>>>> >>>>> if (dp->suspended) >>>>> goto out; >>>>> >>>>> - ret = cdn_dp_request_firmware(dp); >>>>> - if (ret) >>>>> - goto out; >>>>> + if (!dp->fw_loaded) { >>>>> + ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>>>> + if (ret) { >>>>> + DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", ret); >>>>> + goto out; >>>>> + } >>>>> + >>>>> + dp->fw_loaded = true; >>>>> + } >>>>> >>>>> dp->connected = true; >>>>>
On 2024-07-09 11:10, Andy Yan wrote: > At 2024-07-09 16:17:06, "Dragan Simic" <dsimic@manjaro.org> wrote: >> On 2024-07-08 09:46, Andy Yan wrote: >>> At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@manjaro.org> wrote: >>>> On 2024-07-04 04:10, Andy Yan wrote: >>>>> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> wrote: >>>>>> After the additional firmware-related module information was >>>>>> introduced by >>>>>> the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add >>>>>> MODULE_FIRMWARE >>>>>> macro"), there's no longer need for the firmware-loading >>>>>> workarounds >>>>>> whose >>>>>> sole purpose was to prevent the missing firmware blob in an >>>>>> initial >>>>>> ramdisk >>>>>> from causing driver initialization to fail. Thus, delete the >>>>>> workarounds, >>>>>> which removes a sizable chunk of redundant code. >>>>> >>>>> What would happen if there was no ramdisk? And the firmware is in >>>>> rootfs ？ >>>>> >>>>> For example： A buildroot based tiny embedded system。 >>>> >>>> Good point, let me explain, please. >>>> >>>> In general, if a driver is built into the kernel, there should also >>>> be >>>> an initial ramdisk that contains the related firmware blobs, because >>>> it's >>>> unknown is the root filesystem available when the driver is probed. >>>> If >>>> a driver is built as a module and there's no initial ramdisk, having >>>> the related firmware blobs on the root filesystem should be fine, >>>> because >>>> the firmware blobs and the kernel module become available at the >>>> same >>>> time, through the root filesystem. [1] >>>> >>>> Another option for a driver built statically into the kernel, when >>>> there's >>>> no initial ramdisk, is to build the required firmware blobs into the >>>> kernel >>>> image. [2] Of course, that's feasible only when a kernel image is >>>> built >>>> specificially for some device, because otherwise it would become too >>>> large >>>> because of too many drivers and their firmware blobs becoming >>>> included, >>>> but that seems to fit the Buildroot-based example. >>>> >>>> To sum it up, mechanisms already exist in the kernel for various >>>> scenarios >>>> when it comes to loading firmware blobs. Even if the deleted >>>> workaround >>>> attempts to solve some issue specific to some environment, that >>>> isn't >>>> the >>>> right place or the right way for solving any issues of that kind. >>>> >>>> While preparing this patch, I even tried to find another kernel >>>> driver >>>> that >>>> also implements some similar workarounds for firmware loading, to >>>> justify >>>> the existence of such workarounds and to possibly move them into the >>>> kernel's >>>> firmware-loading interface. Alas, I was unable to find such >>>> workarounds >>>> in >>>> other drivers, which solidified my reasoning behind classifying the >>>> removed >>>> code as out-of-place and redundant. >>> >>> For some tiny embedded system，there is no such ramdisk，for example： >>> a buildroot based rootfs，the buildroot only generate rootfs。 >>> >>> And FYI， there are mainline drivers try to fix such issue by >>> defer_probe，for example： >>> smc_abc[0] >>> There are also some other similar scenario in gpu driver{1}[2] >>> >>> [0]https://elixir.bootlin.com/linux/latest/source/drivers/tee/optee/smc_abi.c#L1518 >>> [1]https://patchwork.kernel.org/project/dri-devel/patch/20240109120604.603700-1-javierm@redhat.com/ >>> [2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@minerva.mail-host-address-is-not-set/T/ >> >> Thanks for providing these examples. >> >> Before I continue thinking about the possible systemic solution, >> could you please clarify the way Buildroot builds the kernel and >> prepares the root filesystem? I'm not familiar with Buildroot, >> but it seems to me that it builds the drivers statically into the >> produced kernel image, while it places the related firmware blobs >> into the produced root filesystem. Am I right there? > > in practice we can chose build the drivers statically into the kernel， > we can also build it as a module。 > And in both case， the firmware blobs are put in rootfs。 > If the drivers is built as a module， the module will also put in > rootfs， > so its fine。 > But if a drivers is built into the kernel ，it maybe can't access the > firmware blob > before the rootfs is mounted. > So we can see some drivers try to use DEFER_PROBE to fix this issue. When Buildroot builds the drivers statically into the kernel image, can it also be told to build the required firmware blobs into the kernel image, for which there's already support in the kernel? Of course, that would be feasible if only a small number of firmware blobs would end up built into the kernel image, i.e. if the Buildroot build would be tailored for a specific board. Otherwise... >> As I already wrote earlier, and as the above-linked discussions >> conclude, solving these issues doesn't belong to any specific driver. >> It should be resolved within the kernel's firmware loading mechanism >> instead, and no driver should be specific in that regard. > > IT would be good if it can be resolved within the kernel's firmware > loading mechanism. ... we'll need this as a systemic solution. >>>> [1] >>>> https://www.kernel.org/doc/Documentation/driver-api/firmware/direct-fs-lookup.rst >>>> [2] >>>> https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst >>>> >>>>>> Various utilities used by Linux distributions to generate initial >>>>>> ramdisks >>>>>> need to obey the firmware-related module information, so we can >>>>>> rely >>>>>> on the >>>>>> firmware blob being present in the generated initial ramdisks. >>>>>> >>>>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >>>>>> --- >>>>>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 >>>>>> +++++--------------------- >>>>>> 1 file changed, 10 insertions(+), 43 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>> index bd7aa891b839..e1a7c6a1172b 100644 >>>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>> @@ -44,9 +44,9 @@ static inline struct cdn_dp_device >>>>>> *encoder_to_dp(struct drm_encoder *encoder) >>>>>> #define DPTX_HPD_DEL (2 << 12) >>>>>> #define DPTX_HPD_SEL_MASK (3 << 28) >>>>>> >>>>>> -#define CDN_FW_TIMEOUT_MS (64 * 1000) >>>>>> #define CDN_DPCD_TIMEOUT_MS 5000 >>>>>> #define CDN_DP_FIRMWARE "rockchip/dptx.bin" >>>>>> + >>>>>> MODULE_FIRMWARE(CDN_DP_FIRMWARE); >>>>>> >>>>>> struct cdn_dp_data { >>>>>> @@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct >>>>>> cdn_dp_device *dp, >>>>>> return PTR_ERR_OR_ZERO(dp->audio_pdev); >>>>>> } >>>>>> >>>>>> -static int cdn_dp_request_firmware(struct cdn_dp_device *dp) >>>>>> -{ >>>>>> - int ret; >>>>>> - unsigned long timeout = jiffies + >>>>>> msecs_to_jiffies(CDN_FW_TIMEOUT_MS); >>>>>> - unsigned long sleep = 1000; >>>>>> - >>>>>> - WARN_ON(!mutex_is_locked(&dp->lock)); >>>>>> - >>>>>> - if (dp->fw_loaded) >>>>>> - return 0; >>>>>> - >>>>>> - /* Drop the lock before getting the firmware to avoid blocking >>>>>> boot >>>>>> */ >>>>>> - mutex_unlock(&dp->lock); >>>>>> - >>>>>> - while (time_before(jiffies, timeout)) { >>>>>> - ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>>>>> - if (ret == -ENOENT) { >>>>>> - msleep(sleep); >>>>>> - sleep *= 2; >>>>>> - continue; >>>>>> - } else if (ret) { >>>>>> - DRM_DEV_ERROR(dp->dev, >>>>>> - "failed to request firmware: %d\n", ret); >>>>>> - goto out; >>>>>> - } >>>>>> - >>>>>> - dp->fw_loaded = true; >>>>>> - ret = 0; >>>>>> - goto out; >>>>>> - } >>>>>> - >>>>>> - DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n"); >>>>>> - ret = -ETIMEDOUT; >>>>>> -out: >>>>>> - mutex_lock(&dp->lock); >>>>>> - return ret; >>>>>> -} >>>>>> - >>>>>> static void cdn_dp_pd_event_work(struct work_struct *work) >>>>>> { >>>>>> struct cdn_dp_device *dp = container_of(work, struct >>>>>> cdn_dp_device, >>>>>> event_work); >>>>>> struct drm_connector *connector = &dp->connector; >>>>>> enum drm_connector_status old_status; >>>>>> - >>>>>> int ret; >>>>>> >>>>>> mutex_lock(&dp->lock); >>>>>> >>>>>> if (dp->suspended) >>>>>> goto out; >>>>>> >>>>>> - ret = cdn_dp_request_firmware(dp); >>>>>> - if (ret) >>>>>> - goto out; >>>>>> + if (!dp->fw_loaded) { >>>>>> + ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>>>>> + if (ret) { >>>>>> + DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", ret); >>>>>> + goto out; >>>>>> + } >>>>>> + >>>>>> + dp->fw_loaded = true; >>>>>> + } >>>>>> >>>>>> dp->connected = true; >>>>>>
Hi Dragan， At 2024-07-09 18:10:51, "Dragan Simic" <dsimic@manjaro.org> wrote: >On 2024-07-09 11:10, Andy Yan wrote: >> At 2024-07-09 16:17:06, "Dragan Simic" <dsimic@manjaro.org> wrote: >>> On 2024-07-08 09:46, Andy Yan wrote: >>>> At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@manjaro.org> wrote: >>>>> On 2024-07-04 04:10, Andy Yan wrote: >>>>>> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> wrote: >>>>>>> After the additional firmware-related module information was >>>>>>> introduced by >>>>>>> the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add >>>>>>> MODULE_FIRMWARE >>>>>>> macro"), there's no longer need for the firmware-loading >>>>>>> workarounds >>>>>>> whose >>>>>>> sole purpose was to prevent the missing firmware blob in an >>>>>>> initial >>>>>>> ramdisk >>>>>>> from causing driver initialization to fail. Thus, delete the >>>>>>> workarounds, >>>>>>> which removes a sizable chunk of redundant code. >>>>>> >>>>>> What would happen if there was no ramdisk? And the firmware is in >>>>>> rootfs ？ >>>>>> >>>>>> For example： A buildroot based tiny embedded system。 >>>>> >>>>> Good point, let me explain, please. >>>>> >>>>> In general, if a driver is built into the kernel, there should also >>>>> be >>>>> an initial ramdisk that contains the related firmware blobs, because >>>>> it's >>>>> unknown is the root filesystem available when the driver is probed. >>>>> If >>>>> a driver is built as a module and there's no initial ramdisk, having >>>>> the related firmware blobs on the root filesystem should be fine, >>>>> because >>>>> the firmware blobs and the kernel module become available at the >>>>> same >>>>> time, through the root filesystem. [1] >>>>> >>>>> Another option for a driver built statically into the kernel, when >>>>> there's >>>>> no initial ramdisk, is to build the required firmware blobs into the >>>>> kernel >>>>> image. [2] Of course, that's feasible only when a kernel image is >>>>> built >>>>> specificially for some device, because otherwise it would become too >>>>> large >>>>> because of too many drivers and their firmware blobs becoming >>>>> included, >>>>> but that seems to fit the Buildroot-based example. >>>>> >>>>> To sum it up, mechanisms already exist in the kernel for various >>>>> scenarios >>>>> when it comes to loading firmware blobs. Even if the deleted >>>>> workaround >>>>> attempts to solve some issue specific to some environment, that >>>>> isn't >>>>> the >>>>> right place or the right way for solving any issues of that kind. >>>>> >>>>> While preparing this patch, I even tried to find another kernel >>>>> driver >>>>> that >>>>> also implements some similar workarounds for firmware loading, to >>>>> justify >>>>> the existence of such workarounds and to possibly move them into the >>>>> kernel's >>>>> firmware-loading interface. Alas, I was unable to find such >>>>> workarounds >>>>> in >>>>> other drivers, which solidified my reasoning behind classifying the >>>>> removed >>>>> code as out-of-place and redundant. >>>> >>>> For some tiny embedded system，there is no such ramdisk，for example： >>>> a buildroot based rootfs，the buildroot only generate rootfs。 >>>> >>>> And FYI， there are mainline drivers try to fix such issue by >>>> defer_probe，for example： >>>> smc_abc[0] >>>> There are also some other similar scenario in gpu driver{1}[2] >>>> >>>> [0]https://elixir.bootlin.com/linux/latest/source/drivers/tee/optee/smc_abi.c#L1518 >>>> [1]https://patchwork.kernel.org/project/dri-devel/patch/20240109120604.603700-1-javierm@redhat.com/ >>>> [2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@minerva.mail-host-address-is-not-set/T/ >>> >>> Thanks for providing these examples. >>> >>> Before I continue thinking about the possible systemic solution, >>> could you please clarify the way Buildroot builds the kernel and >>> prepares the root filesystem? I'm not familiar with Buildroot, >>> but it seems to me that it builds the drivers statically into the >>> produced kernel image, while it places the related firmware blobs >>> into the produced root filesystem. Am I right there? >> >> in practice we can chose build the drivers statically into the kernel， >> we can also build it as a module。 >> And in both case， the firmware blobs are put in rootfs。 >> If the drivers is built as a module， the module will also put in >> rootfs， >> so its fine。 >> But if a drivers is built into the kernel ，it maybe can't access the >> firmware blob >> before the rootfs is mounted. >> So we can see some drivers try to use DEFER_PROBE to fix this issue. > >When Buildroot builds the drivers statically into the kernel image, >can it also be told to build the required firmware blobs into the >kernel image, for which there's already support in the kernel? I‘m not sure about that。Firmware and linux kernel are two seperate project or repository。 And i’m also not sure if that needs the support of the specific driver to build the firmware into the kernel？ > >Of course, that would be feasible if only a small number of firmware >blobs would end up built into the kernel image, i.e. if the Buildroot >build would be tailored for a specific board. > >Otherwise... > >>> As I already wrote earlier, and as the above-linked discussions >>> conclude, solving these issues doesn't belong to any specific driver. >>> It should be resolved within the kernel's firmware loading mechanism >>> instead, and no driver should be specific in that regard. >> >> IT would be good if it can be resolved within the kernel's firmware >> loading mechanism. > >... we'll need this as a systemic solution. > >>>>> [1] >>>>> https://www.kernel.org/doc/Documentation/driver-api/firmware/direct-fs-lookup.rst >>>>> [2] >>>>> https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst >>>>> >>>>>>> Various utilities used by Linux distributions to generate initial >>>>>>> ramdisks >>>>>>> need to obey the firmware-related module information, so we can >>>>>>> rely >>>>>>> on the >>>>>>> firmware blob being present in the generated initial ramdisks. >>>>>>> >>>>>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >>>>>>> --- >>>>>>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 >>>>>>> +++++--------------------- >>>>>>> 1 file changed, 10 insertions(+), 43 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>>> index bd7aa891b839..e1a7c6a1172b 100644 >>>>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>>> @@ -44,9 +44,9 @@ static inline struct cdn_dp_device >>>>>>> *encoder_to_dp(struct drm_encoder *encoder) >>>>>>> #define DPTX_HPD_DEL (2 << 12) >>>>>>> #define DPTX_HPD_SEL_MASK (3 << 28) >>>>>>> >>>>>>> -#define CDN_FW_TIMEOUT_MS (64 * 1000) >>>>>>> #define CDN_DPCD_TIMEOUT_MS 5000 >>>>>>> #define CDN_DP_FIRMWARE "rockchip/dptx.bin" >>>>>>> + >>>>>>> MODULE_FIRMWARE(CDN_DP_FIRMWARE); >>>>>>> >>>>>>> struct cdn_dp_data { >>>>>>> @@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct >>>>>>> cdn_dp_device *dp, >>>>>>> return PTR_ERR_OR_ZERO(dp->audio_pdev); >>>>>>> } >>>>>>> >>>>>>> -static int cdn_dp_request_firmware(struct cdn_dp_device *dp) >>>>>>> -{ >>>>>>> - int ret; >>>>>>> - unsigned long timeout = jiffies + >>>>>>> msecs_to_jiffies(CDN_FW_TIMEOUT_MS); >>>>>>> - unsigned long sleep = 1000; >>>>>>> - >>>>>>> - WARN_ON(!mutex_is_locked(&dp->lock)); >>>>>>> - >>>>>>> - if (dp->fw_loaded) >>>>>>> - return 0; >>>>>>> - >>>>>>> - /* Drop the lock before getting the firmware to avoid blocking >>>>>>> boot >>>>>>> */ >>>>>>> - mutex_unlock(&dp->lock); >>>>>>> - >>>>>>> - while (time_before(jiffies, timeout)) { >>>>>>> - ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>>>>>> - if (ret == -ENOENT) { >>>>>>> - msleep(sleep); >>>>>>> - sleep *= 2; >>>>>>> - continue; >>>>>>> - } else if (ret) { >>>>>>> - DRM_DEV_ERROR(dp->dev, >>>>>>> - "failed to request firmware: %d\n", ret); >>>>>>> - goto out; >>>>>>> - } >>>>>>> - >>>>>>> - dp->fw_loaded = true; >>>>>>> - ret = 0; >>>>>>> - goto out; >>>>>>> - } >>>>>>> - >>>>>>> - DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n"); >>>>>>> - ret = -ETIMEDOUT; >>>>>>> -out: >>>>>>> - mutex_lock(&dp->lock); >>>>>>> - return ret; >>>>>>> -} >>>>>>> - >>>>>>> static void cdn_dp_pd_event_work(struct work_struct *work) >>>>>>> { >>>>>>> struct cdn_dp_device *dp = container_of(work, struct >>>>>>> cdn_dp_device, >>>>>>> event_work); >>>>>>> struct drm_connector *connector = &dp->connector; >>>>>>> enum drm_connector_status old_status; >>>>>>> - >>>>>>> int ret; >>>>>>> >>>>>>> mutex_lock(&dp->lock); >>>>>>> >>>>>>> if (dp->suspended) >>>>>>> goto out; >>>>>>> >>>>>>> - ret = cdn_dp_request_firmware(dp); >>>>>>> - if (ret) >>>>>>> - goto out; >>>>>>> + if (!dp->fw_loaded) { >>>>>>> + ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>>>>>> + if (ret) { >>>>>>> + DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", ret); >>>>>>> + goto out; >>>>>>> + } >>>>>>> + >>>>>>> + dp->fw_loaded = true; >>>>>>> + } >>>>>>> >>>>>>> dp->connected = true; >>>>>>> > >_______________________________________________ >Linux-rockchip mailing list >Linux-rockchip@lists.infradead.org >http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Tue, Jul 09, 2024 at 12:10:51PM GMT, Dragan Simic wrote: > On 2024-07-09 11:10, Andy Yan wrote: > > At 2024-07-09 16:17:06, "Dragan Simic" <dsimic@manjaro.org> wrote: > > > On 2024-07-08 09:46, Andy Yan wrote: > > > > At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@manjaro.org> wrote: > > > > > On 2024-07-04 04:10, Andy Yan wrote: > > > > > > At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> wrote: > > > > > > > After the additional firmware-related module information was > > > > > > > introduced by > > > > > > > the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add > > > > > > > MODULE_FIRMWARE > > > > > > > macro"), there's no longer need for the > > > > > > > firmware-loading workarounds > > > > > > > whose > > > > > > > sole purpose was to prevent the missing firmware > > > > > > > blob in an initial > > > > > > > ramdisk > > > > > > > from causing driver initialization to fail. Thus, delete the > > > > > > > workarounds, > > > > > > > which removes a sizable chunk of redundant code. > > > > > > > > > > > > What would happen if there was no ramdisk? And the firmware is in > > > > > > rootfs ？ > > > > > > > > > > > > For example： A buildroot based tiny embedded system。 > > > > > > > > > > Good point, let me explain, please. > > > > > > > > > > In general, if a driver is built into the kernel, there > > > > > should also be > > > > > an initial ramdisk that contains the related firmware blobs, because > > > > > it's > > > > > unknown is the root filesystem available when the driver is probed. > > > > > If > > > > > a driver is built as a module and there's no initial ramdisk, having > > > > > the related firmware blobs on the root filesystem should be fine, > > > > > because > > > > > the firmware blobs and the kernel module become available at > > > > > the same > > > > > time, through the root filesystem. [1] > > > > > > > > > > Another option for a driver built statically into the kernel, when > > > > > there's > > > > > no initial ramdisk, is to build the required firmware blobs into the > > > > > kernel > > > > > image. [2] Of course, that's feasible only when a kernel image is > > > > > built > > > > > specificially for some device, because otherwise it would become too > > > > > large > > > > > because of too many drivers and their firmware blobs becoming > > > > > included, > > > > > but that seems to fit the Buildroot-based example. > > > > > > > > > > To sum it up, mechanisms already exist in the kernel for various > > > > > scenarios > > > > > when it comes to loading firmware blobs. Even if the deleted > > > > > workaround > > > > > attempts to solve some issue specific to some environment, > > > > > that isn't > > > > > the > > > > > right place or the right way for solving any issues of that kind. > > > > > > > > > > While preparing this patch, I even tried to find another > > > > > kernel driver > > > > > that > > > > > also implements some similar workarounds for firmware loading, to > > > > > justify > > > > > the existence of such workarounds and to possibly move them into the > > > > > kernel's > > > > > firmware-loading interface. Alas, I was unable to find such > > > > > workarounds > > > > > in > > > > > other drivers, which solidified my reasoning behind classifying the > > > > > removed > > > > > code as out-of-place and redundant. > > > > > > > > For some tiny embedded system，there is no such ramdisk，for example： > > > > a buildroot based rootfs，the buildroot only generate rootfs。 > > > > > > > > And FYI， there are mainline drivers try to fix such issue by > > > > defer_probe，for example： > > > > smc_abc[0] > > > > There are also some other similar scenario in gpu driver{1}[2] > > > > > > > > [0]https://elixir.bootlin.com/linux/latest/source/drivers/tee/optee/smc_abi.c#L1518 > > > > [1]https://patchwork.kernel.org/project/dri-devel/patch/20240109120604.603700-1-javierm@redhat.com/ > > > > [2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@minerva.mail-host-address-is-not-set/T/ > > > > > > Thanks for providing these examples. > > > > > > Before I continue thinking about the possible systemic solution, > > > could you please clarify the way Buildroot builds the kernel and > > > prepares the root filesystem? I'm not familiar with Buildroot, > > > but it seems to me that it builds the drivers statically into the > > > produced kernel image, while it places the related firmware blobs > > > into the produced root filesystem. Am I right there? > > > > in practice we can chose build the drivers statically into the kernel， > > we can also build it as a module。 > > And in both case， the firmware blobs are put in rootfs。 > > If the drivers is built as a module， the module will also put in > > rootfs， > > so its fine。 > > But if a drivers is built into the kernel ，it maybe can't access the > > firmware blob > > before the rootfs is mounted. > > So we can see some drivers try to use DEFER_PROBE to fix this issue. > > When Buildroot builds the drivers statically into the kernel image, > can it also be told to build the required firmware blobs into the > kernel image, for which there's already support in the kernel? > > Of course, that would be feasible if only a small number of firmware > blobs would end up built into the kernel image, i.e. if the Buildroot > build would be tailored for a specific board. IIRC, it can, but it's not really convenient from a legal point of view. > Otherwise... > > > > As I already wrote earlier, and as the above-linked discussions > > > conclude, solving these issues doesn't belong to any specific driver. > > > It should be resolved within the kernel's firmware loading mechanism > > > instead, and no driver should be specific in that regard. > > > > IT would be good if it can be resolved within the kernel's firmware > > loading mechanism. > > ... we'll need this as a systemic solution. The general policy has been to put drivers that need a firmware as a module, and just never build them statically. Maxime
Hello Andy, On 2024-07-09 12:46, Andy Yan wrote: > At 2024-07-09 18:10:51, "Dragan Simic" <dsimic@manjaro.org> wrote: >> On 2024-07-09 11:10, Andy Yan wrote: >>> At 2024-07-09 16:17:06, "Dragan Simic" <dsimic@manjaro.org> wrote: >>>> On 2024-07-08 09:46, Andy Yan wrote: >>>>> At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@manjaro.org> wrote: >>>>>> On 2024-07-04 04:10, Andy Yan wrote: >>>>>>> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> >>>>>>> wrote: >>>>>>>> After the additional firmware-related module information was >>>>>>>> introduced by >>>>>>>> the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add >>>>>>>> MODULE_FIRMWARE >>>>>>>> macro"), there's no longer need for the firmware-loading >>>>>>>> workarounds >>>>>>>> whose >>>>>>>> sole purpose was to prevent the missing firmware blob in an >>>>>>>> initial >>>>>>>> ramdisk >>>>>>>> from causing driver initialization to fail. Thus, delete the >>>>>>>> workarounds, >>>>>>>> which removes a sizable chunk of redundant code. >>>>>>> >>>>>>> What would happen if there was no ramdisk? And the firmware is in >>>>>>> rootfs ？ >>>>>>> >>>>>>> For example： A buildroot based tiny embedded system。 >>>>>> >>>>>> Good point, let me explain, please. >>>>>> >>>>>> In general, if a driver is built into the kernel, there should >>>>>> also >>>>>> be >>>>>> an initial ramdisk that contains the related firmware blobs, >>>>>> because >>>>>> it's >>>>>> unknown is the root filesystem available when the driver is >>>>>> probed. >>>>>> If >>>>>> a driver is built as a module and there's no initial ramdisk, >>>>>> having >>>>>> the related firmware blobs on the root filesystem should be fine, >>>>>> because >>>>>> the firmware blobs and the kernel module become available at the >>>>>> same >>>>>> time, through the root filesystem. [1] >>>>>> >>>>>> Another option for a driver built statically into the kernel, when >>>>>> there's >>>>>> no initial ramdisk, is to build the required firmware blobs into >>>>>> the >>>>>> kernel >>>>>> image. [2] Of course, that's feasible only when a kernel image is >>>>>> built >>>>>> specificially for some device, because otherwise it would become >>>>>> too >>>>>> large >>>>>> because of too many drivers and their firmware blobs becoming >>>>>> included, >>>>>> but that seems to fit the Buildroot-based example. >>>>>> >>>>>> To sum it up, mechanisms already exist in the kernel for various >>>>>> scenarios >>>>>> when it comes to loading firmware blobs. Even if the deleted >>>>>> workaround >>>>>> attempts to solve some issue specific to some environment, that >>>>>> isn't >>>>>> the >>>>>> right place or the right way for solving any issues of that kind. >>>>>> >>>>>> While preparing this patch, I even tried to find another kernel >>>>>> driver >>>>>> that >>>>>> also implements some similar workarounds for firmware loading, to >>>>>> justify >>>>>> the existence of such workarounds and to possibly move them into >>>>>> the >>>>>> kernel's >>>>>> firmware-loading interface. Alas, I was unable to find such >>>>>> workarounds >>>>>> in >>>>>> other drivers, which solidified my reasoning behind classifying >>>>>> the >>>>>> removed >>>>>> code as out-of-place and redundant. >>>>> >>>>> For some tiny embedded system，there is no such ramdisk，for example： >>>>> a buildroot based rootfs，the buildroot only generate rootfs。 >>>>> >>>>> And FYI， there are mainline drivers try to fix such issue by >>>>> defer_probe，for example： >>>>> smc_abc[0] >>>>> There are also some other similar scenario in gpu driver{1}[2] >>>>> >>>>> [0]https://elixir.bootlin.com/linux/latest/source/drivers/tee/optee/smc_abi.c#L1518 >>>>> [1]https://patchwork.kernel.org/project/dri-devel/patch/20240109120604.603700-1-javierm@redhat.com/ >>>>> [2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@minerva.mail-host-address-is-not-set/T/ >>>> >>>> Thanks for providing these examples. >>>> >>>> Before I continue thinking about the possible systemic solution, >>>> could you please clarify the way Buildroot builds the kernel and >>>> prepares the root filesystem? I'm not familiar with Buildroot, >>>> but it seems to me that it builds the drivers statically into the >>>> produced kernel image, while it places the related firmware blobs >>>> into the produced root filesystem. Am I right there? >>> >>> in practice we can chose build the drivers statically into the >>> kernel， >>> we can also build it as a module。 >>> And in both case， the firmware blobs are put in rootfs。 >>> If the drivers is built as a module， the module will also put in >>> rootfs， >>> so its fine。 >>> But if a drivers is built into the kernel ，it maybe can't access the >>> firmware blob >>> before the rootfs is mounted. >>> So we can see some drivers try to use DEFER_PROBE to fix this issue. >> >> When Buildroot builds the drivers statically into the kernel image, >> can it also be told to build the required firmware blobs into the >> kernel image, for which there's already support in the kernel? > > I‘m not sure about that。Firmware and linux kernel are two seperate > project or repository。 > And i’m also not sure if that needs the support of the specific driver > to build the firmware into the kernel？ > Please see the link below, which I actually referred to. That feature should allow required firmware blobs to be built into the kernel image, although I haven't tested it myself. https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst >> Of course, that would be feasible if only a small number of firmware >> blobs would end up built into the kernel image, i.e. if the Buildroot >> build would be tailored for a specific board. >> >> Otherwise... >> >>>> As I already wrote earlier, and as the above-linked discussions >>>> conclude, solving these issues doesn't belong to any specific >>>> driver. >>>> It should be resolved within the kernel's firmware loading mechanism >>>> instead, and no driver should be specific in that regard. >>> >>> IT would be good if it can be resolved within the kernel's firmware >>> loading mechanism. >> >> ... we'll need this as a systemic solution. >> >>>>>> [1] >>>>>> https://www.kernel.org/doc/Documentation/driver-api/firmware/direct-fs-lookup.rst >>>>>> [2] >>>>>> https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst >>>>>> >>>>>>>> Various utilities used by Linux distributions to generate >>>>>>>> initial >>>>>>>> ramdisks >>>>>>>> need to obey the firmware-related module information, so we can >>>>>>>> rely >>>>>>>> on the >>>>>>>> firmware blob being present in the generated initial ramdisks. >>>>>>>> >>>>>>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 >>>>>>>> +++++--------------------- >>>>>>>> 1 file changed, 10 insertions(+), 43 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>>>> index bd7aa891b839..e1a7c6a1172b 100644 >>>>>>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c >>>>>>>> @@ -44,9 +44,9 @@ static inline struct cdn_dp_device >>>>>>>> *encoder_to_dp(struct drm_encoder *encoder) >>>>>>>> #define DPTX_HPD_DEL (2 << 12) >>>>>>>> #define DPTX_HPD_SEL_MASK (3 << 28) >>>>>>>> >>>>>>>> -#define CDN_FW_TIMEOUT_MS (64 * 1000) >>>>>>>> #define CDN_DPCD_TIMEOUT_MS 5000 >>>>>>>> #define CDN_DP_FIRMWARE "rockchip/dptx.bin" >>>>>>>> + >>>>>>>> MODULE_FIRMWARE(CDN_DP_FIRMWARE); >>>>>>>> >>>>>>>> struct cdn_dp_data { >>>>>>>> @@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct >>>>>>>> cdn_dp_device *dp, >>>>>>>> return PTR_ERR_OR_ZERO(dp->audio_pdev); >>>>>>>> } >>>>>>>> >>>>>>>> -static int cdn_dp_request_firmware(struct cdn_dp_device *dp) >>>>>>>> -{ >>>>>>>> - int ret; >>>>>>>> - unsigned long timeout = jiffies + >>>>>>>> msecs_to_jiffies(CDN_FW_TIMEOUT_MS); >>>>>>>> - unsigned long sleep = 1000; >>>>>>>> - >>>>>>>> - WARN_ON(!mutex_is_locked(&dp->lock)); >>>>>>>> - >>>>>>>> - if (dp->fw_loaded) >>>>>>>> - return 0; >>>>>>>> - >>>>>>>> - /* Drop the lock before getting the firmware to avoid blocking >>>>>>>> boot >>>>>>>> */ >>>>>>>> - mutex_unlock(&dp->lock); >>>>>>>> - >>>>>>>> - while (time_before(jiffies, timeout)) { >>>>>>>> - ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>>>>>>> - if (ret == -ENOENT) { >>>>>>>> - msleep(sleep); >>>>>>>> - sleep *= 2; >>>>>>>> - continue; >>>>>>>> - } else if (ret) { >>>>>>>> - DRM_DEV_ERROR(dp->dev, >>>>>>>> - "failed to request firmware: %d\n", ret); >>>>>>>> - goto out; >>>>>>>> - } >>>>>>>> - >>>>>>>> - dp->fw_loaded = true; >>>>>>>> - ret = 0; >>>>>>>> - goto out; >>>>>>>> - } >>>>>>>> - >>>>>>>> - DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n"); >>>>>>>> - ret = -ETIMEDOUT; >>>>>>>> -out: >>>>>>>> - mutex_lock(&dp->lock); >>>>>>>> - return ret; >>>>>>>> -} >>>>>>>> - >>>>>>>> static void cdn_dp_pd_event_work(struct work_struct *work) >>>>>>>> { >>>>>>>> struct cdn_dp_device *dp = container_of(work, struct >>>>>>>> cdn_dp_device, >>>>>>>> event_work); >>>>>>>> struct drm_connector *connector = &dp->connector; >>>>>>>> enum drm_connector_status old_status; >>>>>>>> - >>>>>>>> int ret; >>>>>>>> >>>>>>>> mutex_lock(&dp->lock); >>>>>>>> >>>>>>>> if (dp->suspended) >>>>>>>> goto out; >>>>>>>> >>>>>>>> - ret = cdn_dp_request_firmware(dp); >>>>>>>> - if (ret) >>>>>>>> - goto out; >>>>>>>> + if (!dp->fw_loaded) { >>>>>>>> + ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); >>>>>>>> + if (ret) { >>>>>>>> + DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", >>>>>>>> ret); >>>>>>>> + goto out; >>>>>>>> + } >>>>>>>> + >>>>>>>> + dp->fw_loaded = true; >>>>>>>> + } >>>>>>>> >>>>>>>> dp->connected = true; >>>>>>>> >> >> _______________________________________________ >> Linux-rockchip mailing list >> Linux-rockchip@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hello Maxime, On 2024-07-09 13:09, Maxime Ripard wrote: > On Tue, Jul 09, 2024 at 12:10:51PM GMT, Dragan Simic wrote: >> On 2024-07-09 11:10, Andy Yan wrote: >> > At 2024-07-09 16:17:06, "Dragan Simic" <dsimic@manjaro.org> wrote: >> > > On 2024-07-08 09:46, Andy Yan wrote: >> > > > At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@manjaro.org> wrote: >> > > > > On 2024-07-04 04:10, Andy Yan wrote: >> > > > > > At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@manjaro.org> wrote: >> > > > > > > After the additional firmware-related module information was >> > > > > > > introduced by >> > > > > > > the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add >> > > > > > > MODULE_FIRMWARE >> > > > > > > macro"), there's no longer need for the >> > > > > > > firmware-loading workarounds >> > > > > > > whose >> > > > > > > sole purpose was to prevent the missing firmware >> > > > > > > blob in an initial >> > > > > > > ramdisk >> > > > > > > from causing driver initialization to fail. Thus, delete the >> > > > > > > workarounds, >> > > > > > > which removes a sizable chunk of redundant code. >> > > > > > >> > > > > > What would happen if there was no ramdisk? And the firmware is in >> > > > > > rootfs ？ >> > > > > > >> > > > > > For example： A buildroot based tiny embedded system。 >> > > > > >> > > > > Good point, let me explain, please. >> > > > > >> > > > > In general, if a driver is built into the kernel, there >> > > > > should also be >> > > > > an initial ramdisk that contains the related firmware blobs, because >> > > > > it's >> > > > > unknown is the root filesystem available when the driver is probed. >> > > > > If >> > > > > a driver is built as a module and there's no initial ramdisk, having >> > > > > the related firmware blobs on the root filesystem should be fine, >> > > > > because >> > > > > the firmware blobs and the kernel module become available at >> > > > > the same >> > > > > time, through the root filesystem. [1] >> > > > > >> > > > > Another option for a driver built statically into the kernel, when >> > > > > there's >> > > > > no initial ramdisk, is to build the required firmware blobs into the >> > > > > kernel >> > > > > image. [2] Of course, that's feasible only when a kernel image is >> > > > > built >> > > > > specificially for some device, because otherwise it would become too >> > > > > large >> > > > > because of too many drivers and their firmware blobs becoming >> > > > > included, >> > > > > but that seems to fit the Buildroot-based example. >> > > > > >> > > > > To sum it up, mechanisms already exist in the kernel for various >> > > > > scenarios >> > > > > when it comes to loading firmware blobs. Even if the deleted >> > > > > workaround >> > > > > attempts to solve some issue specific to some environment, >> > > > > that isn't >> > > > > the >> > > > > right place or the right way for solving any issues of that kind. >> > > > > >> > > > > While preparing this patch, I even tried to find another >> > > > > kernel driver >> > > > > that >> > > > > also implements some similar workarounds for firmware loading, to >> > > > > justify >> > > > > the existence of such workarounds and to possibly move them into the >> > > > > kernel's >> > > > > firmware-loading interface. Alas, I was unable to find such >> > > > > workarounds >> > > > > in >> > > > > other drivers, which solidified my reasoning behind classifying the >> > > > > removed >> > > > > code as out-of-place and redundant. >> > > > >> > > > For some tiny embedded system，there is no such ramdisk，for example： >> > > > a buildroot based rootfs，the buildroot only generate rootfs。 >> > > > >> > > > And FYI， there are mainline drivers try to fix such issue by >> > > > defer_probe，for example： >> > > > smc_abc[0] >> > > > There are also some other similar scenario in gpu driver{1}[2] >> > > > >> > > > [0]https://elixir.bootlin.com/linux/latest/source/drivers/tee/optee/smc_abi.c#L1518 >> > > > [1]https://patchwork.kernel.org/project/dri-devel/patch/20240109120604.603700-1-javierm@redhat.com/ >> > > > [2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@minerva.mail-host-address-is-not-set/T/ >> > > >> > > Thanks for providing these examples. >> > > >> > > Before I continue thinking about the possible systemic solution, >> > > could you please clarify the way Buildroot builds the kernel and >> > > prepares the root filesystem? I'm not familiar with Buildroot, >> > > but it seems to me that it builds the drivers statically into the >> > > produced kernel image, while it places the related firmware blobs >> > > into the produced root filesystem. Am I right there? >> > >> > in practice we can chose build the drivers statically into the kernel， >> > we can also build it as a module。 >> > And in both case， the firmware blobs are put in rootfs。 >> > If the drivers is built as a module， the module will also put in >> > rootfs， >> > so its fine。 >> > But if a drivers is built into the kernel ，it maybe can't access the >> > firmware blob >> > before the rootfs is mounted. >> > So we can see some drivers try to use DEFER_PROBE to fix this issue. >> >> When Buildroot builds the drivers statically into the kernel image, >> can it also be told to build the required firmware blobs into the >> kernel image, for which there's already support in the kernel? >> >> Of course, that would be feasible if only a small number of firmware >> blobs would end up built into the kernel image, i.e. if the Buildroot >> build would be tailored for a specific board. > > IIRC, it can, but it's not really convenient from a legal point of > view. Ah, makes sense. Very different licensing for the same file, etc. >> Otherwise... >> >> > > As I already wrote earlier, and as the above-linked discussions >> > > conclude, solving these issues doesn't belong to any specific driver. >> > > It should be resolved within the kernel's firmware loading mechanism >> > > instead, and no driver should be specific in that regard. >> > >> > IT would be good if it can be resolved within the kernel's firmware >> > loading mechanism. >> >> ... we'll need this as a systemic solution. > > The general policy has been to put drivers that need a firmware as a > module, and just never build them statically. I totally agree, but if Buildroot builds them statically and provides no initial ramdisk, we need a better solution than having various drivers attempt to implement their own workarounds.
On Tue, Jul 09, 2024 at 06:36:08PM GMT, Dragan Simic wrote: > > > > > As I already wrote earlier, and as the above-linked discussions > > > > > conclude, solving these issues doesn't belong to any specific driver. > > > > > It should be resolved within the kernel's firmware loading mechanism > > > > > instead, and no driver should be specific in that regard. > > > > > > > > IT would be good if it can be resolved within the kernel's firmware > > > > loading mechanism. > > > > > > ... we'll need this as a systemic solution. > > > > The general policy has been to put drivers that need a firmware as a > > module, and just never build them statically. > > I totally agree, but if Buildroot builds them statically and provides > no initial ramdisk, we need a better solution than having various drivers > attempt to implement their own workarounds. Buildroot typically allows custom kernel configurations, so it's not really "enforcing" anything like another distro does. It is definitely targetted towards very stripped down systems, so I guess building the drivers statically is a natural choice, but it works fine with modules too. Maxime
Hello Maxime, On 2024-07-10 09:13, Maxime Ripard wrote: > On Tue, Jul 09, 2024 at 06:36:08PM GMT, Dragan Simic wrote: >> > > > > As I already wrote earlier, and as the above-linked discussions >> > > > > conclude, solving these issues doesn't belong to any specific driver. >> > > > > It should be resolved within the kernel's firmware loading mechanism >> > > > > instead, and no driver should be specific in that regard. >> > > > >> > > > IT would be good if it can be resolved within the kernel's firmware >> > > > loading mechanism. >> > > >> > > ... we'll need this as a systemic solution. >> > >> > The general policy has been to put drivers that need a firmware as a >> > module, and just never build them statically. >> >> I totally agree, but if Buildroot builds them statically and provides >> no initial ramdisk, we need a better solution than having various >> drivers >> attempt to implement their own workarounds. > > Buildroot typically allows custom kernel configurations, so it's not > really "enforcing" anything like another distro does. > > It is definitely targetted towards very stripped down systems, so I > guess building the drivers statically is a natural choice, but it works > fine with modules too. It all leads to a conclusion that we need better in-kernel support for delayed firmware loading, instead of drivers implementing various workarounds, for the layouts in which drivers are built statically into the kernel image, but the required firmware blobs reside on the root filesystem. I'll start working on it, hopefully today. :)
On Wed, Jul 10, 2024 at 04:22:12PM GMT, Dragan Simic wrote: > Hello Maxime, > > On 2024-07-10 09:13, Maxime Ripard wrote: > > On Tue, Jul 09, 2024 at 06:36:08PM GMT, Dragan Simic wrote: > > > > > > > As I already wrote earlier, and as the above-linked discussions > > > > > > > conclude, solving these issues doesn't belong to any specific driver. > > > > > > > It should be resolved within the kernel's firmware loading mechanism > > > > > > > instead, and no driver should be specific in that regard. > > > > > > > > > > > > IT would be good if it can be resolved within the kernel's firmware > > > > > > loading mechanism. > > > > > > > > > > ... we'll need this as a systemic solution. > > > > > > > > The general policy has been to put drivers that need a firmware as a > > > > module, and just never build them statically. > > > > > > I totally agree, but if Buildroot builds them statically and provides > > > no initial ramdisk, we need a better solution than having various > > > drivers > > > attempt to implement their own workarounds. > > > > Buildroot typically allows custom kernel configurations, so it's not > > really "enforcing" anything like another distro does. > > > > It is definitely targetted towards very stripped down systems, so I > > guess building the drivers statically is a natural choice, but it works > > fine with modules too. > > It all leads to a conclusion that we need better in-kernel support > for delayed firmware loading, instead of drivers implementing various > workarounds, for the layouts in which drivers are built statically > into the kernel image, but the required firmware blobs reside on the > root filesystem. > > I'll start working on it, hopefully today. :) I don't want to prevent you from trying, but be aware that similar attempts have been shut down repeatedly in the past. Maxime
I'm having issues relevant to this thread. I want to build my rtl8723ds driver into the kernel. When fw_loader tries to load rtw8723d_fw.bin, my sysfs isn't mounted. Ideally, I don't have to use the driver as a module, use initramfs, or build the firmware into the kernel. Please let me know if I can do anything to help with the efforts. [ 0.193910] gpio gpiochip1: Static allocation of GPIO base is deprecated, use dynamic allocation. [ 0.196848] sun8i-a33-pinctrl 1c20800.pinctrl: initialized sunXi PIO driver [ 0.197475] sun8i-a33-pinctrl 1c20800.pinctrl: supply vcc-pb not found, using dummy regulator [ 0.198599] printk: legacy console [ttyS0] disabled [ 0.219380] 1c28000.serial: ttyS0 at MMIO 0x1c28000 (irq = 142, base_baud = 1500000) is a U6_16550A [ 0.226341] printk: legacy console [ttyS0] enabled [ 1.158740] sun8i-a33-pinctrl 1c20800.pinctrl: supply vcc-pg not found, using dummy regulator [ 1.189006] 1c28400.serial: ttyS1 at MMIO 0x1c28400 (irq = 143, base_baud = 1500000) is a U6_16550A [ 1.204192] cfg80211: Loading compiled-in X.509 certificates for regulatory database [ 1.205291] sun8i-a33-pinctrl 1c20800.pinctrl: supply vcc-pf not found, using dummy regulator [ 1.220994] Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7' [ 1.227118] sunxi-mmc 1c10000.mmc: allocated mmc-pwrseq [ 1.231759] Loaded X.509 cert 'wens: 61c038651aabdcf94bd0ac7ff06c7248db18c600' [ 1.239605] clk: Disabling unused clocks [ 1.240126] sunxi-mmc 1c0f000.mmc: Got CD GPIO [ 1.243767] ALSA device list: [ 1.250965] No soundcards found. [ 1.255508] platform regulatory.0: Direct firmware load for regulatory.db failed with error -2 [ 1.257835] sunxi-mmc 1c10000.mmc: initialized, max. request size: 16384 KB [6 [ 1.273169] sunxi-mmc 1c0f000.mmc: initialized, max. request size: 16384 KB [ 1.280679] Waiting for root device /dev/mmcblk1p1... [ 1.285837] sunxi-mmc 1c10000.mmc: card claims to support voltages below defined range [ 1.304227] mmc0: new high speed SDIO card at address 0001 [ 1.312047] rtw_8723ds mmc0:0001:1: Direct firmware load for rtw88/rtw8723d_fw.bin failed with error -2 [ 1.321568] rtw_8723ds mmc0:0001:1: failed to request firmware [ 1.330441] mmc1: host does not support reading read-only switch, assuming write-enable [ 1.334030] rtw_8723ds mmc0:0001:1: failed to load firmware [ 1.343207] mmc1: new high speed SDHC card at address aaaa [ 1.344048] rtw_8723ds mmc0:0001:1: failed to setup chip efuse info [ 1.351159] mmcblk1: mmc1:aaaa SC32G 29.7 GiB [ 1.355846] rtw_8723ds mmc0:0001:1: failed to setup chip information [ 1.366892] rtw_8723ds mmc0:0001:1: probe with driver rtw_8723ds failed with error -22 [ 1.367560] mmcblk1: p1 [ 1.434987] EXT4-fs (mmcblk1p1): orphan cleanup on readonly fs [ 1.443377] EXT4-fs (mmcblk1p1): mounted filesystem c5551457-88e0-47ae-9094-06babfcdeded ro with ordered data mode. Quota mode: disabled. [ 1.455974] VFS: Mounted root (ext4 filesystem) readonly on device 179:1. [ 1.465835] devtmpfs: mounted [ 1.470605] Freeing unused kernel image (initmem) memory: 1024K [ 1.476798] Run /sbin/init as init process [ 1.781051] EXT4-fs (mmcblk1p1): re-mounted c5551457-88e0-47ae-9094-06babfcdeded r/w. Quota mode: disabled. Thanks, Walker
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c index bd7aa891b839..e1a7c6a1172b 100644 --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c @@ -44,9 +44,9 @@ static inline struct cdn_dp_device *encoder_to_dp(struct drm_encoder *encoder) #define DPTX_HPD_DEL (2 << 12) #define DPTX_HPD_SEL_MASK (3 << 28) -#define CDN_FW_TIMEOUT_MS (64 * 1000) #define CDN_DPCD_TIMEOUT_MS 5000 #define CDN_DP_FIRMWARE "rockchip/dptx.bin" + MODULE_FIRMWARE(CDN_DP_FIRMWARE); struct cdn_dp_data { @@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct cdn_dp_device *dp, return PTR_ERR_OR_ZERO(dp->audio_pdev); } -static int cdn_dp_request_firmware(struct cdn_dp_device *dp) -{ - int ret; - unsigned long timeout = jiffies + msecs_to_jiffies(CDN_FW_TIMEOUT_MS); - unsigned long sleep = 1000; - - WARN_ON(!mutex_is_locked(&dp->lock)); - - if (dp->fw_loaded) - return 0; - - /* Drop the lock before getting the firmware to avoid blocking boot */ - mutex_unlock(&dp->lock); - - while (time_before(jiffies, timeout)) { - ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); - if (ret == -ENOENT) { - msleep(sleep); - sleep *= 2; - continue; - } else if (ret) { - DRM_DEV_ERROR(dp->dev, - "failed to request firmware: %d\n", ret); - goto out; - } - - dp->fw_loaded = true; - ret = 0; - goto out; - } - - DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n"); - ret = -ETIMEDOUT; -out: - mutex_lock(&dp->lock); - return ret; -} - static void cdn_dp_pd_event_work(struct work_struct *work) { struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device, event_work); struct drm_connector *connector = &dp->connector; enum drm_connector_status old_status; - int ret; mutex_lock(&dp->lock); if (dp->suspended) goto out; - ret = cdn_dp_request_firmware(dp); - if (ret) - goto out; + if (!dp->fw_loaded) { + ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev); + if (ret) { + DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", ret); + goto out; + } + + dp->fw_loaded = true; + } dp->connected = true;
After the additional firmware-related module information was introduced by the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add MODULE_FIRMWARE macro"), there's no longer need for the firmware-loading workarounds whose sole purpose was to prevent the missing firmware blob in an initial ramdisk from causing driver initialization to fail. Thus, delete the workarounds, which removes a sizable chunk of redundant code. Various utilities used by Linux distributions to generate initial ramdisks need to obey the firmware-related module information, so we can rely on the firmware blob being present in the generated initial ramdisks. Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 +++++--------------------- 1 file changed, 10 insertions(+), 43 deletions(-)