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

discard-cached-psos renamed to use_cached_psos #1207

Merged
merged 5 commits into from
Aug 1, 2023

Conversation

igorbrsn
Copy link
Contributor

@igorbrsn igorbrsn commented Jul 20, 2023

Problem:
The replayer should always use 'discard-cached-psos'. Currently '--dcp' and '--discard-cached-psos' are options and could not be disabled.

Solution:
Added option'--use-cached-psos' with default 'false'. 'discard_cached_psos' renamed to 'use_cached_psos' and would be set 'true' if used.

Test:
Tested with default, enabled and disabled. Warning issued when deprecated '--dcp' is used.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 9704.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3006 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3006 passed.

@bradgrantham-lunarg
Copy link
Contributor

In general I think this is fine, probably "dcp" should be the default. However, "dcp" means "discard cached PSOs". "enable-dcp" would mean "enable discard cached PSOs", the same thing as "dcp". If this change makes "discard cached PSOs" the default and adds an option to not discard the PSOs, then I think the option would be called "use-cached-psos" or some such.

@igorbrsn igorbrsn force-pushed the dev-enable-discard-cached-psos branch from 03a98d4 to 1cd2efa Compare July 20, 2023 19:03
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 9807.

@igorbrsn
Copy link
Contributor Author

In general I think this is fine, probably "dcp" should be the default. However, "dcp" means "discard cached PSOs". "enable-dcp" would mean "enable discard cached PSOs", the same thing as "dcp". If this change makes "discard cached PSOs" the default and adds an option to not discard the PSOs, then I think the option would be called "use-cached-psos" or some such.

Changed to --use-cached-psos

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3009 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 9836.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3010 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3010 passed.

@igorbrsn
Copy link
Contributor Author

In general I think this is fine, probably "dcp" should be the default. However, "dcp" means "discard cached PSOs". "enable-dcp" would mean "enable discard cached PSOs", the same thing as "dcp". If this change makes "discard cached PSOs" the default and adds an option to not discard the PSOs, then I think the option would be called "use-cached-psos" or some such.

Changed to --use-cached-psos

I am updating logic

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 14098.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3033 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3033 failed.

@@ -805,7 +806,14 @@ static gfxrecon::decode::DxReplayOptions GetDxReplayOptions(const gfxrecon::util

if (arg_parser.IsOptionSet(kDiscardCachedPsosLongOption) || arg_parser.IsOptionSet(kDiscardCachedPsosShortOption))
{
replay_options.discard_cached_psos = true;
GFXRECON_LOG_WARNING("The parameters --dcp and --discard-cached-psos have been depracated in favor for "
Copy link
Contributor

Choose a reason for hiding this comment

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

"deprecated"

@@ -183,7 +183,7 @@ Usage:
[--screenshot-dir <dir>] [--screenshot-prefix <file-prefix>]
[--sfa | --skip-failed-allocations] [--replace-shaders <dir>]
[--opcd | --omit-pipeline-cache-data] [--wsi <platform>]
[--dcp | --discard-cached-psos] [--surface-index <N>]
[--use-cached-psos <boolean>] [--surface-index <N>]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it shouldn't be a boolean if it's off if you specify nothing but turned on when you specify it. Making this a boolean would make it inconsistent with other command-line switches.

@@ -2207,7 +2207,7 @@ HRESULT Dx12ReplayConsumerBase::OverrideLoadGraphicsPipeline(
(desc != nullptr) && (desc->GetPointer() != nullptr) && (state != nullptr));

auto desc2 = desc->GetPointer();
if (options_.discard_cached_psos)
if (options_.use_cached_psos == false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer !options_.use_cached_psos to checking for explicit false.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 15327.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3039 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3039 passed.

Copy link
Contributor

@davidd-lunarg davidd-lunarg left a comment

Choose a reason for hiding this comment

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

I've added a couple requests to fixup documentation for the new option. Also, please update the PR title and description to match the latest changes in the PR.

USAGE_desktop_D3D12.md Show resolved Hide resolved
USAGE_desktop_D3D12.md Outdated Show resolved Hide resolved
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 15471.

@igorbrsn igorbrsn changed the title Enable discard-cached-psos discard-cached-psos renamed to use_cached_psos Jul 31, 2023
@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3042 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3042 passed.

@davidd-lunarg davidd-lunarg merged commit 51e3f59 into LunarG:dev Aug 1, 2023
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.

4 participants