Use native dawn dev package for development #338
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
huningxin
reviewed
Jun 9, 2020
There was a problem hiding this comment.
@huningxin , rebase the gsoc_2020_gpu to master
@NALLEIN , please take a look at comments and add a standalone dawn sample into this PR.
| } | ||
| // queue submit succeed | ||
| fence.Release(); | ||
| } |
There was a problem hiding this comment.
Please create a standalone simple dawn example to exercise the dawn API usages here.
|
@NALLEIN , please change "base" branch of this PR: master => gsoc_2020_gpu (use "Edit" button near PR title) |
done |
|
@NALLEIN , please provide the build instructions for this PR. |
|
@NALLEIN , please refer to https://github.com/huningxin/opencv/blob/gsoc_2020_gpu/modules/dnn/test/test_layers.cpp#L218 for softmax layer test. |
|
There are two tests to be added:
|
|
@NALLEIN , I tried your branch with build instructions as > cmake -D CMAKE_BUILD_TYPE=Release -DWITH_WEBGPU=ON -D CMAKE_INSTALL_PREFIX=/usr/local ..
> make -j8It reports error when compiling I am using Ubuntu 16.04 with gcc 5.4.0. I can build vanilla opencv successfully. |
Did you try to compile on branch dev2? I can make successfully on branch dev2. Please run this command |
The repo dev2 can be built successfully on branch dev2. You can execute the following command to compile. |
|
I tried |
Sorry, I've updated this PR,. |
added 2 commits
June 28, 2020 14:42
I made a unit test. It seems that the API wrapper of dawn in dnn/src/webgpu dictionary can work normally. I will modify the module that reads wgpu::Buffer asynchronously. |
wzw-intel
reviewed
Jul 10, 2020
| Format fmt = wFormatInt32); //uniform buffer | ||
| Tensor( const void* data, std::vector<int>& shape, | ||
| wgpu::BufferUsage usage, | ||
| Tensor( const void* data, size_t size_in_byte); //uniform buffer |
There was a problem hiding this comment.
Missing argument "Format fmt" ?
There was a problem hiding this comment.
It seems that format is not needed in uniform buffer.
wzw-intel
reviewed
Jul 10, 2020
| if (!buffer_) | ||
| { | ||
| // TODO: specific data type | ||
| buffer_.reset(new Buffer(device_, data, size_in_byte_, usage_)); | ||
| } |
There was a problem hiding this comment.
if ( size > size_in_byte_ ) , don't fill, but warning and return
There was a problem hiding this comment.
I've modified it, but we just set size_in_byte_ (using the alloc flag to reset buffer) before using fillData, do we need to check again？
There was a problem hiding this comment.
ok, ignore this comment.
wzw-intel
reviewed
Jul 10, 2020
| sizeof(SoftmaxParam), | ||
| wgpu::BufferUsage::Uniform | wgpu::BufferUsage::CopyDst, | ||
| wFormatInt32); | ||
| sizeof(SoftmaxParam)); |
There was a problem hiding this comment.
Oh, I get your reason why to specify usage flag in the constructor's argument.
However, Tensor is a concept on the context of dnn network or more generally, "computation graph". The uniform buffer is an optimization method specific to a library. e.g. webgpu. I suggest to use webgpu::Buffer object directly for uniform parameters in your operator's implementation. So that we can simply keep the Tensor a read/write ndarray with unified usage ""wgpu::BufferUsage::Storage | wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::CopySrc""
There was a problem hiding this comment.
Thanks, can I use different Tensor constructors to omit buffer usage?
@wzw-intel
There was a problem hiding this comment.
No need to add usage parameter for Tensor constructor, since Tensor object's usage is always "wgpu::BufferUsage::Storage | wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::CopySrc"
There was a problem hiding this comment.
Made corresponding changes, PTAL. Sorry for the delay. @wzw-intel
added 2 commits
July 13, 2020 02:20
wzw-intel
reviewed
Jul 13, 2020
| wgpu::BufferUsage usage = wgpu::BufferUsage::Storage); | ||
|
|
||
| wgpu::BufferUsage usage = wgpu::BufferUsage::Storage | | ||
| wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopySrc ); |
There was a problem hiding this comment.
typo: duplicated wgpu::BufferUsage::CopySrc
wzw-intel
reviewed
Jul 13, 2020
| @@ -14,8 +14,10 @@ class Buffer | |||
| Buffer(const std::shared_ptr<wgpu::Device> device); | |||
| Buffer( const std::shared_ptr<wgpu::Device> device, | |||
There was a problem hiding this comment.
unnecessary blank in "Buffer( const"
|
code is ok for me. BTW, please fix the coding style issues |
Thanks, done. Sorry for the delay caused by graduation-related matters last week. PTAL. @wzw-intel |
wzw-intel
reviewed
Jul 14, 2020
| const void* data, size_t size, | ||
| wgpu::BufferUsage usage = wgpu::BufferUsage::Storage | | ||
| wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopySrc ); | ||
| wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::CopySrc ); |
There was a problem hiding this comment.
Unneccesary blank in "wgpu::BufferUsage::CopySrc );", incorrect indent for the parameters list.
Please check coding style conformance for all source files.
https://github.com/opencv/opencv/wiki/Coding_Style_Guide
There was a problem hiding this comment.
Thanks, I've fix the coding style issue.
wzw-intel
reviewed
Jul 16, 2020
| @@ -12,7 +12,7 @@ class Buffer; | |||
| class Tensor{ | |||
| public: | |||
| Tensor(Format fmt = wFormatFp32); | |||
| Tensor( const void* data, std::vector<int>& shape, | |||
| Tensor(const void* data, std::vector<int>& shape, | |||
| Format fmt = wFormatFp32); | |||
There was a problem hiding this comment.
plase correct the wrong indents of paramters, not only for this function.
There was a problem hiding this comment.
plase correct the wrong indents of paramters, not only for this function.
Thanks for your patience, I've modified it. By the way, I've add the layer test for softmax in this repo, please help to check whether it works on your device.
git clone https://github.com/NALLEIN/opencv.git
cd opencv/
git checkout -b layerTest origin/layerTest
mkdir build
cd build
cmake -D CMAKE_BUILD_TYPE=Release -DWITH_WEBGPU=ON -D CMAKE_INSTALL_PREFIX=/usr/local ..
make -j8
$(PATH_TO_OPENCV)/build/bin/opencv_test_dnn --gtest_filter=Layer_Test_Softmax.Accuracy
The test case is here. If there is nothing wrong with the test, I will start to complete other ops for evaluation 2.
@huningxin @wzw-intel PTAL.
There was a problem hiding this comment.
test passed on my machine with nVidia GPU.
NALLEIN
force-pushed
the
dawnBuild
branch
2 times, most recently
from
6580754 to
ec67c9a
Compare
huningxin
approved these changes
Jul 21, 2020
This was referenced Jul 21, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To fix #334
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.