Re: [PATCH] drm/rockchip: cdn-dp: Remove redundant workarounds for firmware loading

[Maxime Ripard]

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@xxxxxxxxxxx> wrote:
> >Hello Andy,
> >
> >On 2024-07-04 04:10, Andy Yan wrote:
> >> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@xxxxxxxxxxx> 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

Attachment: signature.asc
Description: PGP signature