add qnx platform support #42
Conversation
afrantzis
left a comment
There was a problem hiding this comment.
Thanks for the pull request. I have left some comments in-line.
src/gl-state-egl.cpp
Outdated
| GLStateEGL::init_display(void* native_display, GLVisualConfig& visual_config) | ||
| { | ||
| native_display_ = reinterpret_cast<EGLNativeDisplayType>(native_display); | ||
| native_display_ = static_cast<EGLNativeDisplayType>((intptr_t)native_display); |
There was a problem hiding this comment.
What is the purpose of this cast change? On systems where EGLNativeDisplayType is a pointer type (e.g. X11), the static_cast<> from intptr_t to EGLNativeDisplayType will fail. reinterpret_cast<> should work both if EGLNativeDisplay is a pointer type or an integer type. Is there something special in QNX requiring this change?
There was a problem hiding this comment.
on QNX, EGLNativeDisplayType is defined as an integer, looks like Apple did the same thing, in my opinion, this is unnecessary since pointer is much better and bigger than int. :)
anyway, reinterpre_cast will fail in converting int to typed pointer, so maybe we can just simply convert it like this:
native_display_ = (EGLNativeDisplayType)(uintptr_t)native_display;
now it works on both Linux & QNX.
There was a problem hiding this comment.
Ok, so the issue is that on QNX EGLNativeDisplayType is an integer type of size less than the native pointer type (e.g. EGLNativeDisplayType = 32 bits on a 64 bit system), and this is what causes the direct reinterpret_cast and, for that matter, a direct plain C cast, too, to fail. In this case, I am fine with the latest proposed change.
src/gl-state-egl.cpp
Outdated
| if (get_platform_display != nullptr) { | ||
| egl_display_ = get_platform_display( | ||
| GLMARK2_NATIVE_EGL_DISPLAY_ENUM, native_display_, NULL); | ||
| GLMARK2_NATIVE_EGL_DISPLAY_ENUM, (void *)native_display_, NULL); |
There was a problem hiding this comment.
reinterpret_cast<void*>(native_display_)
src/native-state-qnx.cpp
Outdated
| } | ||
|
|
||
| Log::debug("Creating Window buffers W: %d H: %d FMT: %d\n", | ||
| properties.width, properties.height, format); |
There was a problem hiding this comment.
now I replaced all the tabs to space.
qnx_build/common.mk
Outdated
| include $(QCONFIG) | ||
|
|
||
| define PINFO | ||
| PINFO DESCRIPTION=glmakr2 application |
There was a problem hiding this comment.
Typo glmakr2 -> glmark2.
afrantzis
left a comment
There was a problem hiding this comment.
Thanks for the update. I have a couple of additional comments (see inline), but other than them this looks good. Note that I don't have a QNX platform to try this on, so my impressions are from code review only.
| if (get_platform_display != nullptr) { | ||
| egl_display_ = get_platform_display( | ||
| GLMARK2_NATIVE_EGL_DISPLAY_ENUM, native_display_, NULL); | ||
| GLMARK2_NATIVE_EGL_DISPLAY_ENUM, reinterpret_cast<void *>(native_display_), NULL); |
There was a problem hiding this comment.
Unnecessary extra white space/indentation at start of line.
| void | ||
| NativeStateQnx::visible(bool visible) | ||
| { | ||
| win_visible_ = visible ? 1 : 0; |
There was a problem hiding this comment.
The core glmark core code expects that calling native_state.visible(true) will show the window without any additional calls (i.e. you can't expect that the core will call create_window() after visible(true)). The correct way to set this up would be for win_visible_ to start as false and then visible() to call screen_get_window_property_iv(..., SCREEN_PROPERTY_VISIBLE, ...) to show/hide the window.
Note that the current code happens to work because it makes the window visible from the start, but it's not strictly correct in terms of NativeState API.
This patch added egl native-state implementations on qnx platform.
also added qnx make files. so for building glmark2 on qnx, just goto qnx_build directory, setup qnx env and type make.