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

Tue Jul 9 16:36:08 UTC 2024

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 at manjaro.org> wrote:
>> > > On 2024-07-08 09:46, Andy Yan wrote:
>> > > > At 2024-07-04 18:35:42, "Dragan Simic" <dsimic at manjaro.org> wrote:
>> > > > > On 2024-07-04 04:10, Andy Yan wrote:
>> > > > > > At 2024-07-04 07:32:02, "Dragan Simic" <dsimic at 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.