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

(#3729) only DRM_MODE_CONNECTED outputs can be added to the output list #3747

Closed
wants to merge 1 commit into from

Conversation

mattkae
Copy link
Contributor

@mattkae mattkae commented Feb 7, 2025

fixes #3729

According to the documentation for drmModeConnection:

Describes the connector status. DRM_MODE_CONNECTED means that the connector has a sink plugged in. DRM_MODE_DISCONNECTED means the contrary. DRM_MODE_UNKNOWNCONNECTION is used when it could be either. User-space should first try to enable DRM_MODE_CONNECTED connectors and ignore other connectors. If there are no DRM_MODE_CONNECTED connectors, user-space should then try to probe and enable DRM_MODE_UNKNOWNCONNECTION connectors.

With this mind, it does not make sense for us to create a DisplaySink for outputs that are not connected, since these do not have display sinks.

How to test

  • Start miral-shell from your laptop (or something with a single display)
  • Plug in another output somehow
  • Once your mouse cursor disappears (this is by design apparently!), unplug the other output. You have to be fast! Once the mouse reappears, you are too late
  • The compositor should no longer crash because we only try to send buffers to outputs that are connected

@mattkae mattkae requested a review from a team as a code owner February 7, 2025 16:45
@mattkae mattkae requested a review from AlanGriffiths February 7, 2025 16:48
@mattkae mattkae changed the title (#3279) only connected outputs can be added to the output list (#3729) only connected outputs can be added to the output list Feb 7, 2025
@mattkae mattkae changed the title (#3729) only connected outputs can be added to the output list (#3729) only DRM_MODE_CONNECTED outputs can be added to the output list Feb 7, 2025
@mattkae
Copy link
Contributor Author

mattkae commented Feb 7, 2025

I will add tests once we are happy with this implementation.

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

Thanks for tracking that down!

Do we need a similar check in mga::RealKMSOutputContainer::update_from_hardware_state()?

@@ -14,10 +14,12 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#define MIR_LOG_COMPONENT "gbm-kms"
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be no need for this: MIR_LOG_COMPONENT_FALLBACK="gbm-kms" is in CMakeLists.txt

@@ -55,6 +57,12 @@ void mgg::RealKMSOutputContainer::update_from_hardware_state()
drm_fd == candidate->drm_fd();
});

if (connector->connection != DRM_MODE_CONNECTED)
{
mir::log_warning("Connector is not connected, so not adding this output to the list.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is "info" at best (nothing is wrong), and probably not useful at all without some way to identify which output.

@AlanGriffiths
Copy link
Collaborator

Oh, and do run mir_unit_tests_gbm-kms ... MesaDisplayConfigurationTest* before pushing an update

@mattkae
Copy link
Contributor Author

mattkae commented Feb 7, 2025

Maybe this isn't the best of ways to do this... Perhaps we still add the connector, but we don't use the output.

@mattkae
Copy link
Contributor Author

mattkae commented Feb 7, 2025

This is wrong. The tests proved this to me 👍

@mattkae mattkae closed this Feb 7, 2025
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.

If I time my output unplug at the right moment, then I can cause Mir to crash (tested on current main)
2 participants