Bug 107047 - [PATCH] Fix details screen not showing when fbcon is loaded after starting plymouth
Summary: [PATCH] Fix details screen not showing when fbcon is loaded after starting pl...
Status: RESOLVED FIXED
Alias: None
Product: plymouth
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Ray Strode [halfline]
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-26 18:53 UTC by Hans de Goede
Modified: 2018-07-10 13:30 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/3] ply-renderer: Fix activate handling (1.05 KB, patch)
2018-06-26 18:53 UTC, Hans de Goede
Details | Splinter Review
[PATCH 2/3] main: Do not activate renderers when showing details (3.12 KB, patch)
2018-06-26 18:58 UTC, Hans de Goede
Details | Splinter Review
[PATCH 3/3] main: Move ply_device_manager_deactivate_renderers() into hide_splash() (1.98 KB, patch)
2018-06-26 18:58 UTC, Hans de Goede
Details | Splinter Review
[PATCH] show_theme: Only activate renderers if the splash uses pixel-displays (2.69 KB, patch)
2018-07-02 11:08 UTC, Hans de Goede
Details | Splinter Review
[PATCH] main: Consistently only (de)activate renderers if the splash uses pixel-displays (2.17 KB, patch)
2018-07-03 07:24 UTC, Hans de Goede
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Hans de Goede 2018-06-26 18:53:47 UTC
Created attachment 140348 [details] [review]
[PATCH 1/3] ply-renderer: Fix activate handling

Hi,

The 3 patches which I'm attaching are related to this kernel patch:

https://lkml.org/lkml/2018/6/26/489

Which delays fbcon taking over the console (and binding to fbdev devices) until there actually is some text output to the console. This is intended for use with the "quiet" cmdline option, in combination with a bootloader which leaves the vendor's logo / EFI bootgraphics put up by the firmware intact on the EFI framebuffer.

The end goal here is a boot where the firmware shows its boot graphics and these stay in place for a couple of seconds until the GUI loads and the GUI then smoothly takes over the framebuffer without any distruptions.

Plymouth details mode breaks with this change. The bringup of fbcon is now delayed till that switch to detailed mode (which causes the first output on the console when booting with "quiet" on the kernel commandline).

The fbcon init on the switch to detailed mode fails, because plymouth keeps its drm-master rights when switching to detailed mode. 

The drm_master rights are not dropped, because of various issues in the code to activate/deactivate renderers. These issues cause the drm renderer plugin to not be deactivated when toggling modes. See the individual commit messages for details.

Regards,

Hans
Comment 1 Hans de Goede 2018-06-26 18:58:12 UTC
Created attachment 140349 [details] [review]
[PATCH 2/3] main: Do not activate renderers when showing details
Comment 2 Hans de Goede 2018-06-26 18:58:34 UTC
Created attachment 140350 [details] [review]
[PATCH 3/3] main: Move ply_device_manager_deactivate_renderers() into hide_splash()
Comment 3 Ray Strode [halfline] 2018-06-29 20:16:10 UTC
Comment on attachment 140349 [details] [review]
[PATCH 2/3] main: Do not activate renderers when showing details

Review of attachment 140349 [details] [review]:
-----------------------------------------------------------------

so your patches make sense to me, but I don't think this one is good enough. At the moment showing_details is just used to determine what action pressing the escape key should do..which way to toggle.  You want to use it to know whether or not text is showing on screen, but the problem is, that details aren't the only case where text is showing on screen.  we also show text for other text based splash plugins.  I think probably we should add a function `ply_boot_splash_needs_graphics` or `ply_boot_splash_uses_pixel_displays` or something like that just does: 

`return splash->plugin_interface->add_pixel_display != NULL;`

then activate_renderers would only get called when that function returns true.
Comment 4 Ray Strode [halfline] 2018-06-29 20:16:50 UTC
i'm going to be gone next week, but feel free to build these patches in Fedora in the interim.
Comment 5 Ray Strode [halfline] 2018-06-29 20:27:48 UTC
i pushed two of these:

To ssh://git.freedesktop.org/git/plymouth
   014c215..778e0fb  master -> master
Comment 6 Hans de Goede 2018-07-01 12:29:22 UTC
Thank you for the review and for pushing the 2 patches.

Monday (tomorrow), I will modify the third patch as you suggested, retest and then attach it here.

I will also kick of a F29 plymouth build with the 3 patches added then.
Comment 7 Hans de Goede 2018-07-02 11:08:19 UTC
Created attachment 140422 [details] [review]
[PATCH] show_theme: Only activate renderers if the splash uses pixel-displays

Here is a patch replacing the "[PATCH 2/3] main: Do not activate renderers when showing details" patch as requested.

This patch introduces and uses a new ply_boot_splash_uses_pixel_displays() function as suggested.
Comment 8 Hans de Goede 2018-07-02 11:14:45 UTC
While working on this I noticed that there were quite a few compiler warning when building plymouth on Fedora 28, wrote 2 patches fixing these, see bug 107086.

Also a gentle reminder that I also have some details mode related patches pending in bug 107048.
Comment 9 Hans de Goede 2018-07-03 07:24:29 UTC
Created attachment 140444 [details] [review]
[PATCH] main: Consistently only (de)activate renderers if the splash uses pixel-displays

Hi,

As discussed I've done a plymouth build for F29/rawhide including the 3 patches, using the 2nd version of the patch to conditionally activate the renderers.

In hindsight that patch (as used in Fedora atm) is incomplete. I should have guarded all (de)activate)renderers calls with the same check. Both for consistency reasons and because not doing so actually causes problems (in some corner cases).

Here is a patch to consistently have the guard everywhere.

Regards,

Hans
Comment 10 Ray Strode [halfline] 2018-07-10 13:30:07 UTC
Thanks, squashed the two together and pushed:

To ssh://git.freedesktop.org/git/plymouth
   778e0fb..447c783  master -> master


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.