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

[Dragan Simic]

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@xxxxxxxxxxx> wrote:
> > > > On 2024-07-08 09:46, Andy Yan wrote:
> > > > > At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@xxxxxxxxxxx> wrote:
> > > > > > 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。
> > > > >
> > > > > 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@xxxxxxxxxx/
> > > > > [2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@xxxxxxxxxxxx-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.