Skip to content

Allow recipe to change fence timeout.#624

Merged
dj2 merged 2 commits intogoogle:masterfrom
dj2:fence_timeout
Sep 4, 2019
Merged

Allow recipe to change fence timeout.#624
dj2 merged 2 commits intogoogle:masterfrom
dj2:fence_timeout

Conversation

@dj2
Copy link
Collaborator

@dj2 dj2 commented Aug 29, 2019

This CL adds an API to the recipe class to set the fence timeout for a
given script.

The default fence timeout is also bumped from 100ms to 1000ms.

This CL adds an API to the recipe class to set the fence timeout for a
given script.

The default fence timeout is also bumped from 100ms to 1000ms.
@dj2 dj2 added the enhancement New feature or request label Aug 29, 2019
@dj2 dj2 requested a review from dneto0 August 29, 2019 03:03
@dj2 dj2 self-assigned this Aug 29, 2019
@dj2
Copy link
Collaborator Author

dj2 commented Aug 29, 2019

@ben-clayton

@ben-clayton
Copy link
Contributor

Looks perfect for our needs, thank you!


/// Sets the fence timeout to |timeout_ms|.
void SetFenceTimeout(uint32_t timeout_ms) override {
engine_data_.fence_timeout_ms = timeout_ms;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This breaks the abstraction. Setting the fence timeout on one script will change the timeout for all other scripts running on that engine.

Instead you should have the executor get the timeout from the script when the script is being executed, then apply it when the script is executed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

engine->SetEngineData(script->GetEngineData());

The engine has the data set from the script when the script executes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

engine data is per script

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah. thanks

@ben-clayton
Copy link
Contributor

Gentle ping.
This one issue is causing mayhem with our tests. If this is going to take a while, I'll consider adding a timeout statement to all of deqp's amber scripts.

@dj2
Copy link
Collaborator Author

dj2 commented Aug 31, 2019

Kokoro doesn't seem to be showing the details links anymore so I can't see why the dawn build failed ....

@sarahM0
Copy link
Contributor

sarahM0 commented Sep 3, 2019

The failure is because of a new change in dawn buffer UsageBit (not changes in this PR). I'll put up a fix for it shortly.

@dj2
Copy link
Collaborator Author

dj2 commented Sep 3, 2019

Awesome, thanks Sarah. @dneto0 can you take another look please.

@dj2 dj2 requested a review from dneto0 September 4, 2019 12:39
@dj2 dj2 merged commit 6e16cab into google:master Sep 4, 2019
@dj2 dj2 deleted the fence_timeout branch September 4, 2019 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants