Re: [PATCH 1/5] d3d9: Refuse to reset a lost device.
On 28 November 2014 at 11:16, Stefan Dösinger <stefan(a)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? -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJUe1O+AAoJEN0/YqbEcdMwfDkP/3c4tHlBu0G+RpZoViU9b1la gVBVko0zmaMjjSwdq+KO2JuRK8BXh3AFSk72sH8SDLeJBiUPM1ielzZiLkgduevW SoQrxM2ELOnLuFfbQRceytCauZcLOWd4FbmRHyAR5HklIrz+9FBmdATdo91K4n/K xzy36/X8uXjIGgqwWg0o/+hSMmS+Fav+X1FnjFwF+ZU/7iq7ORop+6MYUaK5w3Ku ukOy8nfuyTRMSdqX0FkGp1R2rvQgQTI3nWfeiqlDRVS9Zl3704uTNYbO/Hs6mlRw AxY1Hfi5rpD8+CfCRr04W7oDvGu34CQeODdEdT9n2UckJQMWxxJCnT4SYqarNBna 4ZUcqdIyxtJP6+2Q2VJYfXMFXYkkkzsdtCeZiARmBUXIzaQFahz+pmAWyec8HyMS 5wIvrhgTx77WTPTSs62mCLzk0QWxdXT/AbKKtL3tpMbw7c7IM6Yp5RbnN2sLJ5Vf F8jNglmHBovukXe0dzhpw9Tfv1BTjrnBVyL3iZ8VdTWIKYVtKJzMAKMFe0FCfqQz +4etk1BbgBNLOBX5pB9vBPaLXyk+0WnuVQ0466De4fap8BN6Nnw/PO5kx6YXLzYc MmpSCbKIMif/+OgcbuLSc+sbT31swX7OPR6P+57QcVUSW2jdLF5pxXGKEmt6ZPZt GX2puHTkETiHpBWZoF6g =cmxO -----END PGP SIGNATURE-----
On 30 November 2014 at 18:28, Stefan Dösinger <stefandoesinger(a)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. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJUfJA+AAoJEN0/YqbEcdMwWGQP/29bSTK+b6Zviqv+px4MiPMh IA3/XVqO4PYI6XTeiqTMayVHcLZewO0lMUiCdf7P8bOJea+vaB8TD6gbwc3OObwz V1kUmcfg42tcgxnXmCBFjmQvnFIvyZJvEmr5HhVg/isN3VW+aZjfM9/og+WmIMzr OhMNiwMEETghRwwJQxOxMogpml1Mfdn3E5S60+k/RlUbedcs6Fkc9FoKh7xOecBQ /13cLEfO7RsVkqnjMgJQdkol4DogbkdG2/oG+A6ZN36frp3NKEmJGyqgF7B6NDh4 t3CDO7aiqTXrVY2fCHDL97obE03Envr0YNbmIVhijTa1uagLvIm5oNnQ4YLcght1 IYD5OIT04SpVkylZjLyr+Z269cw8sexJ0Gz+E+R5DNwfzEcvioZkiTmsPECq9zrr uOyAiyFKx+QsidJeE+ii6ghVDO731OWCMWyXsCjCkejtFo9Opz1lDGnBJ5helJZV xCuNJ/Sl4RHhFM0uvsWWSom7+8qD4kS5tKiYjxMkySXkrsYGC143AaS1bqjP0yCN H22VljxqB8uyB7Nr2wh+UkyrYmSGGiyQACXhm7VLyui0em1jWIo9zM0Cp4k7SyCZ 2nK5h5znM5UzzjRfE6td8Cj45kh8AUK+r425EOdnjGo+UPLT06Kc7RvKOahDxyHp YM13l/jIjF5QoepI69BC =/ZOE -----END PGP SIGNATURE-----
participants (2)
-
Henri Verbeet -
Stefan Dösinger