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/ocl: Improve Linux CL/GL sharing support #673

Closed
wants to merge 25 commits into from

Conversation

smunaut
Copy link
Contributor

@smunaut smunaut commented Aug 25, 2023

This PR is aimed at drastically improving the support for the CL/GL sharing extension on linux.
The current support is not really usable as it only supports a few texture format, and only on EGL contexts. It is also pretty buggy since it requires the texture to be bound when placing the CL call to share it which is just plain wrong and will not work in many applications.
This new version makes used of the "official" interop extension from MESA which is available for GLX and EGL contexts, allows sharing of buffers and not just texture and supports many more formats.
This is still far from being a fully compliant / full featured version of the extension, but it's a big step forward in my opinion and allows to run some real applications. I've tested gr-fosphor (SDR spectrum display) and Davinci Resolve as examples. Both of theses don't work without theses improvements.

Resolves: #659
Resolves: #667

Signed-off-by: Sylvain Munaut [email protected]

@k1gen
Copy link

k1gen commented Aug 25, 2023

I have tested this on my machine, and the app I wanted to work (DaVinci Resolve) - worked flawlessly on UHD620!

@smunaut
Copy link
Contributor Author

smunaut commented Sep 6, 2023

So any chance someone from intel can review / comment or just general advise on what is required to get this merged ?

opencl/source/sharings/gl/linux/gl_buffer_linux.cpp Outdated Show resolved Hide resolved
opencl/source/sharings/gl/linux/gl_buffer_linux.cpp Outdated Show resolved Hide resolved
opencl/source/sharings/gl/linux/gl_buffer_linux.cpp Outdated Show resolved Hide resolved
opencl/source/sharings/gl/linux/gl_buffer_linux.cpp Outdated Show resolved Hide resolved
opencl/source/sharings/gl/linux/gl_context_guard_linux.cpp Outdated Show resolved Hide resolved
opencl/source/sharings/gl/linux/gl_sync_event_linux.cpp Outdated Show resolved Hide resolved
opencl/source/sharings/gl/linux/gl_texture_linux.cpp Outdated Show resolved Hide resolved
opencl/source/sharings/gl/linux/gl_texture_linux.cpp Outdated Show resolved Hide resolved
opencl/source/sharings/gl/linux/gl_texture_linux.cpp Outdated Show resolved Hide resolved
opencl/source/sharings/gl/linux/gl_texture_linux.cpp Outdated Show resolved Hide resolved
@smunaut
Copy link
Contributor Author

smunaut commented Sep 7, 2023

Thanks for the review, I'll try to address these next week.

@smunaut smunaut force-pushed the clgl branch 2 times, most recently from e1f390d to b1149c2 Compare October 2, 2023 14:32
@smunaut
Copy link
Contributor Author

smunaut commented Oct 2, 2023

@JablonskiMateusz Ok, so it took me a while longer to finally get to it, but I have addressed the comments above, rebased over master (and also tested only the 23.30 release) and seems to all be fine.

@@ -10,29 +10,8 @@

namespace NEO {
GLContextGuard::GLContextGuard(GLSharingFunctions &sharingFcns) : sharingFunctions(&sharingFcns) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this class still needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's used by the "common code" shared between linux / windows. The alternative would be #ifdef in the common code but I wanted to avoid touching windows / common code as much as possible.

glDeviceHandle = contextInfo.deviceHandle;

return retVal;
std::unique_ptr<OsLibrary> dynLibrary(OsLibrary::load(""));;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(1) - I know that we cannot link using load-time linking as GL sharing is optional but what is an issue with run-time linking by name? When user is using GL interop then GL library need to be available in the system
(2) I totally agree, we cannot use GL symbols directly
(3) This approach assumes that GL is loaded before OCL is loaded but I can imagine a scenario where CL is initialized before GL is even loaded and in such case I hope should try to load GL to determine if sharing is available.

opencl/source/sharings/gl/linux/gl_buffer_linux.cpp Outdated Show resolved Hide resolved
@smunaut
Copy link
Contributor Author

smunaut commented Oct 9, 2023

@JablonskiMateusz Responding here for the libGL thing because for some reason, github doesn't me put a reply to that specific comment 🤷

(1) Because for whatever reason the user is free to have loaded (through whatever mechanism) any GL library and possibly not the first one that would be returned by a normal libdl lookup of libGL.so.0. Then there is also the EGL/GLX thing where we have no idea a-priori which one the user will want to use and we don't want to force loading the wrong one in the same address space, so it's easier to lookup what the user chose by looking at already available symbols.

(3) In the initGLFunctions it's not an issue because this is called when creating the CL context but at that point the user must have given a reference to the current GL context in the parameter list which means that GL must have been loaded already.

In the isEnabled case (to decide wether we show the extension or not), it's less clear. All the functionality of the extensions depend on a GL context being present first so to me it make sense to advertise it only if GL is loaded. But we could just as well just return true all the time and leave it up to the user to actually make sure it gets a valid GL context in time before trying to actually make use of it.

@JablonskiMateusz
Copy link
Contributor

@JablonskiMateusz Responding here for the libGL thing because for some reason, github doesn't me put a reply to that specific comment 🤷

(1) Because for whatever reason the user is free to have loaded (through whatever mechanism) any GL library and possibly not the first one that would be returned by a normal libdl lookup of libGL.so.0. Then there is also the EGL/GLX thing where we have no idea a-priori which one the user will want to use and we don't want to force loading the wrong one in the same address space, so it's easier to lookup what the user chose by looking at already available symbols.

(3) In the initGLFunctions it's not an issue because this is called when creating the CL context but at that point the user must have given a reference to the current GL context in the parameter list which means that GL must have been loaded already.

In the isEnabled case (to decide wether we show the extension or not), it's less clear. All the functionality of the extensions depend on a GL context being present first so to me it make sense to advertise it only if GL is loaded. But we could just as well just return true all the time and leave it up to the user to actually make sure it gets a valid GL context in time before trying to actually make use of it.

We cannot assume that GL is already loaded. Where does that requirement come from? We need to try to open gl library in order to expose the extension - that's our baseline.
If GL library was already loaded then we will just increase ref count instead of loading or reloading it. It will be used in the same form as was already loaded.

@smunaut
Copy link
Contributor Author

smunaut commented Oct 11, 2023

But we don't know which GL library ... it's not our choice.

It's be 100% valid for a user to do dlopen("/path/to/some/custom/libGL.so.1234", RTLD_GLOBAL); to load a GL library that's not the system default one ... So we can't hard code any names or path anywhere.

If you don't want to assume GL is already loaded to return the extension, then I would return it all the time. Which from reading the extension spec is perfectly valid since all that's needed is that there is some way/combination of GL context/device for which we could support it.

@eero-t
Copy link

eero-t commented Oct 11, 2023

Btw. @smunaut have you looked (e.g. latrace'd) what Nvidia and AMD OpenCL drivers do?

@smunaut
Copy link
Contributor Author

smunaut commented Oct 11, 2023

@eero-t

AMD looksup symbols in the global scope without loading anything it self, you can see it here :

https://github.com/ROCm-Developer-Tools/clr/blob/5914ac3c6e9b3848023a7fa25e19e560b1c38541/rocclr/device/rocm/rocglinterop.cpp#L71C38-L71C69

(Now, here they lookup MesaGLInteropGLXExportObject directly which is "the old way" of doing it because until recently that was the only way since glxGetProcAddress wouldn't support that symbol but it was shown that this direct method didn't work when using EGL + GLVND so mesa added those interop symbols to the normal GetProcAddress ... but it's the same idea, they search in the global scope what's already there and don't load anything themselves).

The AMD implementation always advertises the extension, it's statically present in the list of supported extension and it doesn't do any checks before it returns it AFAICT. ( https://github.com/ROCm-Developer-Tools/clr/blob/5914ac3c6e9b3848023a7fa25e19e560b1c38541/rocclr/device/device.hpp#L204 )

In Mesa's rusticl they also just search the global scope : https://gitlab.freedesktop.org/mesa/mesa/-/commit/02f7d3640021f23d54022c6fbdf1304c44672033#cb331a47f57adfd7e2565e461a25af9ec00ff8e6_0_32
They call dlsym with a NULL first argument to just lookup in what's already loaded rather than any form of explicit linking.
However contrary to AMD's they do not return the extension if GL is not loaded already.

As for NVIDIA, I'm not sure, no sources I can look at ... I'd need to try it.

@JablonskiMateusz
Copy link
Contributor

I think we may self load in this case. Extension can be exposed always but to work with sharing GL library needs to be loaded.

@smunaut smunaut changed the title [draft] Improve Linux CL/GL sharing support Improve Linux CL/GL sharing support Nov 22, 2023
@smunaut
Copy link
Contributor Author

smunaut commented Nov 22, 2023

  • Rebased to master
  • Removed the isGLSharingEnabled check and always advertise extension
  • Adapted to the final version of the mesa interop interface that is now part of git master on the mesa repo.

@JablonskiMateusz
Copy link
Contributor

@smunaut please adjust commit msg to https://github.com/intel/compute-runtime/blob/master/CONTRIBUTING.md#commit-message-guidelines I think squash to single commit would be needed

@smunaut
Copy link
Contributor Author

smunaut commented Nov 23, 2023

Is squashing really needed ? It makes the diff really hard to read and also means I can't put explanation of why a particular change was done that way in the commit message since it's all globbed up into a giant unreadable diff.

All the commits would belong in feature/ocl AFAICT.

@JablonskiMateusz
Copy link
Contributor

it will be squashed in the end but we can do this on our side. Let's keep it here without squashing. However, I got some errors when applying the PR:

../../../opencl/source/sharings/gl/linux/gl_buffer_linux.cpp:225:51: error: no matching constructor for initialization of 'NEO::Gmm'
graphicsAllocation->setDefaultGmm(new Gmm(helper,
^ ~~~~~~~
../../../shared/source/gmm_helper/gmm.h:45:5: note: candidate constructor not viable: requires 7 arguments, but 8 were provided
Gmm(GmmHelper *gmmHelper, const void *alignedPtr, size_t alignedSize, size_t alignment,
^
../../../shared/source/gmm_helper/gmm.h:44:5: note: candidate constructor not viable: requires 4 arguments, but 8 were provided
Gmm(GmmHelper *gmmHelper, ImageInfo &inputOutputImgInfo, const StorageInfo &storageInfo, bool preferCompressed);
^
../../../shared/source/gmm_helper/gmm.h:48:5: note: candidate constructor not viable: requires 3 arguments, but 8 were provided
Gmm(GmmHelper *gmmHelper, GMM_RESOURCE_INFO *inputGmm, bool openingHandle);
^
../../../shared/source/gmm_helper/gmm.h:47:5: note: candidate constructor not viable: requires 2 arguments, but 8 were provided
Gmm(GmmHelper *gmmHelper, GMM_RESOURCE_INFO *inputGmm);
^
../../../shared/source/gmm_helper/gmm.h:40:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 8 were provided
class Gmm {
^
../../../shared/source/gmm_helper/gmm.h:43:5: note: candidate constructor not viable: requires 0 arguments, but 8 were provided
Gmm() = delete;
^
1 error generated.

Also, when fixed the issue locally I got test failures:

97x TEST_F(GlSharingTextureTests, givenMockGlWhen2dGlTextureIsCreatedThenMemObjectHasGlHandler) {
98x cl_int retVal = CL_INVALID_VALUE;
99x
100x glSharing->uploadDataToTextureInfo(textureId);
101x
102t> auto glTexture = GlTexture::createSharedGlTexture(clContext.get(), (cl_mem_flags)0, GL_TEXTURE_2D, 0, textureId, &retVal);
103x ASSERT_NE(nullptr, glTexture);

46x texIn.version = 2;
47x texIn.target = getBaseTargetType(target);
48x texIn.obj = texture;
49x texIn.miplevel = miplevel;
50x
51x switch (flags) {
52x case CL_MEM_READ_ONLY:
53x texIn.access = MESA_GLINTEROP_ACCESS_READ_ONLY;
54x break;
55x case CL_MEM_WRITE_ONLY:
56x texIn.access = MESA_GLINTEROP_ACCESS_WRITE_ONLY;
57x break;
58x case CL_MEM_READ_WRITE:
59x texIn.access = MESA_GLINTEROP_ACCESS_READ_WRITE;
60x break;
61x default:
62t> errorCode.set(CL_INVALID_VALUE);
63x return nullptr;
64x }

Could you please ensure that this code is working after rebase?

@smunaut
Copy link
Contributor Author

smunaut commented Jan 24, 2024

I'm doing what I can to test but again, I'm trying to rebase to master and build here but I just can't ... master just doesn't build and it ends with ocloc segfaulting currently, so there isn't much testing I can do.

I can build 23.48.27912.11 so currently that's the most recent thing I could rebase and test onto. Both 23.52.28202.13 and master don't build for me.

@smunaut
Copy link
Contributor Author

smunaut commented Jan 26, 2024

Ok, I managed to build master on my machine (seems there is a requirement for some specific version of dependencies that are not checked by the build system).

So I rebased and checked it build. I added a couple of small fixes and removed non-applicable broken tests.

Running make run_adlp_0_ocl_tests passes without error. I can't run all the tests because some of them fail on my machine even without this patch and the testing stops at the first failure ... But this specific test target should check everything that is affected by this patch set.

I didn't change the commit message since if you squash on your side you'll have to make a new one anyway so at that point you can use the feature/ocl prefix for it at that time.

- They're broken. The eglCreateContext doesn't even have the
  right prototype, attempting to call it would crash

- They're unused. If at some point we need them, we can re-implement
  them propertly

- They increase dependency on EGL which makes adding GLX support
  harder for nothing

Signed-off-by: Sylvain Munaut <[email protected]>
Unused, no equivalent on linux anyway

Signed-off-by: Sylvain Munaut <[email protected]>
Those don't exist on linux and won't exist, so no point in
keeping these around.

Signed-off-by: Sylvain Munaut <[email protected]>
…sLinux

This is not used and OS_HANDLE can't hold anything anyway since
it's 32 bits.

Signed-off-by: Sylvain Munaut <[email protected]>
This is just the header and function prototypes and wrapper so far.

The interop header comes from MESA repository at
https://gitlab.freedesktop.org/mesa/mesa
and can be found under mesa/include/GL/mesa_glinterop.h

It has been stripped of the WGL prototypes since we don't need
those here and they cause a type conflict in this project.

Signed-off-by: Sylvain Munaut <[email protected]>
Those are not required anymore

Signed-off-by: Sylvain Munaut <[email protected]>
This is not used and wouldn't be a good idea to call it anyway since
we can't rely/modify the GL bind context anyway

Signed-off-by: Sylvain Munaut <[email protected]>
The debug flag is -1 when not specified so previously it would
enable them all the time, no matter what isGlSharingEnabled() returns

Signed-off-by: Sylvain Munaut <[email protected]>
Check if 'glEnable' is in the global scope.
This would indicate a GL library has been loaded by
the application and that it's worth doing CL/GL interop.

We can't really test further because we don't know if user
wants GLX or EGL or if those extensions are supported ...

Signed-off-by: Sylvain Munaut <[email protected]>
In a CL/GL sharing application we can't go and load another GL library
explicitely, it might not be the one the user has used.

But we know that whatever the user wants to share with, it has to
be loaded already when he requests the context, so we can just
lookup the symbols in the global scope.

Signed-off-by: Sylvain Munaut <[email protected]>
This seems to be re-used and the windows equivalent resets it after
finalize.

Signed-off-by: Sylvain Munaut <[email protected]>
We always consider it to be enabled.

Signed-off-by: Sylvain Munaut <[email protected]>
Theses were heavily reliant on the old implementation and don't test
anything of value. (CL/GL sharing is completely broken yet the test
pass ...). So get rid of that junk.

Signed-off-by: Sylvain Munaut <[email protected]>
We need to wait for sync to actually be done

Signed-off-by: Sylvain Munaut <[email protected]>
@smunaut
Copy link
Contributor Author

smunaut commented Feb 16, 2024

Just rebased on top of master again.
Checked that it builds, works in a coupld of applications, and that make run_adlp_0_ocl_tests works too.

@smunaut
Copy link
Contributor Author

smunaut commented Feb 29, 2024

👋

@JablonskiMateusz
Copy link
Contributor

Hi @smunaut
I'll try merge it but I need additional input from your side.
Please update PR title and PR description to meet our commit msg restrictions https://github.com/intel/compute-runtime/blob/master/CONTRIBUTING.md#commit-message-guidelines

@smunaut smunaut changed the title Improve Linux CL/GL sharing support feature/ocl: Improve Linux CL/GL sharing support Mar 1, 2024
@smunaut
Copy link
Contributor Author

smunaut commented Mar 1, 2024

@JablonskiMateusz I've updated the PR title and description to match those guidelines. I didn't update the commits themselves since AFAIR you said you would squash them all together and I assume the final commit title/description will be taken from the PR.

@smunaut
Copy link
Contributor Author

smunaut commented Mar 1, 2024

Merged outside of github.

Thanks @JablonskiMateusz

@smunaut smunaut closed this Mar 1, 2024
@JablonskiMateusz JablonskiMateusz added the merged change was merged label Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged change was merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CL/GL sharing support more texture formats cl_khr_gl_sharing GLX support
4 participants