Merge pull request #613 from tomaka/fix-cb-unlock

Fix resources being unlocked when they shouldn't be

vulkano/src/command_buffer/auto.rs

@@ -637,9 +637,14 @@ unsafe impl<P> CommandBuffer for AutoCommandBuffer<P> {
     }
 
     #[inline]
-    fn prepare_submit(&self, future: &GpuFuture, queue: &Queue)
-                      -> Result<(), CommandBufferExecError> {
-        self.inner.prepare_submit(future, queue)
+    fn lock_submit(&self, future: &GpuFuture, queue: &Queue)
+                   -> Result<(), CommandBufferExecError> {
+        self.inner.lock_submit(future, queue)
+    }
+
+    #[inline]
+    unsafe fn unlock(&self) {
+        self.inner.unlock()
     }
 
     #[inline]

vulkano/src/command_buffer/synced.rs

@@ -2134,6 +2134,7 @@ impl<'a> Hash for CbKey<'a> {
     }
 }
 
+// TODO: should we really implement this trait on this type?
 unsafe impl<P> CommandBuffer for SyncCommandBuffer<P> {
     type PoolAlloc = P;
 
@@ -2142,8 +2143,8 @@ unsafe impl<P> CommandBuffer for SyncCommandBuffer<P> {
         &self.inner
     }
 
-    fn prepare_submit(&self, future: &GpuFuture, queue: &Queue)
-                      -> Result<(), CommandBufferExecError> {
+    fn lock_submit(&self, future: &GpuFuture, queue: &Queue)
+                   -> Result<(), CommandBufferExecError> {
         // TODO: if at any point we return an error, we can't recover
 
         let commands_lock = self.commands.lock().unwrap();
@@ -2210,6 +2211,37 @@ unsafe impl<P> CommandBuffer for SyncCommandBuffer<P> {
         Ok(())
     }
 
+    unsafe fn unlock(&self) {
+        let commands_lock = self.commands.lock().unwrap();
+
+        for (key, entry) in self.resources.iter() {
+            let (command_id, resource_ty, resource_index) = match *key {
+                CbKey::Command {
+                    command_id,
+                    resource_ty,
+                    resource_index,
+                    ..
+                } => {
+                    (command_id, resource_ty, resource_index)
+                },
+                _ => unreachable!(),
+            };
+
+            match resource_ty {
+                KeyTy::Buffer => {
+                    let cmd = &commands_lock[command_id];
+                    let buf = cmd.buffer(resource_index);
+                    buf.unlock();
+                },
+                KeyTy::Image => {
+                    let cmd = &commands_lock[command_id];
+                    let img = cmd.image(resource_index);
+                    img.unlock();
+                },
+            }
+        }
+    }
+
     #[inline]
     fn check_buffer_access(
         &self, buffer: &BufferAccess, exclusive: bool, queue: &Queue)
@@ -2258,38 +2290,3 @@ unsafe impl<P> DeviceOwned for SyncCommandBuffer<P> {
         self.inner.device()
     }
 }
-
-impl<P> Drop for SyncCommandBuffer<P> {
-    fn drop(&mut self) {
-        unsafe {
-            let commands_lock = self.commands.lock().unwrap();
-
-            for (key, entry) in self.resources.iter() {
-                let (command_id, resource_ty, resource_index) = match *key {
-                    CbKey::Command {
-                        command_id,
-                        resource_ty,
-                        resource_index,
-                        ..
-                    } => {
-                        (command_id, resource_ty, resource_index)
-                    },
-                    _ => unreachable!(),
-                };
-
-                match resource_ty {
-                    KeyTy::Buffer => {
-                        let cmd = &commands_lock[command_id];
-                        let buf = cmd.buffer(resource_index);
-                        buf.unlock();
-                    },
-                    KeyTy::Image => {
-                        let cmd = &commands_lock[command_id];
-                        let img = cmd.image(resource_index);
-                        img.unlock();
-                    },
-                }
-            }
-        }
-    }
-}

vulkano/src/command_buffer/traits.rs

@@ -49,18 +49,28 @@ pub unsafe trait CommandBuffer: DeviceOwned {
         self.inner().queue_family()
     }*/
 
-    /// Checks whether this command buffer is allowed to be submitted after the `future` and on
-    /// the given queue.
-    ///
-    /// Calling this function means that at some point you will submit the command buffer to the
-    /// GPU. Once the function has returned `Ok`, the resources used by the command buffer will
-    /// likely be in a locked state until the command buffer is destroyed.
-    ///
-    /// **You should not call this function directly**, otherwise any further attempt to submit
-    /// will return a runtime error.
-    // TODO: what about command buffers that are submitted multiple times?
+    #[deprecated(note = "Renamed to `lock_submit`")]
     fn prepare_submit(&self, future: &GpuFuture, queue: &Queue)
-                      -> Result<(), CommandBufferExecError>;
+                      -> Result<(), CommandBufferExecError>
+    {
+        self.lock_submit(future, queue)
+    }
+
+    /// Checks whether this command buffer is allowed to be submitted after the `future` and on
+    /// the given queue, and if so locks it.
+    ///
+    /// If you call this function, then you should call `unlock` afterwards.
+    // TODO: require `&mut self` instead, but this has some consequences on other parts of the lib
+    fn lock_submit(&self, future: &GpuFuture, queue: &Queue)
+                   -> Result<(), CommandBufferExecError>;
+
+    /// Unlocks the command buffer. Should be called once for each call to `lock_submit`.
+    ///
+    /// # Safety
+    ///
+    /// Must not be called if you haven't called `lock_submit` before.
+    // TODO: require `&mut self` instead, but this has some consequences on other parts of the lib
+    unsafe fn unlock(&self);
 
     /// Executes this command buffer on a queue.
     ///
@@ -120,12 +130,12 @@ pub unsafe trait CommandBuffer: DeviceOwned {
         assert_eq!(self.device().internal_object(),
                    future.device().internal_object());
 
-        self.prepare_submit(&future, &queue)?;
-
         if !future.queue_change_allowed() {
             assert!(future.queue().unwrap().is_same(&queue));
         }
 
+        self.lock_submit(&future, &queue)?;
+
         Ok(CommandBufferExecFuture {
                previous: future,
                command_buffer: self,
@@ -168,9 +178,14 @@ unsafe impl<T> CommandBuffer for T
     }
 
     #[inline]
-    fn prepare_submit(&self, future: &GpuFuture, queue: &Queue)
-                      -> Result<(), CommandBufferExecError> {
-        (**self).prepare_submit(future, queue)
+    fn lock_submit(&self, future: &GpuFuture, queue: &Queue)
+                   -> Result<(), CommandBufferExecError> {
+        (**self).lock_submit(future, queue)
+    }
+
+    #[inline]
+    unsafe fn unlock(&self) {
+        (**self).unlock();
     }
 
     #[inline]
@@ -268,7 +283,10 @@ unsafe impl<F, Cb> GpuFuture for CommandBufferExecFuture<F, Cb>
 
     #[inline]
     unsafe fn signal_finished(&self) {
-        self.finished.store(true, Ordering::SeqCst);
+        if self.finished.swap(true, Ordering::SeqCst) == false {
+            self.command_buffer.unlock();
+        }
+
         self.previous.signal_finished();
     }
 
@@ -333,6 +351,7 @@ impl<F, Cb> Drop for CommandBufferExecFuture<F, Cb>
                 self.flush().unwrap();
                 // Block until the queue finished.
                 self.queue.wait().unwrap();
+                self.command_buffer.unlock();
                 self.previous.signal_finished();
             }
         }

vulkano/src/sync/future/mod.rs

@@ -90,6 +90,9 @@ pub unsafe trait GpuFuture: DeviceOwned {
     ///
     /// This must only be done if you called `build_submission()`, submitted the returned
     /// submission, and determined that it was finished.
+    ///
+    /// The implementation must be aware that this function can be called multiple times on the
+    /// same future.
     unsafe fn signal_finished(&self);
 
     /// Returns the queue that triggers the event. Returns `None` if unknown or irrelevant.