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

allow specifying an adapter name also when using DXGI #261

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

deischi
Copy link

@deischi deischi commented Jul 24, 2019

I see no need to use DXCORE to specify an adapter name.

@deischi deischi requested a review from a team as a code owner July 24, 2019 09:51
@msftclas
Copy link

msftclas commented Jul 24, 2019

CLA assistant check
All CLA requirements met.

@deischi deischi changed the title allow specifying an adapter name also when using old DXGI allow specifying an adapter name also when using DXGI Jul 24, 2019
@ryanlai2
Copy link
Contributor

Hi @deischi, thank you for your contribution! It's correct that specifying adapter name shouldn't be constrained to DxCore.

I have a few suggested changes:

  • There is already a conditional to handle if "-GPUAdapterName" flag is used. Can your code to choose adapter be added inside here?:

    else if ((TypeHelper::GetWinmlDeviceKind(deviceType) != LearningModelDeviceKind::Cpu) && !adapterName.empty())

  • Within the conditional of (WinMLDeviceKind isn't CPU and adaptername isn't empty). One flow can be like this:

    • Try to use DXGI to enumerate adapters
    • Fall back to using DxCore if DXGI doesn't work / isn't available.
    • #ifdef check for DXCORE_SUPPORTED_BUILD will need to be around the DxCore code.
    • Create Session with chosen adapter.
  • Check for first matching substring for adapter name.

  • Spaces instead of tabs for spacing.

Thanks! Let me know if you have any questions.

@deischi
Copy link
Author

deischi commented Jul 25, 2019

I have tried to add my changes as local and small as possible to quickly get a result.

You are right, having the adapter selection only once would be better.
Please understand that improving the WinMlRunner is not one of the top priorities, so it will definitely take some time until come to that.
Also I have no access to DXCore, any changes I will do related to that will be done blindly.

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.

3 participants