Differential D11758 Diff 30971 plugins/platforms/drm/egl_gbm_backend.cpp

Changeset View

Standalone View

View Options

plugins/platforms/drm/egl_gbm_backend.cpp

Show First 20 Lines • Show All 252 Lines • ▼ Show 20 Line(s)248 const EGLint config_attribs[] = {
253 EGL_ALPHA_SIZE, 0, 253​ EGL_ALPHA_SIZE, 0,
254 EGL_RENDERABLE_TYPE, isOpenGLES() ? EGL_OPENGL_ES2_BIT : EGL_OPENGL_BIT, 254​ EGL_RENDERABLE_TYPE, isOpenGLES() ? EGL_OPENGL_ES2_BIT : EGL_OPENGL_BIT,
255 EGL_CONFIG_CAVEAT, EGL_NONE, 255​ EGL_CONFIG_CAVEAT, EGL_NONE,
256 EGL_NONE, 256​ EGL_NONE,
257 }; 257​ };
258 258​
259 EGLint count; 259​ EGLint count;
260 EGLConfig configs[1024]; 260​ EGLConfig configs[1024];
261 if (eglChooseConfig(eglDisplay(), config_attribs, configs, 1, &count) == EGL_FALSE) { 261​ if (!eglChooseConfig(eglDisplay(), config_attribs, configs, sizeof(configs)/sizeof(EGLConfig), &count)) {
262 qCCritical(KWIN_DRM) << "choose config failed"; 262​ qCCritical(KWIN_DRM) << "choose config failed";
263 return false; 263​ return false;
264 } 264​ }
265 if (count != 1) { ​
266 qCCritical(KWIN_DRM) << "choose config did not return a config" << count; ​
267 return false; ​
268 } ​
269 setConfig(configs[0]); ​
270 265​
266​ qCDebug(KWIN_DRM) << "EGL buffer configs count:" << count;
267​
268​ // loop through all configs, chosing the first one that has suitable format
269​ for (EGLint i = 0; i < count; i++) {
270​ EGLint gbmFormat, blueSize, redSize, greenSize, alphaSize;
zzagUnsubmitted
Done

Coding style nitpick: use camelCase instead of snake_case.

zzag: Coding style nitpick: use camelCase instead of snake_case.
271​ // query some configuration parameters, to show in debug log
272​ eglGetConfigAttrib(eglDisplay(), configs[i], EGL_NATIVE_VISUAL_ID, &gbmFormat);
273​ // number of bits for color channel
274​ eglGetConfigAttrib(eglDisplay(), configs[i], EGL_RED_SIZE, &redSize);
275​ eglGetConfigAttrib(eglDisplay(), configs[i], EGL_GREEN_SIZE, &greenSize);
276​ eglGetConfigAttrib(eglDisplay(), configs[i], EGL_BLUE_SIZE, &blueSize);
277​ eglGetConfigAttrib(eglDisplay(), configs[i], EGL_ALPHA_SIZE, &alphaSize);
278​
279​ // output debug info
280​ // GBM formats are declared as FOURCC code (integer from ASCII chars, so use this fact)
281​ char gbmFormatStr[8] = {0, 0, 0, 0, 0, 0, 0, 0};
davidedmundsonUnsubmitted
Done

May as well wrap this bit in QLoggingCategory::isDebugEnabled()

If EGLInt is 8 bytes long you need to be 9 to include the '\0' character.
I can imagine this works if EGLInt is four bytes long, but then why 8? Are you adding padding?

davidedmundson: May as well wrap this bit in QLoggingCategory::isDebugEnabled() If EGLInt is 8 bytes long you…
282​ memcpy(gbmFormatStr, &gbmFormat, sizeof(EGLint));
283​ qCDebug(KWIN_DRM) << i << "th config GBM format:" << gbmFormatStr
davidedmundsonUnsubmitted
Done

th -> the

davidedmundson: th -> the
284​ << "; color sizes (RGBA order):" << redSize << greenSize << blueSize << alphaSize;
285​
286​ if ((gbmFormat == GBM_FORMAT_XRGB8888) || (gbmFormat == GBM_FORMAT_ARGB8888)) {
zzagUnsubmitted
Done

(a) Maybe, I'm wrong but that's not how arrays are compared.
(b) GBM_FORMAT_XRGB8888 is an integer(uint32_t).

zzag: (a) Maybe, I'm wrong but that's not how arrays are compared. (b) GBM_FORMAT_XRGB8888 is an…
zzagUnsubmitted
Done

Stop! Misinterpreted it with gbm_format_str

zzag: Stop! Misinterpreted it with gbm_format_str
alexeyminAuthorUnsubmitted
Done

Yeah, since GBM_FORMAT_XRGB8888 and others are defined as __gbm_fourcc_code('X', 'R', '2', '4'), which shifts ASCII chars into different offsets in integer, we can directly print this integer as string of chars, if followed by zero bytes.
And it was only used for debug output, guess this part of code can be removed at all. Especially part which gets EGL_RED_SIZE etc, they are not used for anything except debug output. But it was important to understand waht is happening here.

alexeymin: Yeah, since `GBM_FORMAT_XRGB8888` and others are defined as `__gbm_fourcc_code('X', 'R', '2'…
zzagUnsubmitted
Done

Sorry for that.

Anyways, you could merge these [this and one below] two if statements.

zzag: Sorry for that. Anyways, you could merge these [this and one below] two if statements.
alexeyminAuthorUnsubmitted
Done

NP!
About two ifs, initially I tested configs with GBM_FORMAT_XRGB8888 and GBM_FORMAT_ARGB8888 separately, that's why two ifs left. For me variant with ARGB also worked. Note: gbm surface is created with XRGB8888 format (link) , but I guess it is somehow compatible with config ARGB8888. I'm not sure if this is right thing to do; probably the if (gbm_format == GBM_FORMAT_ARGB8888) { block should be removed, if it causes problems for someone. It needs testing on different GPUs/drivers, I guess...

alexeymin: NP! About two `if`s, initially I tested configs with `GBM_FORMAT_XRGB8888` and…
287​ setConfig(configs[i]);
271 return true; 288​ return true;
272} 289​ }
290​ }
291​
292​ qCCritical(KWIN_DRM) << "choose EGL config did not return a suitable config" << count;
293​ return false;
294​}
273 295​
274void EglGbmBackend::present() 296​void EglGbmBackend::present()
275{ 297​{
276 for (auto &o: m_outputs) { 298​ for (auto &o: m_outputs) {
277 makeContextCurrent(o); 299​ makeContextCurrent(o);
278 presentOnOutput(o); 300​ presentOnOutput(o);
279 } 301​ }
280} 302​}
▲ Show 20 Lines • Show All 122 Lines • Show Last 20 Lines