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

webgpu: update dataToGPU doc #6810

Merged
merged 1 commit into from Oct 12, 2022
Merged

webgpu: update dataToGPU doc #6810

merged 1 commit into from Oct 12, 2022

Conversation

xhcao
Copy link
Collaborator

@xhcao xhcao commented Sep 1, 2022

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@xhcao
Copy link
Collaborator Author

xhcao commented Sep 1, 2022

@qjia7 @gyagp PTAL

resource: {
buffer: data.buffer,
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better to add ... here to imply that there are other entries. And it will be good if you can also show the shader snippet about how to use it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

data.buffer is normal GPUBuffer object, and the shader could treat it as other GPUBuffer objects, and the shader does not know that it comes from dataToGPU, so I think it does not need to add the shader usage here. Do you think so?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the hard part is not how to use the buffer. But how to calculate the expected buffer index and then get the buffer data in the pixel shader. And also in the shader, the user should be careful that the binding point should be same with binding in API side. I'm just worried users may be not familiar with webgpu and don't know how to use it. But since you already provided the example code link, I am ok if want to keep it simple here.


The dataToGPU exports a GPUBuffer object, whose content and size is the same as
the source tensor on the webgpu backend, it is user's responsibility to parse
the content and size on the downsream webgpu processing steps.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: downsream -> downstream

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


# For webgpu backend
The example below shows how to get the tensor buffer by calling
`datatoGPU`. Also see this [example code](https://github.com/tensorflow/tfjs-examples/tree/master/gpu-pipeline/webgpu) for details.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be "dataToGPU", and WebGL also has this typo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

// bufSize: the size of the buffer.
// tensorRef: the tensor associated with the buffer.
//
// Unlike webgl backend, There is no parameter for dataGPU api on the webgpu
Copy link
Collaborator

Choose a reason for hiding this comment

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

dataGPU -> dataToGPU

Copy link
Collaborator

Choose a reason for hiding this comment

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

api -> API

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


The dataToGPU exports a GPUBuffer object, whose content and size is the same as
the source tensor on the webgpu backend, it is user's responsibility to parse
the content and size on the downstream webgpu processing steps.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why we need this paragraph. In addition, bufSize is returned, but there is no mention how to use it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

// data has below fields:
// {buffer, bufSize, tensorRef}
// buffer: A GPUBuffer.
// bufSize: the size of the buffer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So my left open is why we need bufSize.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. I had explicitly used bufSize in the sample.

Copy link
Collaborator

@gyagp gyagp left a comment

Choose a reason for hiding this comment

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

LGTM

@xhcao xhcao merged commit 2cc528b into tensorflow:master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants