Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable the initial set of the CRTC when modes match #3034

Merged
merged 9 commits into from
Sep 11, 2023

Conversation

mattkae
Copy link
Contributor

@mattkae mattkae commented Sep 6, 2023

How to test 🧪

  1. Provide the run_with command for your compositor with the new SmoothBoothSupport option. An example of adding this to shell_main.cpp would be:
...
return runner.run_with(
        {
            CursorTheme{"default:DMZ-White"},
            WaylandExtensions{},
            X11Support{},
            SmoothBootSupport{}, // Add me!
            window_managers,
            display_configuration_options,
            external_client_launcher,
            launcher,
            config_keymap,
            AppendEventFilter{quit_on_ctrl_alt_bksp},
            StartupInternalClient{spinner},
            ConfigurationOption{run_startup_apps, "startup-apps", "Colon separated list of startup apps", ""},
            pre_init(ConfigurationOption{[&](std::string const& typeface) { ::wallpaper::font_file(typeface); },
                                         "shell-wallpaper-font", "font file to use for wallpaper", ::wallpaper::font_file()}),
            ConfigurationOption{[&](std::string const& cmd) { terminal_cmd = cmd; },
                                "shell-terminal-emulator", "terminal emulator to use", terminal_cmd}
        });
  1. Start your computer in a tty (this means that gbm cannot start. see [this link](systemctl set-default multi-user.target) on how to do that)
  2. Once started, ssh into your computer from another computer as root
  3. Run:
plymouthd
plymouth show-splash
plymouth deactivate
  1. Then run your shell, e.g. miral-shell
  2. Note that you should no longer see a black screen in-between plymouth and the compositor

What's new 🆕

  • Providing create_display with the options to that it can read whether or not it should do a smooth boot
  • If it should do a smooth boot, the Displaybuffer will NOT initialize the initial visible frame buffer AND it will NOT set the CRTC for each output. This ensures that plymouth's previous contents remain on the screen

Some caveats 🕳️

  • We may see a flicker if the next set_crtc changes the resolution or something

Up next ⏭️

I have another branch ready to go that will read the mapping into a Mir buffer and offer it back to the Scene to be displayed. However, this will call on us to transition the buffer out of existence, which is a bigger story

@mattkae mattkae marked this pull request as ready for review September 6, 2023 18:06
@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

❗ No coverage uploaded for pull request base (platform-API-merge@d32e781). Click here to learn what that means.
The diff coverage is 90.90%.

@@                  Coverage Diff                  @@
##             platform-API-merge    #3034   +/-   ##
=====================================================
  Coverage                      ?   77.10%           
=====================================================
  Files                         ?     1050           
  Lines                         ?    72609           
  Branches                      ?        0           
=====================================================
  Hits                          ?    55983           
  Misses                        ?    16626           
  Partials                      ?        0           
Files Changed Coverage Δ
src/platforms/gbm-kms/server/kms/kms_output.h 100.00% <ø> (ø)
...c/platforms/gbm-kms/server/kms/real_kms_output.cpp 78.66% <87.50%> (ø)
...rc/platforms/gbm-kms/server/kms/display_buffer.cpp 71.18% <93.75%> (ø)
...unit-tests/platforms/gbm-kms/kms/mock_kms_output.h 53.84% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Saviq
Copy link
Collaborator

Saviq commented Sep 6, 2023

@mattkae would you please add the option to the demo server?

@mattkae mattkae requested a review from RAOF September 7, 2023 15:30
@Saviq Saviq changed the title Disabling the initial set of the CRTC when the "smooth_boot" option is set to true Disable the initial set of the CRTC when modes match Sep 7, 2023
try
{
geometry::Rectangle rectangle = output->get_rectangle();
if (rectangle != area)
Copy link
Contributor

@RAOF RAOF Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably be checking the full drmModeModeInfo here? I don't know how often it'll come up, but if we're switching to a specific refresh rate (or variable refresh rate mode), then the size will match but the actual mode won't, and we'll need to do a full modeset.

Actually, maybe we should only be checking the drmModeModeInfo here - I'm fairly sure that's the only thing that needs to match in order for page flipping to work later; I'm fairly sure the (x,y) you're scanning out of the current framebuffer doesn't matter.

Hm, actually… do we even have the right information here? Maybe we need to remove needs_crtc_set from DisplayBuffer entirely, and have it as a method on KMSOutput that can check the current CRTC configuration against the requested CRTC configuration?

Copy link
Contributor Author

@mattkae mattkae Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions:

  • At startup, it seems to me like we're already setting the requested mode_index to the current_mode_index (which is read from the current_crtc) [See mgg::Display::configure_locked]. We can still do the check, but 99% of the time it appears that we are going to try and set the mode to the same mode that is currently set anyway (unless there is an error).

I think that moving the logic here to KMSOutput makes a lot of sense though!

@mattkae mattkae requested a review from RAOF September 8, 2023 18:27
Copy link
Contributor

@RAOF RAOF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup!

We'll probably extend this later into removing schedule_set_crtc and needs_set_crtc entirely, but that can probably wait for Atomic KMS.

@RAOF RAOF merged commit 7f5c19d into platform-API-merge Sep 11, 2023
29 of 30 checks passed
@RAOF RAOF deleted the flickerless-no-crtc branch September 11, 2023 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants