Message-Id: <DDJJ4P7ED3LJ.6YD2M62RB5VY@nvidia.com>
Date: Thu, 16 Oct 2025 15:23:38 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alistair Popple" <apopple@...dia.com>,
 <rust-for-linux@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
 <dakr@...nel.org>, <acourbot@...dia.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "David Airlie" <airlied@...il.com>,
 "Simona Vetter" <simona@...ll.ch>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "John Hubbard"
 <jhubbard@...dia.com>, "Joel Fernandes" <joelagnelf@...dia.com>, "Timur
 Tabi" <ttabi@...dia.com>, <linux-kernel@...r.kernel.org>,
 <nouveau@...ts.freedesktop.org>, "Lyude Paul" <lyude@...hat.com>
Subject: Re: [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@...dia.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@...dia.com>
> Signed-off-by: Alistair Popple <apopple@...dia.com>
> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
> Reviewed-by: Lyude Paul <lyude@...hat.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`.

Powered by blists - more mailing lists