Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature] OpenCL support #7451

Merged
merged 86 commits into from Aug 8, 2018
Merged

[feature] OpenCL support #7451

merged 86 commits into from Aug 8, 2018

Conversation

@elpaso
Copy link
Contributor

elpaso commented Jul 23, 2018

OpenCL processing

This is the implementation of QEP 121: qgis/QGIS-Enhancement-Proposals#121 that was accepted as a GRANT candidate.

Available algorithms

The following processing algorithms have been ported:

  • slope
  • aspect
  • hillshade
  • ruggedness

Even if was not in scope for this QEP, the hillshade renderer has been optimized and can also benefit of OpenCL acceleration.

Performance improvements

I tested this implementation on 2 different machines (with Linux on both):

  • machine 1:
    • CPU: AMD Ryzen 7 1700
    • GPU: AMD Radeon HD 7700 Series
  • machine 2:
    • CPU: Intel Core i7-4770HQ
    • GPU: integrated Intel HD Graphics

Note that on machine 2, the intel OpenCL runtime identifies two different OpenCL devices, one is the CPU and the other is the GPU, while the AMD OpenCL runtime only identifies the GPU as an OpenCL device on machine 1.

The tests have been run by comparing two different algorithms (slope and hillshade) on three different implementations from the QGIS processing toolbox:

  • QGIS without OpenCL (current behavior in 3.3)
  • GDAL (2.2.2)
  • QGIS with OpenCL (on machine 2 only, this was executed on both OpenCL devices)

The input raster is a 40000x40000 DEM float 32: eu_dem_v11_E40N20 from copernicus.

opencl_speed_comparison

Building and configuration

A new CMake option (enabled by default) will build QGIS with OpenCL support, (opencl dev headers need to be installed on the building machine).

To run OpenCL, you need to install:

  • OpenCL library for your platform (this is O.S. specific)
  • one or more OpenCL runtimes (this is O.S. and hardware vendor specific)

Note that OpenCL in QGIS is opt-in: you need to enable it in the QGIS options dialog as shown below:

immagine

Selecting a different OpenCL device

If you happen to have more than one OpenCL device on your system you will notice more than one entry in the a.m. configuration dialog, please note that if you change the device QGIS needs to be restarted in order to pick up the new settings.

@@ -28,6 +28,18 @@ MESSAGE(STATUS "QGIS version: ${COMPLETE_VERSION} ${RELEASE_NAME} (${QGIS_VERSIO


#############################################################
# Configure OpenCL if available

option(USE_OPENCL "Use OpenCL" ON)

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Jul 23, 2018

Member

Can cmake commands be capitalized (just for consistency within the cmake files)

@@ -34,7 +35,8 @@ Starts the calculation, reads from mInputFile and stores the result in mOutputFi

:param feedback: feedback object that receives update and that is checked for cancelation.

:return: 0 in case of success*
:return: 0 in case of success
TODO: return an enum

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Jul 23, 2018

Member

For when is this todo planned?

This comment has been minimized.

Copy link
@elpaso

elpaso Jul 23, 2018

Author Contributor

No idea, it is not related to this PR and I've not the necessary kowledge to to do it quickly. I just wanted to note it. I'll remove the TODO if you prefer.

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Jul 23, 2018

Member

Not really.
I just wondered if it's API related and therefore the point in time is QGIS 4 (in which case I think it should be tagged appropriately and can remain in the API docs) or who will take care of this at what point in time. In the second case I think there's no point in tagging this in doc-visible way.

This comment has been minimized.

Copy link
@elpaso

elpaso Jul 23, 2018

Author Contributor

That code is API and it is quite old, I think we should file an issue for QGIS 4 to fix this, and I didn't really want to make it land in the docs, I just wanted to note that it is something that should be eventually fixed. BTW, there is no functional change but it's just code style to return a human-readable meaningful enum instead of a meaningless int.

* \param row row index
* \param column column index
* \returns true if value is no data */
bool isNoData( qgssize row, qgssize column )

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Jul 23, 2018

Member

const?

* \see init()
* \see available()
*/
static bool activate( const QString preferredDeviceId = QString() );

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Jul 23, 2018

Member

const QString &


#else

mOptionsListWidget->removeItemWidget( mOptionsListWidget->findItems( QStringLiteral( "GPU" ), Qt::MatchExactly ).first() );

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Jul 23, 2018

Member

That is the visual text, correct? Won't this break on translated builds?

This comment has been minimized.

Copy link
@elpaso

elpaso Jul 23, 2018

Author Contributor

It is actually already broken :) Thanks for noting

else
{
mGPUEnableCheckBox->setEnabled( false );
mGPUInfoTextBrowser->setText( QStringLiteral( "An OpenCL compatible device was not found on your system.<br>"

This comment has been minimized.

Copy link
@m-kuhn

#ifdef HAVE_OPENCL
// Setup OpenCL source path
// TODO: cmd line switch and env var

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Jul 23, 2018

Member

For when is this TODO planned?


/**
* \brief processRasterGPU executes the computation on the GPU
* \param feedback

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Jul 23, 2018

Member

Missing description for the parameter source which is harder to know what it's about than feedback - which is also missing content ;)

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Jul 23, 2018

Nice one 🎉

@elpaso elpaso force-pushed the elpaso:opencl-utils-2 branch from 29c4ed6 to b9d08b3 Jul 23, 2018
@nyalldawson nyalldawson added the Feature label Jul 24, 2018
@nyalldawson nyalldawson added this to the 3.4 milestone Jul 24, 2018
Copy link
Contributor

nyalldawson left a comment

Looks great to me!

{
try
{
QgsMessageLog::logMessage( QObject::tr( "Running OpenCL program: %1" )

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Jul 28, 2018

Contributor

I think this should be a QgsDebugMsg instead of a message log - it seems more of a debug/testing feedback to me


void QgsOpenClUtils::init()
{
static std::once_flag initialized;

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Jul 28, 2018

Contributor

This is nice - we should use this more!

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Jul 29, 2018

This looks good to go for me. I think given the nature of the work though another review would be valuable. @wonder-sk @mhugo ?

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Aug 7, 2018

@elpaso

Let's merge?

@elpaso elpaso force-pushed the elpaso:opencl-utils-2 branch from 6ab1f01 to 44adfe5 Aug 8, 2018
@@ -477,7 +473,7 @@ IF (PEDANTIC)
# ADD_DEFINITIONS( -fstrict-aliasing -Wstrict-aliasing=1 -Wredundant-decls )

IF ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-return-type-c-linkage -Wno-overloaded-virtual -Wimplicit-fallthrough")
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-return-type-c-linkage -Wno-overloaded-virtual")

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Aug 8, 2018

Member

Is that done on purpose?

This comment has been minimized.

Copy link
@elpaso

elpaso Aug 8, 2018

Author Contributor

no: rebase issue

@elpaso elpaso merged commit 0b502ff into qgis:master Aug 8, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elpaso

This comment has been minimized.

Copy link
Contributor Author

elpaso commented Aug 8, 2018

Thank you guys for the review!

@elpaso elpaso deleted the elpaso:opencl-utils-2 branch Aug 8, 2018
@nirvn

This comment has been minimized.

Copy link
Contributor

nirvn commented Aug 10, 2018

@elpaso , I can't get to compile QGIS with OpenCL enabled on my machine (Ubuntu 18.04, NVIDIA driver), I get the following error:

In file included from /home/webmaster/dev/cpp/QGIS/src/core/raster/qgshillshaderenderer.cpp:33:0:
/home/webmaster/dev/cpp/QGIS/src/core/qgsopenclutils.h:24:10: fatal error: CL/cl2.hpp: No such file or directory
 #include <CL/cl2.hpp>
          ^~~~~~~~~~~~
@nirvn

This comment has been minimized.

Copy link
Contributor

nirvn commented Aug 10, 2018

@elpaso , OK, the opencl-clhpp-headers package is required. Maybe it'd be worth updating the INSTALL file to mention this dependency?

@elpaso

This comment has been minimized.

Copy link
Contributor Author

elpaso commented Aug 10, 2018

@nirvn which distribution? I don't have that package on xenial. The package that provides that header is opencl-headers on xenial.

@jef-n

This comment has been minimized.

Copy link
Member

jef-n commented Aug 10, 2018

See also 6e30c62

@nirvn

This comment has been minimized.

Copy link
Contributor

nirvn commented Aug 10, 2018

@elpaso , I'm on Ubuntu 18.04 LTS. Ubuntu's packages website indicates this is a new package since 18.04: https://packages.ubuntu.com/search?keywords=opencl-clhpp-headers

@nirvn

This comment has been minimized.

Copy link
Contributor

nirvn commented Aug 10, 2018

@elpaso , @jef-n , on debian, the package is also there starting with stretch: https://packages.debian.org/search?keywords=opencl-clhpp-headers

@jef-n

This comment has been minimized.

Copy link
Member

jef-n commented Aug 10, 2018

See also af075bf (opencl-headers depends on opencl-c-headers and opencl-clhpp-headers)

@nirvn

This comment has been minimized.

Copy link
Contributor

nirvn commented Aug 10, 2018

@jef-n , great, thanks.

@anitagraser anitagraser mentioned this pull request Jan 4, 2020
6 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.