[DRM plugin] Remember static kernel objects, amplify use of DrmCrtc
ClosedPublic

Authored by romangg on Mar 20 2017, 10:24 PM.

Details

Summary

To get an image from KWin to the screen in the DRM pipeline we combine a CRTC, an encoder and a connector. These objects are static in the sense, that they represent real hardware on the graphics card, which doesn't change in a session. See here for more details: https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html

Until now we used DrmOutput as the main representation for such an active rendering pipeline. I.e. it gets created and destroyed on hot plug events of displays. On the other side we had no fixed representation of the static kernel objects throughout the lifetime of KWin. This has several disadvantages:

  • We always need to query all available static objects on an hot plug event.
  • We can't manipulate the frame buffer of a CRTC after an output has been disconnected
  • Adding functionality for driving multiple displays on a single CRTC (i.e. cloning) would be difficult
  • We can't destroy the last frame buffer on display disconnect because the CRTC still accesses it and have therefore a memory leak on every display disconnect

This patch solves these issues by storing representations of all available CRTC and Connector objects in DrmBackend on init via DrmCrtc and DrmConnector instances. On an hotplug event these vectors are looped for a fitting CRTC and Connector combinations. Buffer handling is moved to the respective CRTC instance. All changes in overview:

  • Query all available CRTCs and Connectors and save for subsequent hotplug events
  • Fix logic errors in queryResources()
  • Move framebuffers, buffer flip and blank logic in DrmCrtc
  • Remove restoreSaved(). It isn't necessary and is dangerous if the old framebuffer was deleted in the meantime. Also could reveal sensitive user info from old session.

Implements T5708.

Test Plan

Login, logout, VT switching, connect and disconnect external monitor, energy saving mode.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
romangg created this revision.Mar 20 2017, 10:24 PM
Restricted Application added a subscriber: kwin. · View Herald TranscriptMar 20 2017, 10:24 PM
romangg edited the summary of this revision. (Show Details)Mar 20 2017, 10:28 PM
romangg updated this revision to Diff 12805.Mar 25 2017, 11:11 PM
  • In queryRessources() test count_modes for new connections at the right time
  • On blank() also delete possible next buffer
romangg retitled this revision from [Drm Backend] Remember static kernel objects, amplify use of DrmCrtc to [Drm] Remember static kernel objects, amplify use of DrmCrtc.Mar 25 2017, 11:20 PM
romangg retitled this revision from [Drm] Remember static kernel objects, amplify use of DrmCrtc to [DRM plugin] Remember static kernel objects, amplify use of DrmCrtc.
romangg updated this revision to Diff 12809.Mar 25 2017, 11:55 PM

Small TODO edit.

romangg added inline comments.Mar 26 2017, 2:10 AM
plugins/platforms/drm/drm_object_crtc.cpp
94

This and the unconditional delete above will break in QPainter mode, where the buffers are reused.

anthonyfieroni added inline comments.
plugins/platforms/drm/drm_backend.cpp
315–316

You mean std::erase(std::remove_if(...), ....end()) otherwise you have dangling pointer(s) in conllection

438–454

This part looks incorrect to me, you need to set m_crtc and m_conn for init but if it fails then you go to outer loop it leave con and crtc with dangling output.

romangg updated this revision to Diff 12819.Mar 26 2017, 12:54 PM
romangg marked 2 inline comments as done.

Fixed problems, which were pointed out in inline comments.

plugins/platforms/drm/drm_backend.cpp
315–316

If remove_if is true, the object already got deleted in tryInit. remove_if then just removes the dangling pointer entry from the QVector. Afterwards everything is supposed to be clean, is it?

anthonyfieroni added inline comments.Mar 26 2017, 6:39 PM
plugins/platforms/drm/drm_backend.cpp
315–316
romangg added inline comments.Mar 26 2017, 7:08 PM
plugins/platforms/drm/drm_backend.cpp
315–316

Thanks! So like this?

m_connectors.erase(std::remove_if(m_connectors.begin(), m_connectors.end(), tryInit), m_connectors.end());
m_crtcs.erase(std::remove_if(m_crtcs.begin(), m_crtcs.end(), tryInit), m_crtcs.end());
anthonyfieroni added inline comments.Mar 26 2017, 7:12 PM
plugins/platforms/drm/drm_backend.cpp
315–316

Yes.

romangg updated this revision to Diff 12845.Mar 26 2017, 7:18 PM
romangg marked 5 inline comments as done.

Thanks to @anthonyfieroni for pointing out required use of erase-remove idiom.

Looks good to me. Just minor qAsConst cases.

plugins/platforms/drm/drm_backend.cpp
374

qAsConst(m_connectors)

410

qAsConst in case encoders() doesn't return a const ref.

415

qAsConst

romangg updated this revision to Diff 14323.May 9 2017, 12:41 AM
romangg marked 3 inline comments as done.

Use qAsConst and update to current master.

graesslin accepted this revision.May 9 2017, 2:59 PM
This revision is now accepted and ready to land.May 9 2017, 2:59 PM
This revision was automatically updated to reflect the committed changes.