Alexandre Courbot
2025-Oct-16 06:23 UTC
[PATCH v5 04/14] gpu: nova-core: Add a slice-buffer (sbuffer) datastructure
On Mon Oct 13, 2025 at 3:20 PM JST, Alistair Popple wrote:> From: Joel Fernandes <joelagnelf at nvidia.com> > > A data structure that can be used to write across multiple slices which > may be out of order in memory. This lets SBuffer user correctly and > safely write out of memory order, without error-prone tracking of > pointers/offsets. > > let mut buf1 = [0u8; 3]; > let mut buf2 = [0u8; 5]; > let mut sbuffer = SBuffer::new([&mut buf1[..], &mut buf2[..]]); > > let data = b"hello"; > let result = sbuffer.write(data); > > An internal conversion of gsp.rs to use this resulted in a nice -ve delta: > gsp.rs: 37 insertions(+), 99 deletions(-) > > Co-developed-by: Alistair Popple <apopple at nvidia.com> > Signed-off-by: Alistair Popple <apopple at nvidia.com> > Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com> > Reviewed-by: Lyude Paul <lyude at redhat.com> > > --- > > Changes for v5: > - Typos > - s/ETOOSMALL/EINVAL/ > - Add documentation > - Fix up examples > > Changes for v3: > - Addressed minor review comment from Lyude > --- > drivers/gpu/nova-core/nova_core.rs | 1 + > drivers/gpu/nova-core/sbuffer.rs | 218 +++++++++++++++++++++++++++++ > 2 files changed, 219 insertions(+) > create mode 100644 drivers/gpu/nova-core/sbuffer.rs > > diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs > index fffcaee2249f..a6feeba6254c 100644 > --- a/drivers/gpu/nova-core/nova_core.rs > +++ b/drivers/gpu/nova-core/nova_core.rs > @@ -11,6 +11,7 @@ > mod gpu; > mod gsp; > mod regs; > +mod sbuffer; > mod util; > mod vbios; > > diff --git a/drivers/gpu/nova-core/sbuffer.rs b/drivers/gpu/nova-core/sbuffer.rs > new file mode 100644 > index 000000000000..d9c412a68bd8 > --- /dev/null > +++ b/drivers/gpu/nova-core/sbuffer.rs > @@ -0,0 +1,218 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +use core::ops::Deref; > + > +use kernel::alloc::KVec; > +use kernel::error::code::*; > +use kernel::prelude::*; > + > +/// A buffer abstraction for discontiguous byte slices. > +/// > +/// This allows you to treat multiple non-contiguous `&mut [u8]` slices > +/// of the same length as a single stream-like read/write buffer. > +/// > +/// # Example: > +/// > +/// ``` > +/// let mut buf1 = [0u8; 5]; > +/// let mut buf2 = [0u8; 5]; > +/// let mut sbuffer = SBufferIter::new_writer([&buf1, &buf2]); > +/// > +/// let data = b"hello"; > +/// let result = sbuffer.write_all(data); > +/// ```This example doesn't build - there are several things wrong with it. It is also missing statements to confirm and show the expected result. Here is a fixed and slightly improved version: /// let mut buf1 = [0u8; 5]; /// let mut buf2 = [0u8; 5]; /// let mut sbuffer = SBufferIter::new_writer([&mut buf1[..], &mut buf2[..]]); /// /// let data = b"hi world!"; /// sbuffer.write_all(data)?; /// drop(sbuffer); /// /// assert_eq!(buf1, *b"hi wo"); /// assert_eq!(buf2, *b"rld!\0"); /// /// # Ok::<(), Error>(())> +/// > +/// A sliding window of slices to process. > +/// > +/// Both read and write buffers are implemented in terms of operating on slices of a requested > +/// size. This base class implements logic that can be shared between the two to support that. > +/// > +/// `S` is a slice type, `I` is an iterator yielding `S`.Why is there another doccomment after the example section? It looks like this should be merged with the first doccomment before the example? There is also no `S` generic parameter.> +pub(crate) struct SBufferIter<I: Iterator> { > + /// `Some` if we are not at the end of the data yet. > + cur_slice: Option<I::Item>, > + /// All the slices remaining after `cur_slice`. > + slices: I, > +} > + > +impl<'a, I> SBufferIter<I> > +where > + I: Iterator, > +{ > + /// Creates a reader buffer for a discontiguous set of byte slices. > + /// > + /// # Example: > + /// > + /// ``` > + /// let buf1: [u8; 5] = [0, 1, 2, 3, 4]; > + /// let buf2: [u8; 5] = [5, 6, 7, 8, 9]; > + /// let sbuffer = SBufferIter::new_reader([&buf1[..], &buf2[..]]); > + /// let sum: u8 = sbuffer.sum(); > + /// assert_eq!(sum, 45); > + /// ``` > + #[expect(unused)] > + pub(crate) fn new_reader(slices: impl IntoIterator<IntoIter = I>) -> Self > + where > + I: Iterator<Item = &'a [u8]>, > + { > + Self::new(slices) > + } > + > + /// Creates a writeable buffer for a discontiguous set of byte slices. > + /// > + /// # Example: > + /// > + /// ``` > + /// let mut buf1 = [0u8; 5]; > + /// let mut buf2 = [0u8; 5]; > + /// let mut sbuffer = SBufferIter::new_writer([&mut buf1[..], &mut buf2[..]]); > + /// sbuffer.write_all(&[0u8, 1, 2, 3, 4, 5, 6, 7, 8, 9][..])?; > + /// drop(sbuffer); > + /// assert_eq!(buf1, [0, 1, 2, 3, 4]); > + /// assert_eq!(buf2, [5, 6, 7, 8, 9]); > + /// > + /// ``` > + #[expect(unused)] > + pub(crate) fn new_writer(slices: impl IntoIterator<IntoIter = I>) -> Self > + where > + I: Iterator<Item = &'a mut [u8]>, > + { > + Self::new(slices) > + } > + > + fn new(slices: impl IntoIterator<IntoIter = I>) -> Self > + where > + I::Item: Deref<Target = [u8]>, > + { > + let mut slices = slices.into_iter(); > + > + Self { > + // Skip empty slices to avoid trouble down the road.I guess "Skip empty slices" is enough as it is part of the algorithm. :)> + cur_slice: slices.find(|s| !s.deref().is_empty()), > + slices, > + } > + } > + > + fn get_slice_internal( > + &mut self, > + len: usize, > + mut f: impl FnMut(I::Item, usize) -> (I::Item, I::Item), > + ) -> Option<I::Item>Let's document this a bit as its purpose is not immediately clear. We can take the documentation from the `get_slice` methods, with a short explanation that the closure is supposed to split the slice received as first argument at the position given as the second.> + where > + I::Item: Deref<Target = [u8]>, > + { > + match self.cur_slice.take() { > + None => None, > + Some(cur_slice) => { > + if len >= cur_slice.len() { > + // Caller requested more data than is in the current slice, return it entirely > + // and prepare the following slice for being used. Skip empty slices to avoid > + // trouble. > + self.cur_slice = self.slices.find(|s| !s.is_empty()); > + > + Some(cur_slice) > + } else { > + // The current slice can satisfy the request, split it and return a slice of > + // the requested size. > + let (ret, next) = f(cur_slice, len); > + self.cur_slice = Some(next); > + > + Some(ret) > + } > + } > + } > + } > +} > + > +/// Provides a way to get non-mutable slices of data to read from. > +impl<'a, I> SBufferIter<I> > +where > + I: Iterator<Item = &'a [u8]>, > +{ > + /// Returns a slice of at most `len` bytes, or `None` if we are at the end of the data. > + /// > + /// If a slice shorter than `len` bytes has been returned, the caller can call this method > + /// again until it returns `None` to try and obtain the remainder of the data. > + fn get_slice(&mut self, len: usize) -> Option<&'a [u8]> { > + self.get_slice_internal(len, |s, pos| s.split_at(pos)) > + } > + > + /// Ideally we would implement `Read`, but it is not available in `core`. > + /// So mimic `std::io::Read::read_exact`.That's a useful side-comment, but we also need small sentence describing the method, e.g. "Fill `dst` with the next bytes from this `SBufferIter`, or return `EINVAL` if there isn't enough data available."> + #[expect(unused)] > + pub(crate) fn read_exact(&mut self, mut dst: &mut [u8]) -> Result { > + while !dst.is_empty() { > + match self.get_slice(dst.len()) { > + None => return Err(EINVAL), > + Some(src) => { > + let dst_slice; > + (dst_slice, dst) = dst.split_at_mut(src.len()); > + dst_slice.copy_from_slice(src); > + } > + } > + } > + > + Ok(()) > + } > + > + /// Read all the remaining data into a `KVec`. > + /// > + /// `self` will be empty after this operation. > + #[expect(unused)] > + pub(crate) fn read_into_kvec(&mut self, flags: kernel::alloc::Flags) -> Result<KVec<u8>> {nit: `flush_into_kvec` is probably a more descriptive name, as `read` is already used for the other method.> + let mut buf = KVec::<u8>::new(); > + > + if let Some(slice) = core::mem::take(&mut self.cur_slice) { > + buf.extend_from_slice(slice, flags)?; > + } > + for slice in &mut self.slices { > + buf.extend_from_slice(slice, flags)?; > + } > + > + Ok(buf) > + } > +} > + > +/// Provides a way to get mutable slices of data to write into. > +impl<'a, I> SBufferIter<I> > +where > + I: Iterator<Item = &'a mut [u8]>, > +{ > + /// Returns a mutable slice of at most `len` bytes, or `None` if we are at the end of the data. > + /// > + /// If a slice shorter than `len` bytes has been returned, the caller can call this method > + /// again until it returns `None` to try and obtain the remainder of the data. > + fn get_slice_mut(&mut self, len: usize) -> Option<&'a mut [u8]> { > + self.get_slice_internal(len, |s, pos| s.split_at_mut(pos)) > + } > + > + /// Ideally we would implement `Write`, but it is not available in `core`. > + /// So mimic `std::io::Write::write_all`.Same comment as `read_all`.
Miguel Ojeda
2025-Oct-16 19:19 UTC
[PATCH v5 04/14] gpu: nova-core: Add a slice-buffer (sbuffer) datastructure
On Thu, Oct 16, 2025 at 8:23?AM Alexandre Courbot <acourbot at nvidia.com> wrote:> > On Mon Oct 13, 2025 at 3:20 PM JST, Alistair Popple wrote: > > > > +/// # Example: > > +/// > > +/// ``` > > +/// let mut buf1 = [0u8; 5]; > > +/// let mut buf2 = [0u8; 5]; > > +/// let mut sbuffer = SBufferIter::new_writer([&buf1, &buf2]); > > +/// > > +/// let data = b"hello"; > > +/// let result = sbuffer.write_all(data); > > +/// ``` > > This example doesn't build - there are several things wrong with it. It > is also missing statements to confirm and show the expected result. Here > is a fixed and slightly improved version:Yeah, I mentioned this one in a previous version -- the section header is also still wrong too. Alistair, please check the link I gave: https://docs.kernel.org/rust/coding-guidelines.html#code-documentation or other code in the `kernel` crate for examples on how it is usually done. It is not critical today, of course, but the further it is from what will be needed in a few months, the harder it will become to start building the docs and running the examples as KUnit tests. Thanks! Cheers, Miguel
Alistair Popple
2025-Oct-17 04:45 UTC
[PATCH v5 04/14] gpu: nova-core: Add a slice-buffer (sbuffer) datastructure
On 2025-10-17 at 06:18 +1100, Miguel Ojeda <miguel.ojeda.sandonis at gmail.com> wrote...> On Thu, Oct 16, 2025 at 8:23?AM Alexandre Courbot <acourbot at nvidia.com> wrote: > > > > On Mon Oct 13, 2025 at 3:20 PM JST, Alistair Popple wrote: > > > > > > +/// # Example: > > > +/// > > > +/// ``` > > > +/// let mut buf1 = [0u8; 5]; > > > +/// let mut buf2 = [0u8; 5]; > > > +/// let mut sbuffer = SBufferIter::new_writer([&buf1, &buf2]); > > > +/// > > > +/// let data = b"hello"; > > > +/// let result = sbuffer.write_all(data); > > > +/// ``` > > > > This example doesn't build - there are several things wrong with it. It > > is also missing statements to confirm and show the expected result. Here > > is a fixed and slightly improved version:Argh, you're right. I cut and pasted then edited the wrong thing from my test build. How are you building these? The `rustdoc` target seems to ignore Nova (or I'm doing something wrong).> Yeah, I mentioned this one in a previous version -- the section header > is also still wrong too. > > Alistair, please check the link I gave:Will do. I thought Joel had addressed your comments in the fix patch I pulled in from him (he wrote most of this originally) but I can see the `/// # Example:` heading is wrong.> https://docs.kernel.org/rust/coding-guidelines.html#code-documentation > > or other code in the `kernel` crate for examples on how it is usually done. > > It is not critical today, of course, but the further it is from what > will be needed in a few months, the harder it will become to start > building the docs and running the examples as KUnit tests.No, I think it *is* critical :-) Much easier just to get this right from the beginning than deal with heaps of errors/warnings later. It's just my fingers that are still getting used to the subtle differences between C kernel code and Rust kernel code, so thanks for the guidance. - Alistair> > Thanks! > > Cheers, > Miguel