On 28 November 2014 at 11:16, Stefan Dösinger stefan@codeweavers.com wrote:
- if (!device->d3d_parent->extended && InterlockedCompareExchange(&device->device_state,
D3D9_DEVICE_STATE_LOST, D3D9_DEVICE_STATE_LOST) == D3D9_DEVICE_STATE_LOST)
Do you need the InterlockedCompareExchange()?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-11-28 16:08, schrieb Henri Verbeet:
Do you need the InterlockedCompareExchange()?
Tbh I'm not sure when it is needed and when it isn't, so I chose to go for the safe option. I know the read of device->device_state should be atomic if its address is 4 byte aligned (and if it isn't InterlockedCompareExchange won't help). But on the other hand you've used it for read-only access in the fence implementation in your command stream work. What was the reason for that? Preventing the loop from looping over an unchanged register?
On 30 November 2014 at 18:28, Stefan Dösinger stefandoesinger@gmail.com wrote:
Am 2014-11-28 16:08, schrieb Henri Verbeet:
Do you need the InterlockedCompareExchange()?
Tbh I'm not sure when it is needed and when it isn't, so I chose to go for the safe option. I know the read of device->device_state should be atomic if its address is 4 byte aligned (and if it isn't InterlockedCompareExchange won't help). But on the other hand you've used it for read-only access in the fence implementation in your command stream work. What was the reason for that? Preventing the loop from looping over an unchanged register?
That code wasn't anything particularly final, but InterlockedCompareExchange() implies a memory barrier and would prevent the CPU from seeing stale values. I don't think seeing a stale value would really matter here, but if it does this probably also isn't enough since you're not protected from e.g. focus loss in the middle of a reset either.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-11-30 19:08, schrieb Henri Verbeet:
That code wasn't anything particularly final, but InterlockedCompareExchange() implies a memory barrier and would prevent the CPU from seeing stale values. I don't think seeing a stale value would really matter here
Yeah, I don't see where a stale value would come from, as this is the first read of this value in this function.
There may be a stale value in the CPU cache, but to my knowledge enforcing cache coherency is the CPU's job.
but if it does this probably also isn't enough since you're not protected from e.g. focus loss in the middle of a reset either.
I tested focus loss during reset and reset during focus loss on Windows, and both of those cases segfault. I don't think we have to worry about this. I didn't provoke an actual race though - I called SetForegroundWindow and reset from test_proc. The result is the same with a D3DCREATE_MULTITHREADED device.
I think this also means we don't have to worry about races in wined3d_set_adapter_display_mode.
Fwiw, device::reset has to be called from the thread that created the device, otherwise it fails. It does not have to be called from the thread that created the window though. (Same for CreateDevice obviously.) So the ACTIVATEAPP handler and reset can run concurrently in theory.
-
Henri Verbeet -
Stefan Dösinger