Merge ~bdrung/ubuntu/+source/initramfs-tools:ubuntu/devel-drm into ~ubuntu-core-dev/ubuntu/+source/initramfs-tools:ubuntu/devel

Proposed by Benjamin Drung
Status: Merged
Merge reported by: Benjamin Drung
Merged at revision: 429179111c1c97da9290beb0a8fc24af89fc82f5
Proposed branch: ~bdrung/ubuntu/+source/initramfs-tools:ubuntu/devel-drm
Merge into: ~ubuntu-core-dev/ubuntu/+source/initramfs-tools:ubuntu/devel
Diff against target: 85 lines (+36/-22)
3 files modified
debian/changelog (+16/-0)
hook-functions (+13/-0)
hooks/framebuffer (+7/-22)
Reviewer Review Type Date Requested Status
Daniel van Vugt (community) Needs Information
Review via email: mp+462691@code.launchpad.net

This proposal supersedes a proposal from 2024-03-19.

Description of the change

Implement https://bugs.launchpad.net/ubuntu/+source/plymouth/+bug/1970069/comments/95

There is an alternative: Only pick the first two commits (move framebuffer drivers into auto_add_modules) and then drop the framebuffer hook (Debian dropped that hook a long time ago) and just let Plymouth call:

```
if [ "${MODULES-}" = "dep" ]; then
 if [ -e /sys/class/drm ]; then
  class_add_modules drm
 fi
elif [ "$MODULES" != "list" ]; then
 auto_add_modules drm fbdev

 for x in "${MODULESDIR}"/initrd/*; do
  x=${x##*/}
  x=${x%.*}
  case ${x} in
  '*')
   break
   ;;
  *fb)
   ;;
  esac

  manual_add_modules "${x}"
 done
fi
```

The clear separation to initramfs-tools are auto_add_modules and class_add_modules in this case.

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote (last edit ):

> framebuffer: Drop not existing fbcon video kernel module

I believe we should still be including drivers that might be built as a module in some kernels (like simpledrm and efifb in older Ubuntu kernels). fbcon is potentially needed to provide the console on which the disk unlock prompt appears if there is no working Plymouth. But maybe that dependency on fbcon belongs elsewhere with the disk unlock prompt...

Revision history for this message
Daniel van Vugt (vanvugt) :
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

> There is an alternative: Only pick the first two commits (move framebuffer drivers into auto_add_modules) and then drop the framebuffer hook (Debian dropped that hook a long time ago)

There have been moments where I wondered if FRAMEBUFFER=y needs to exist, but I think it's good to keep because:

 * It's simpler for consumers.
 * It prevents other packages from accidentally pulling in more drivers than they need to (like Plymouth does right now).
 * It prevents other packages from having to learn new syntax.
 * I don't think there should be a "drm" class because it implies the full set of large DRM drivers that we _don't_ want.

4f00055... by Benjamin Drung

Move adding framebuffer drivers into auto_add_modules

The hook function `auto_add_modules` contains all the logic for
selecting kernel modules. So move adding the framebuffer drivers from
the framebuffer hook into `auto_add_modules`.

93079d6... by Benjamin Drung

framebuffer: Drop looking in $MODULESDIR/initrd/ for kernel modules

The `${MODULESDIR}/initrd` kernel directory is a relic from the Linux
kernel. Looking in this (now empty) directory can be dropped (according
to Timo Aaltonen).

bc9b014... by Benjamin Drung

Support MODULES=dep in framebuffer hook

Import MODULES=dep logic from plymouth except for excluding specific
modules in this case. The git history did not reveal a reason for
excluding those modules and we want the system loaded modules in the
`MODULES=dep` mode.

LP: #1970069

4291791... by Benjamin Drung

Release initramfs-tools 0.142ubuntu23

Signed-off-by: Benjamin Drung <email address hidden>

Revision history for this message
Benjamin Drung (bdrung) wrote :

* Dropped the "framebuffer: Drop not existing fbcon video kernel module" commit
* renamed "drm" to "minimal_drm" to make the purpose clearer
* dropped the $MODULESDIR/initrd/ code (checked with the kernel team)

Please re-review.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Looks fine, but is "${MODULES-}" a typo? Is it meant to look different to "$MODULES" below it?

Revision history for this message
Daniel van Vugt (vanvugt) :
review: Needs Information
Revision history for this message
Benjamin Drung (bdrung) wrote :

The "${MODULES-}" is used in case "set -u" is used. See https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

Thanks for your review. I'll use that syntax for the second occurrence and release that new version.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index b45ac16..b9d94a8 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,19 @@
1initramfs-tools (0.142ubuntu23) noble; urgency=medium
2
3 [ Daniel van Vugt ]
4 * hooks/framebuffer: Only add simple/tiny framebuffer drivers. This is to
5 limit the size of initrd when FRAMEBUFFER=y is soon enabled for desktop
6 installations (LP: #1970069, #1869655).
7
8 [ Benjamin Drung ]
9 * autopkgtest: Increase QEMU timeouts on arm64/armhf
10 * hooks/framebuffer:
11 - Move adding framebuffer drivers into auto_add_modules
12 - Drop looking in $MODULESDIR/initrd/ for kernel modules
13 - Support MODULES=dep in framebuffer hook
14
15 -- Benjamin Drung <bdrung@ubuntu.com> Wed, 20 Mar 2024 11:57:22 +0100
16
1initramfs-tools (0.142ubuntu22) noble; urgency=medium17initramfs-tools (0.142ubuntu22) noble; urgency=medium
218
3 * autopkgtest: update systemd-udevd path from /lib to /usr/lib19 * autopkgtest: update systemd-udevd path from /lib to /usr/lib
diff --git a/hook-functions b/hook-functions
index 33e03e6..8aa8dec 100644
--- a/hook-functions
+++ b/hook-functions
@@ -747,6 +747,19 @@ auto_add_modules()
747 modules="$modules pwm_imx27 nwl-dsi ti-sn65dsi86 imx-dcss"747 modules="$modules pwm_imx27 nwl-dsi ti-sn65dsi86 imx-dcss"
748 modules="$modules mux-mmio mxsfb imx8mq-interconnect"748 modules="$modules mux-mmio mxsfb imx8mq-interconnect"
749 ;;749 ;;
750 fbdev)
751 # Video drivers from drivers/video/fbdev
752 modules="$modules efifb fbcon simplefb vesafb vga16fb"
753 ;;
754 minimal_drm)
755 # Minimal required set of Direct Rendering Manager (DRM) drivers for video
756 modules="$modules =drivers/gpu/drm/tiny vboxvideo virtio-gpu"
757 # if there is a privacy screen then its driver must be loaded before the
758 # kms driver will bind, otherwise its probe() will return -EPROBE_DEFER
759 # So include privacy screen providers
760 # atm all providers live under drivers/platform/x86
761 manual_add_modules -s "drm_privacy_screen_register" "=drivers/platform/x86"
762 ;;
750 virtual)763 virtual)
751 # Hyper-V764 # Hyper-V
752 modules="$modules hv_vmbus hv_utils hv_netvsc hv_mouse hv_storvsc hyperv-keyboard"765 modules="$modules hv_vmbus hv_utils hv_netvsc hv_mouse hv_storvsc hyperv-keyboard"
diff --git a/hooks/framebuffer b/hooks/framebuffer
index c559efd..336fc85 100755
--- a/hooks/framebuffer
+++ b/hooks/framebuffer
@@ -19,25 +19,10 @@ esac
1919
20. /usr/share/initramfs-tools/hook-functions20. /usr/share/initramfs-tools/hook-functions
2121
22manual_add_modules "=drivers/gpu/drm/tiny" \22if [ "${MODULES-}" = "dep" ]; then
23 fbcon vesafb vga16fb vboxvideo simplefb efifb virtio-gpu23 if [ -e /sys/class/drm ]; then
2424 class_add_modules drm
25# if there is a privacy screen then its driver must be loaded before the25 fi
26# kms driver will bind, otherwise its probe() will return -EPROBE_DEFER26elif [ "$MODULES" != "list" ]; then
27# So include privacy screen providers27 auto_add_modules fbdev minimal_drm
28# atm all providers live under drivers/platform/x8628fi
29manual_add_modules -s "drm_privacy_screen_register" "=drivers/platform/x86"
30
31for x in "${MODULESDIR}"/initrd/*; do
32 x=${x##*/}
33 x=${x%.*}
34 case ${x} in
35 '*')
36 break
37 ;;
38 *fb)
39 ;;
40 esac
41
42 manual_add_modules "${x}"
43done

Subscribers

People subscribed via source and target branches