Atomic Mode Setting / Universal Plane preliminary support
ClosedPublic

Authored by romangg on Aug 8 2016, 9:31 AM.

Details

Summary

This is Milestone I of full support of Atomic Mode Setting (AMS) and Universal Planes in the KWin DRM backend.

With Milestone I we can use the primary plane of a DRM output and do an AMS commit (this means mode setting aswell as page flipping), if the driver supports it.
Until now the functionality is only tested on Intel graphics. You need the drm-next kernel for most recent DRM kernel developments. As boot option set "i915.nuclear_pageflip". Additionally at the moment AMS is still hidden behind the environment variable KWIN_DRM_AMS. Set it, if you want to try out AMS.

What needs to be done next: Make it possible to transfer EGL buffers directly to planes and implement logic for deciding about using a plane or not for a specific buffer.

You can read more about it on LWN: https://lwn.net/Articles/653071
And on Martin's blog: https://blog.martin-graesslin.com/blog/2015/08/layered-compositing/
I used as model previous work by Daniel Stone for Weston: https://git.collabora.com/cgit/user/daniels/weston.git

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 retitled this revision from to Atomic Mode Setting / Universal Plane preliminary support.Aug 8 2016, 9:31 AM
romangg updated this object.
romangg edited the test plan for this revision. (Show Details)
romangg added a reviewer: graesslin.
romangg set the repository for this revision to R108 KWin.
romangg added a project: KWin.
Restricted Application added a subscriber: kwin. · View Herald TranscriptAug 8 2016, 9:31 AM
graesslin edited edge metadata.Aug 8 2016, 2:31 PM

Lots of minor coding style issues otherwise looks quite good :-)

There's one thing which looks dangerous and is in a few new classes: you delete this from within the ctor. That looks dangerous and crashy (heap-use-after-free). The returned object will have junk memory in that case. If it gets to that point at all. It can also fail on the stack already.

Possible solution to the problem is to move the init method outside of the ctor, so that the user of the new object has to call it. Then it can perform the cleanup if needed.

plugins/platforms/drm/drm_backend.cpp
322

nitpick

351

nitpick

361–366

would it make sense to also create the Crtc and Connector in the non AMS mode? My thinking is to reduce the differences

plugins/platforms/drm/drm_backend.h
33

nitpick: unrelated added empty line.

83

nitpick: unrelated added empty line.

95

nitpick: unrelated removed line

134

nitpick: unrelated removed line

141

for better readability I would add the comment in a dedicated line above the variable

plugins/platforms/drm/drm_object.cpp
65

maybe remove the debug spam instead of commenting :-)

93

nitpick: missing whitespaces

100

just as a hint: you can simplify that as:

DrmObject::Prop::~Prop() = default;
123

can we be sure this loop terminates?

125

nitpick: whitespaces and missing braces

139

do we need this loop? All it does is print out debug values...

140

nitpick whitespaces.

plugins/platforms/drm/drm_object.h
95

I suggest to rename to Property or maybe DrmProperty?

plugins/platforms/drm/drm_object_connector.cpp
33

that looks very dangerous. I would not delete from the ctor and I think that could/will crash.

54

nitpick: indentation

plugins/platforms/drm/drm_object_connector.h
36

general coding style comment. Types normally start with a capital letter. So Property (no need for the Enum suffix, It's properly namespaced). Also for enums we prefer to use the new enum class nowadays which gives you proper namespacing. And the enum values follow the normal CamelCase rules.

So for this enum it should be something like:

enum class Property {
    Crtc,
    Count
};
41

bool instead of int

plugins/platforms/drm/drm_object_plane.cpp
115–121

whitespaces

163–168

is the differentiation of the return values needed? If not I suggest to go with a bool

plugins/platforms/drm/drm_output.cpp
111

nitpick: coding style

112

I fear this could result in crashers of the QPainter compositor. It's using two buffers and if the old one gets deleted here it will try to use it again in the next frame, which will boom. No real idea how we can do that. The best idea I have right now is a flag: deleteAfterPageFlip in DrmBuffer.

118–119

nitpick: coding style

120

nitpick: braces missing

122

nitpick: whitespaces

543

for readability I would say

p->type() != DrmPlane::TYPE_CURSOR
587

nitpick: coding style

596–598

coding style

601–602

coding style

romangg marked 26 inline comments as done.Aug 8 2016, 10:38 PM

Lots of minor coding style issues otherwise looks quite good :-)

There's one thing which looks dangerous and is in a few new classes: you delete this from within the ctor. That looks dangerous and crashy (heap-use-after-free). The returned object will have junk memory in that case. If it gets to that point at all. It can also fail on the stack already.

Possible solution to the problem is to move the init method outside of the ctor, so that the user of the new object has to call it. Then it can perform the cleanup if needed.

Regarding the "delete this" in constructor. Well, I looked it up of course before using it, and as long as you always use only the new keyword to initiate an object, you should be safe. With this I hoped to do stuff like if (DrmPlane *p = new DrmPlane(kplane->plane_id, m_fd) in DrmBackend, but now I read somewhere that in this case with delete this in the constructor the returned pointer would be undefined and not set to nullptr. So to make sure I implemented your suggestion of using a seperate init function (although it's IMHO way more ugly than doing the above :'| ).

Just out of curiosity, what do you think about the error handling? At one point I commented, that throw-catch would make sense here. It would look cleaner in some areas definitely. But if you oppose it, I'm fine with that (less work for me).

plugins/platforms/drm/drm_backend.cpp
361–366

I think we talked about this topic already. The problem is, that DrmCrtc and DrmConnector ask the kernel for properties, which are only available in AMS, specifically DrmConnector asks for the atomic property CRTC_ID, which I know of being atomic. The properties MODE_ID and ACTIVE of DrmCrtc could also be atomically, but I couldn't find any informations in this regard.
On second thought, as long as the objects can call drmModeObjectGetProperties, the code in DrmObject should be robust enough, to ask for properties, which are not available, without crashing. I'll try it out.

plugins/platforms/drm/drm_object.cpp
65

Fine! But watching the changing property values on repaints can be really helpful and now I have to remember where to put it in emergency case. Also I found my comment line to be very edgy. :D

100

Awesome!

123
  • nameCount = m_enumNames.size()
  • initEnumMap called <=> !enumNames.isEmpty()
  • So nameCount >= 1
  • On j== nameCount the loop breaks, whereas 1<=j<=nameCount
139

This is meant by the TODO above. I wanted to do something like:

if (debug) {
    for (...) {
    ...
    }
}

But I didn't succeed in this. The problem was, that, although QLoggingCategory has an isEnabled() method, I couldn't apply it on the macros in logging.h in some meaningful way. If you know otherwise, please tell me.

In general the debug statement is really useful, since otherwise you wouldn't know which enum value the object has.
On the other side if there is no easy solution it's also fine with me, if we simply remove the loop.

plugins/platforms/drm/drm_object.h
95

Ok, I chose Property over DrmProperty, because the class is only visible from within DrmObject and subclasses and all access to its objects goes through DrmObject. Do you like the reasoning, i.e. is it in line with other code in KWin?

plugins/platforms/drm/drm_object_connector.h
36

I tried using enum classes but the problem is, that you can't subclass them and implicit conversion between int and enum class are not allowed. Look at DrmObject::propValue(int index) and the other property functions there. You want to call them with the enum implemented in the respective subclass, but if you do it with enum classes, you would need always a static cast (look at DrmOutput::atomicReqModesetPopulate, why this would be ugly.

plugins/platforms/drm/drm_object_plane.cpp
163–168

Yes, it's needed.

  • -1 = error, abort atomic commit
  • 0 = no error but no value needed to be changed -> don't include plane in atomic commit
  • 1 = no error and value needed to be changed -> include in atomic commit
romangg marked 4 inline comments as done.Aug 8 2016, 11:28 PM
romangg added inline comments.
plugins/platforms/drm/drm_backend.cpp
361–366

I encountered a second more severe problem when trying it out: drmModeSetCrtc in "drm_output.cpp" expects a pointer to the id of a connector. But until now we use in DrmObject only a getter function for obtaining the object id as a value.

We could in general change it for giving back a pointer on the value, but this would circumvent the safeguard of the encapsulation. And we would need to change all calls for it. I really don't think this is proportional to the result: Disposing the two member variables m_crtcId and m_connector in DrmOutput. What do you think?

plugins/platforms/drm/drm_output.cpp
112

This is definitely a grave issue, which I overlooked. Do you want to think a little bit longer about another solution or should we go with the flag? At the moment I also can't think of something better.

graesslin added inline comments.Aug 9 2016, 7:50 AM
plugins/platforms/drm/drm_backend.cpp
361–366

hmm yes that second problem is more severe. So disposing the two members might be the best idea.

plugins/platforms/drm/drm_object.cpp
139

Apparently there is QLoggingCategory::isDebugEnabled which should allow exactly that.

plugins/platforms/drm/drm_object.h
95

yep that's a fine reasoning.

plugins/platforms/drm/drm_object_connector.h
36

you don't need to static_cast. You can also int(EnumValue).

plugins/platforms/drm/drm_object_plane.cpp
163–168

ok, please return an enum value instead of int describing the values.

plugins/platforms/drm/drm_output.cpp
112

If I find something better I will tell, otherwise flag it is.

romangg updated this revision to Diff 5854.Aug 12 2016, 8:40 AM
romangg edited edge metadata.
romangg marked 21 inline comments as done.

Haven't tested everything yet, but I'm all for trying to merge it into master as soon as possible!

plugins/platforms/drm/drm_object.cpp
51

I think you don't need the explicit conversion to QByteArray - or you could use qstrcmp.

62

nitpick: coding style

plugins/platforms/drm/drm_object.h
69

what does the int return value mean? I see 0 and -1 being returned from the implementation. Maybe bool would be better?

84

same here. What does the return value mean? From implementation it seems to be tri-state?

plugins/platforms/drm/drm_object_connector.cpp
48

please wrap in QByteArrayLiteral

plugins/platforms/drm/drm_object_crtc.cpp
47–48

QByteArrayLiteral

plugins/platforms/drm/drm_object_plane.cpp
49

nitpick: coding style

62–72

QByteArrayLiteral

76–78

QByteArrayLiteral

romangg updated this revision to Diff 6176.Aug 23 2016, 9:45 AM
romangg marked 9 inline comments as done.

Resolved issues from last code review by Martin.

graesslin accepted this revision.Aug 29 2016, 12:52 PM
graesslin edited edge metadata.
This revision is now accepted and ready to land.Aug 29 2016, 12:52 PM
This revision was automatically updated to reflect the committed changes.