From 1fa288b224efe6a41d056dc08b3cb783e4f8f841 Mon Sep 17 00:00:00 2001 From: Karol Herbst Date: Tue, 6 Aug 2024 00:52:23 +0200 Subject: [PATCH] rusticl/memory: Fix memory unmaps after rework An application could map and unmap a host ptr allocation multiple times, but because how the refcounting works, we might never ended up syncing the written data to the mapped region. This moves the refcounting out of the event processing. Fixes: 7b22bc617bf ("rusticl/memory: complete rework on how mapping is implemented") Part-of: --- src/gallium/frontends/rusticl/api/memory.rs | 9 +- src/gallium/frontends/rusticl/core/memory.rs | 136 +++++++++++++------ 2 files changed, 101 insertions(+), 44 deletions(-) diff --git a/src/gallium/frontends/rusticl/api/memory.rs b/src/gallium/frontends/rusticl/api/memory.rs index 22d3b5a8226..b38beb2d688 100644 --- a/src/gallium/frontends/rusticl/api/memory.rs +++ b/src/gallium/frontends/rusticl/api/memory.rs @@ -2192,13 +2192,20 @@ fn enqueue_unmap_mem_object( // SAFETY: it's required that applications do not cause data races let mapped_ptr = unsafe { MutMemoryPtr::from_ptr(mapped_ptr) }; + let needs_sync = m.unmap(mapped_ptr)?; create_and_queue( q, CL_COMMAND_UNMAP_MEM_OBJECT, evs, event, false, - Box::new(move |q, ctx| m.unmap(q, ctx, mapped_ptr)), + Box::new(move |q, ctx| { + if needs_sync { + m.sync_unmap(q, ctx, mapped_ptr) + } else { + Ok(()) + } + }), ) } diff --git a/src/gallium/frontends/rusticl/core/memory.rs b/src/gallium/frontends/rusticl/core/memory.rs index fcfc6282319..17586a32918 100644 --- a/src/gallium/frontends/rusticl/core/memory.rs +++ b/src/gallium/frontends/rusticl/core/memory.rs @@ -39,6 +39,8 @@ struct Mapping { layout: Layout, writes: bool, ptr: Option, + /// reference count from the API perspective. Once it reaches 0, we need to write back the + /// mappings content to the GPU resource. count: u32, inner: T, } @@ -152,10 +154,17 @@ impl Mem { } } - pub fn unmap(&self, q: &Queue, ctx: &PipeContext, ptr: MutMemoryPtr) -> CLResult<()> { + pub fn sync_unmap(&self, q: &Queue, ctx: &PipeContext, ptr: MutMemoryPtr) -> CLResult<()> { match self { - Self::Buffer(b) => b.unmap(q, ctx, ptr), - Self::Image(i) => i.unmap(q, ctx, ptr), + Self::Buffer(b) => b.sync_unmap(q, ctx, ptr), + Self::Image(i) => i.sync_unmap(q, ctx, ptr), + } + } + + pub fn unmap(&self, ptr: MutMemoryPtr) -> CLResult { + match self { + Self::Buffer(b) => b.unmap(ptr), + Self::Image(i) => i.unmap(ptr), } } } @@ -916,7 +925,9 @@ impl Buffer { } fn is_mapped_ptr(&self, ptr: *mut c_void) -> bool { - self.maps.lock().unwrap().contains_key(ptr as usize) + let mut maps = self.maps.lock().unwrap(); + let entry = maps.entry(ptr as usize); + matches!(entry, Entry::Occupied(entry) if entry.get().count > 0) } pub fn map(&self, size: usize, offset: usize, writes: bool) -> CLResult { @@ -1001,6 +1012,31 @@ impl Buffer { self.read(q, ctx, mapping.offset, ptr, mapping.size()) } + pub fn sync_unmap(&self, q: &Queue, ctx: &PipeContext, ptr: MutMemoryPtr) -> CLResult<()> { + // no need to update + if self.is_pure_user_memory(q.device)? { + return Ok(()); + } + + match self.maps.lock().unwrap().entry(ptr.as_ptr() as usize) { + Entry::Vacant(_) => Err(CL_INVALID_VALUE), + Entry::Occupied(entry) => { + let mapping = entry.get(); + + if mapping.writes { + self.write(q, ctx, mapping.offset, ptr.into(), mapping.size())?; + } + + // only remove if the mapping wasn't reused in the meantime + if mapping.count == 0 { + entry.remove(); + } + + Ok(()) + } + } + } + fn tx<'a>( &self, q: &Queue, @@ -1021,22 +1057,16 @@ impl Buffer { .ok_or(CL_OUT_OF_RESOURCES) } - pub fn unmap(&self, q: &Queue, ctx: &PipeContext, ptr: MutMemoryPtr) -> CLResult<()> { - let mapping = match self.maps.lock().unwrap().entry(ptr.as_ptr() as usize) { - Entry::Vacant(_) => return Err(CL_INVALID_VALUE), + pub fn unmap(&self, ptr: MutMemoryPtr) -> CLResult { + match self.maps.lock().unwrap().entry(ptr.as_ptr() as usize) { + Entry::Vacant(_) => Err(CL_INVALID_VALUE), Entry::Occupied(mut entry) => { - entry.get_mut().count -= 1; - (entry.get().count == 0).then(|| entry.remove()) + let entry = entry.get_mut(); + debug_assert!(entry.count > 0); + entry.count -= 1; + Ok(entry.count == 0) } - }; - - if let Some(mapping) = mapping { - if mapping.writes && !self.is_pure_user_memory(q.device)? { - self.write(q, ctx, mapping.offset, ptr.into(), mapping.size())?; - } - }; - - Ok(()) + } } pub fn write( @@ -1307,7 +1337,9 @@ impl Image { } fn is_mapped_ptr(&self, ptr: *mut c_void) -> bool { - self.maps.lock().unwrap().contains_key(ptr as usize) + let mut maps = self.maps.lock().unwrap(); + let entry = maps.entry(ptr as usize); + matches!(entry, Entry::Occupied(entry) if entry.get().count > 0) } pub fn is_parent_buffer(&self) -> bool { @@ -1437,6 +1469,41 @@ impl Image { ) } + pub fn sync_unmap(&self, q: &Queue, ctx: &PipeContext, ptr: MutMemoryPtr) -> CLResult<()> { + // no need to update + if self.is_pure_user_memory(q.device)? { + return Ok(()); + } + + match self.maps.lock().unwrap().entry(ptr.as_ptr() as usize) { + Entry::Vacant(_) => Err(CL_INVALID_VALUE), + Entry::Occupied(entry) => { + let mapping = entry.get(); + let row_pitch = self.image_desc.row_pitch()? as usize; + let slice_pitch = self.image_desc.slice_pitch(); + + if mapping.writes { + self.write( + ptr.into(), + q, + ctx, + &mapping.region, + row_pitch, + slice_pitch, + &mapping.origin, + )?; + } + + // only remove if the mapping wasn't reused in the meantime + if mapping.count == 0 { + entry.remove(); + } + + Ok(()) + } + } + } + fn tx_image<'a>( &self, q: &Queue, @@ -1448,33 +1515,16 @@ impl Image { ctx.texture_map(r, bx, rw).ok_or(CL_OUT_OF_RESOURCES) } - pub fn unmap(&self, q: &Queue, ctx: &PipeContext, ptr: MutMemoryPtr) -> CLResult<()> { - let mapping = match self.maps.lock().unwrap().entry(ptr.as_ptr() as usize) { - Entry::Vacant(_) => return Err(CL_INVALID_VALUE), + pub fn unmap(&self, ptr: MutMemoryPtr) -> CLResult { + match self.maps.lock().unwrap().entry(ptr.as_ptr() as usize) { + Entry::Vacant(_) => Err(CL_INVALID_VALUE), Entry::Occupied(mut entry) => { - entry.get_mut().count -= 1; - (entry.get().count == 0).then(|| entry.remove()) - } - }; - - let row_pitch = self.image_desc.row_pitch()? as usize; - let slice_pitch = self.image_desc.slice_pitch(); - - if let Some(mapping) = mapping { - if mapping.writes && !self.is_pure_user_memory(q.device)? { - self.write( - ptr.into(), - q, - ctx, - &mapping.region, - row_pitch, - slice_pitch, - &mapping.origin, - )?; + let entry = entry.get_mut(); + debug_assert!(entry.count > 0); + entry.count -= 1; + Ok(entry.count == 0) } } - - Ok(()) } pub fn write(