nitpick

nitpick

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

nitpick: unrelated added empty line.

nitpick: unrelated added empty line.

nitpick: unrelated removed line

nitpick: unrelated removed line

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

maybe remove the debug spam instead of commenting :-)

nitpick: missing whitespaces

just as a hint: you can simplify that as:

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

can we be sure this loop terminates?

nitpick: whitespaces and missing braces

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

nitpick whitespaces.

I suggest to rename to Property or maybe DrmProperty?

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

nitpick: indentation

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
};

bool instead of int

whitespaces

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

nitpick: coding style

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.

nitpick: coding style

nitpick: braces missing

nitpick: whitespaces

for readability I would say

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

nitpick: coding style

coding style

coding style

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.

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

Awesome!

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

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.

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?

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.

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

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?

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.

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

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

yep that's a fine reasoning.

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

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

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

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

nitpick: coding style

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

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

please wrap in QByteArrayLiteral

QByteArrayLiteral

nitpick: coding style

QByteArrayLiteral

QByteArrayLiteral