Changeset View
Standalone View
plugins/platforms/drm/egl_gbm_backend.cpp
| Show First 20 Lines • Show All 432 Lines • ▼ Show 20 Line(s) | |||||
| 433 | } | 433 | } | ||
| 434 | 434 | | |||
| 435 | void EglGbmBackend::present() | 435 | void EglGbmBackend::present() | ||
| 436 | { | 436 | { | ||
| 437 | Q_UNREACHABLE(); | 437 | Q_UNREACHABLE(); | ||
| 438 | // Not in use. This backend does per-screen rendering. | 438 | // Not in use. This backend does per-screen rendering. | ||
| 439 | } | 439 | } | ||
| 440 | 440 | | |||
| 441 | static QVector<EGLint> regionToRects(const QRegion &lastRegion, int height) | ||||
zzag: s/lastRegion/region/ | |||||
The region is in the global screen coordinates, but the buffer damage needs to be in the output local coordinates. zzag: The region is in the global screen coordinates, but the buffer damage needs to be in the output… | |||||
zzag: whitespace before `*` | |||||
| 442 | { | ||||
| 443 | QVector<EGLint> rects; | ||||
| 444 | rects.reserve(lastRegion.rectCount() * 4); | ||||
It would be nice to map rects from the global coordinate space to the output coordinate space and scale them both at the same time to avoid an extra for loop and save a couple of memory allocations. zzag: It would be nice to map rects from the global coordinate space to the output coordinate space… | |||||
zzag: No auto please. | |||||
| 445 | for (const auto &rect : lastRegion) { | ||||
| 446 | rects << rect.left(); | ||||
| 447 | rects << rect.right(); | ||||
I suggest to use (rect.y() + rect.height()) instead of rect.bottom() + 1 because QRect::bottom() deviates from the true bottom border. QRect::right() and QRect::bottom() are good when you need to snap the right or the bottom border of one rectangle to another, e.g. rect.moveRight(anotherRect.right()); otherwise, it is better to stick with x() + width() and y() + height() to avoid off by one errors. zzag: I suggest to use (rect.y() + rect.height()) instead of rect.bottom() + 1 because QRect::bottom… | |||||
| 448 | rects << height - rect.top(); | ||||
| 449 | rects << height - rect.bottom(); | ||||
| 450 | } | ||||
| 451 | return rects; | ||||
| 452 | } | ||||
| 453 | | ||||
| 441 | void EglGbmBackend::presentOnOutput(Output &output) | 454 | void EglGbmBackend::presentOnOutput(Output &output) | ||
| 442 | { | 455 | { | ||
| 456 | if (supportsSwapBuffersWithDamage()) { | ||||
| 457 | QVector<EGLint> rects = regionToRects(output.damageHistory.constFirst(), | ||||
| 458 | output.output->geometry().height()); | ||||
| 459 | eglSwapBuffersWithDamageEXT(eglDisplay(), output.eglSurface, | ||||
| 460 | rects.data(), rects.count()/4); | ||||
| 461 | } else { | ||||
| 443 | eglSwapBuffers(eglDisplay(), output.eglSurface); | 462 | eglSwapBuffers(eglDisplay(), output.eglSurface); | ||
| 463 | } | ||||
| 444 | output.buffer = m_backend->createBuffer(output.gbmSurface); | 464 | output.buffer = m_backend->createBuffer(output.gbmSurface); | ||
| 445 | 465 | | |||
damagedRegion contains the region that will be repainted in the next frame, so we don't need to accumulate surface damages again. I suppose we want something like const QRegion region = damagedRegion & output.output->geometry(); zzag: `damagedRegion` contains the region that will be repainted in the next frame, so we don't need… | |||||
zzag: This comment hasn't been addressed. | |||||
| 446 | if(m_remoteaccessManager && gbm_surface_has_free_buffers(output.gbmSurface->surface())) { | 466 | if(m_remoteaccessManager && gbm_surface_has_free_buffers(output.gbmSurface->surface())) { | ||
| 447 | // GBM surface is released on page flip so | 467 | // GBM surface is released on page flip so | ||
| 448 | // we should pass the buffer before it's presented. | 468 | // we should pass the buffer before it's presented. | ||
According to the spec, eglSetDamageRegionKHR() handles the case where n_rects is 0, so we can drop rects.isEmpty() check and thus simplify code a bit more. zzag: According to the spec, eglSetDamageRegionKHR() handles the case where n_rects is 0, so we can… | |||||
| 449 | m_remoteaccessManager->passBuffer(output.output, output.buffer); | 469 | m_remoteaccessManager->passBuffer(output.output, output.buffer); | ||
| 450 | } | 470 | } | ||
| 451 | 471 | | |||
| 452 | m_backend->present(output.buffer, output.output); | 472 | m_backend->present(output.buffer, output.output); | ||
Use categorized logging please. Also, it's not a critical error if eglSetDamageRegionKHR() fails. qCWarning(KWIN_DRM) would be a better choice. zzag: Use categorized logging please. Also, it's not a critical error if eglSetDamageRegionKHR()… | |||||
| 453 | 473 | | |||
| 454 | if (supportsBufferAge()) { | 474 | if (supportsBufferAge()) { | ||
| 455 | eglQuerySurface(eglDisplay(), output.eglSurface, EGL_BUFFER_AGE_EXT, &output.bufferAge); | 475 | eglQuerySurface(eglDisplay(), output.eglSurface, EGL_BUFFER_AGE_EXT, &output.bufferAge); | ||
| 456 | } | 476 | } | ||
| 457 | } | 477 | } | ||
| 458 | 478 | | |||
| 459 | void EglGbmBackend::screenGeometryChanged(const QSize &size) | 479 | void EglGbmBackend::screenGeometryChanged(const QSize &size) | ||
| 460 | { | 480 | { | ||
| Show All 17 Lines | 497 | { | |||
| 478 | const QSize &overall = screens()->size(); | 498 | const QSize &overall = screens()->size(); | ||
| 479 | const QRect &v = output.output->geometry(); | 499 | const QRect &v = output.output->geometry(); | ||
| 480 | qreal scale = output.output->scale(); | 500 | qreal scale = output.output->scale(); | ||
| 481 | 501 | | |||
| 482 | glViewport(-v.x() * scale, (v.height() - overall.height() + v.y()) * scale, | 502 | glViewport(-v.x() * scale, (v.height() - overall.height() + v.y()) * scale, | ||
| 483 | overall.width() * scale, overall.height() * scale); | 503 | overall.width() * scale, overall.height() * scale); | ||
| 484 | } | 504 | } | ||
| 485 | 505 | | |||
| 486 | QRegion EglGbmBackend::prepareRenderingForScreen(int screenId) | 506 | QRegion EglGbmBackend::prepareRenderingForScreen(int screenId) | ||
| 487 | { | 507 | { | ||
| 488 | const Output &output = m_outputs.at(screenId); | 508 | const Output &output = m_outputs.at(screenId); | ||
| 489 | 509 | | |||
| 490 | makeContextCurrent(output); | 510 | makeContextCurrent(output); | ||
| 491 | prepareRenderFramebuffer(output); | 511 | prepareRenderFramebuffer(output); | ||
| 492 | setViewport(output); | 512 | setViewport(output); | ||
| 493 | 513 | | |||
| 494 | if (supportsBufferAge()) { | 514 | if (supportsBufferAge()) { | ||
| 495 | QRegion region; | 515 | QRegion region; | ||
| 496 | 516 | | |||
| 497 | // Note: An age of zero means the buffer contents are undefined | 517 | // Note: An age of zero means the buffer contents are undefined | ||
| 498 | if (output.bufferAge > 0 && output.bufferAge <= output.damageHistory.count()) { | 518 | if (output.bufferAge > 0 && output.bufferAge <= output.damageHistory.count()) { | ||
I think we can drop this method now since only EglGbmBackend::prepareRenderingForScreen() needs to accumulate damage. zzag: I think we can drop this method now since only EglGbmBackend::prepareRenderingForScreen() needs… | |||||
I think next person who reads the code will prefer to see it encapsulated. apol: I think next person who reads the code will prefer to see it encapsulated.
| |||||
Currently pendingDamageRegions() is used only in two places - aboutToStartPainting() and prepareRenderingForScreen(). There is no reason to accumulate surface damages _again_ in aboutToStartPainting(). After the second usage of pendingDamageRegions() is dropped, the introduction of pendingDamageRegions() will look unrelated. zzag: Currently pendingDamageRegions() is used only in two places - aboutToStartPainting() and… | |||||
| 499 | for (int i = 0; i < output.bufferAge - 1; i++) | 519 | for (int i = 0; i < output.bufferAge - 1; i++) | ||
| 500 | region |= output.damageHistory[i]; | 520 | region |= output.damageHistory[i]; | ||
| 501 | } else { | 521 | } else { | ||
| 502 | region = output.output->geometry(); | 522 | region = output.output->geometry(); | ||
| 503 | } | 523 | } | ||
| 504 | 524 | | |||
| 525 | if (output.bufferAge > 0 | ||||
| 526 | && !region.isEmpty() | ||||
| 527 | && supportsPartialUpdate()) | ||||
| 528 | { | ||||
| 529 | // https://www.khronos.org/registry/EGL/extensions/KHR/EGL_KHR_partial_update.txt | ||||
This is the region in the buffer which must be repaired, i.e. the damage from the previous frames. Shouldn't we also include the current damage region? zzag: This is the region in the buffer which must be repaired, i.e. the damage from the previous… | |||||
| 530 | | ||||
| 531 | QVector<EGLint> rects = regionToRects(region, | ||||
| 532 | output.output->geometry().height()); | ||||
zzag: IIRC, it is okay to set n_rects to 0 so we probably can drop this check. | |||||
| 533 | | ||||
| 534 | bool correct = rects.isEmpty() | ||||
| 535 | || eglSetDamageRegionKHR(eglDisplay(), output.eglSurface, | ||||
| 536 | rects.data(), rects.count()/4); | ||||
| 537 | if (!correct) { | ||||
| 538 | qCritical() << "failed eglSetDamageRegionKHR" << eglGetError(); | ||||
| 539 | } | ||||
| 540 | } | ||||
| 541 | | ||||
| 542 | | ||||
| 505 | return region; | 543 | return region; | ||
| 506 | } | 544 | } | ||
| 507 | return QRegion(); | 545 | return QRegion(); | ||
| 508 | } | 546 | } | ||
| 509 | 547 | | |||
| 510 | void EglGbmBackend::endRenderingFrame(const QRegion &renderedRegion, const QRegion &damagedRegion) | 548 | void EglGbmBackend::endRenderingFrame(const QRegion &renderedRegion, const QRegion &damagedRegion) | ||
| 511 | { | 549 | { | ||
| 512 | Q_UNUSED(renderedRegion) | 550 | Q_UNUSED(renderedRegion) | ||
| ▲ Show 20 Lines • Show All 67 Lines • Show Last 20 Lines | |||||
s/lastRegion/region/