Re: [PATCH 07/12] drm/tyr: Add generic slot manager

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 11 Feb 2026 17:37:08 -0800
Deborah Brouwer <deborah.brouwer@collabora.com> wrote:

> From: Boris Brezillon <boris.brezillon@collabora.com>
> 
> Introduce a generic slot manager to dynamically allocate limited hardware
> slots to software "seats". It can be used for both address space (AS) and
> command stream group (CSG) slots.
> 
> The slot manager initially assigns seats to its free slots. It then
> continues to reuse the same slot for a seat, as long as another seat
> did not start to use the slot in the interim.
> 
> When contention arises because all of the slots are allocated, the slot
> manager will lazily evict and reuse slots that have become idle (if any).
> 
> The seat state is protected using the LockedBy pattern with the same lock
> that guards the SlotManager. This ensures the seat state stays consistent
> across slot operations.
> 
> Hardware specific behaviour can be customized using the slot manager's
> `SlotOperations` trait.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> ---
>  drivers/gpu/drm/tyr/slot.rs | 359 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/tyr/tyr.rs  |   1 +
>  2 files changed, 360 insertions(+)
>  create mode 100644 drivers/gpu/drm/tyr/slot.rs
> 
> diff --git a/drivers/gpu/drm/tyr/slot.rs b/drivers/gpu/drm/tyr/slot.rs
> new file mode 100644
> index 000000000000..37bf8800a225
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/slot.rs
> @@ -0,0 +1,359 @@
> +// SPDX-License-Identifier: GPL-2.0 or MIT
> +
> +//! Slot management abstraction for limited hardware resources.
> +//!
> +//! This module provides a generic [`SlotManager`] that assigns limited hardware
> +//! slots to logical "seats". A seat represents an entity (such as a vm address
> +//! space) that needs access to a hardware slot.
> +//!
> +//! The [`SlotManager`] tracks slot allocation using sequence numbers to detect
> +//! when a seat's binding has been invalidated. When a seat requests activation,
> +//! the manager will either reuse the seat's existing slot (if still valid),
> +//! allocate a free slot (if any are available), or evict the oldest idle slot if any
> +//! slots are idle.
> +//!
> +//! Hardware-specific behavior is customized by implementing the [`SlotOperations`]
> +//! trait, which allows callbacks when slots are activated or evicted.
> +//!
> +//! This is primarily used for managing address space slots in the GPU, where
> +//! the number of hardware address space slots is limited.

I'd probably mention that we intend to use it for other stuff (Csg
slots), hence the generalization done here.

> +//!
> +//! [SlotOperations]: crate::slot::SlotOperations
> +//! [SlotManager]: crate::slot::SlotManager

Thanks a lot for adding some docs to my barebone initial implementation
and fixing the stuff I got wrong along the way. :D

> +#![allow(dead_code)]
> +
> +use core::{
> +    mem::take,
> +    ops::{
> +        Deref,
> +        DerefMut, //
> +    }, //
> +};
> +
> +use kernel::{
> +    prelude::*,
> +    sync::LockedBy, //
> +};
> +

Don't know what the doc rules are in rust, but for this sort of generic
layer, maybe we should provide extensive docs around objects, fields
and public functions. I see that most struct fields are documented, but
not the struct themselves. the enum doesn't seem to be documented, and
some of the public functions are not either. And that's all my fault,
because I gave you this raw piece of code without much doc (you added a
lot already). Just saying that, maybe now that things have settled
down, is a good time to add proper doc where it's missing.


/// Seat information.
///
/// This can't be accessed directly by the element embedding a `Seat`,
/// but is used by the generic slot manager logic to control residency
/// of a certain object on a hardware slot.
> +pub(crate) struct SeatInfo {
> +    /// Slot used by this seat.
       ///
       /// This index is only valid if the slot pointed by this index
       /// has its `SlotInfo::seqno` match SeatInfo::seqno. Otherwise,
       /// it means the object has been evicted from the hardware slot,
       /// and a new slot needs to be acquired to make this object
       /// resident again.
> +    slot: u8,
> +
> +    /// Sequence number encoding the last time this seat was active.
> +    /// We also use it to check if a slot is still bound to a seat.
> +    seqno: u64,
> +}
> +

/// Seat state
///
/// This is meant to be embedded in the object that wants to acquire
/// hardware slots. It also starts in the `Seat::NoSeat` state, and
/// the slot manager will change the object value when an active/evict
/// request to is issued. 
> +#[derive(Default)]
> +pub(crate) enum Seat {
> +    #[expect(clippy::enum_variant_names)]
       /// Resource is not resident.
       ///
       /// All objects start with a seat in that state. The seat also
       /// gets back to that state if the user requests eviction. It
       /// can also end up in that state next time an operation is done
       /// on an `Seat::Idle` seat and the slot managers finds out this
       /// object has been evicted from the slot.
> +    #[default]
> +    NoSeat,

       /// Resource is actively used and resident.
       ///
       /// When a seat is in that state, it can't be evicted, and the
       /// slot pointed by `SlotInfo::slot` is guaranteed to be reserved
       /// for this object as long as the seat stays active.
> +    Active(SeatInfo),

       /// Resource is idle and might or might not be resident.
       ///
       /// When a seat is in that state, we can't know for sure if the
       /// object is resident or evicted until the next request we issue
       /// to the slot manager. This tells the slot manager it can
       /// reclaim the underlying slot if needed.
       /// In order for the hardware to use this object again, the seat
       /// needs to be turned into an `Seat::Active` state again
       /// with a `SlotManager::activate()` call.
> +    Idle(SeatInfo),
> +}
> +
> +impl Seat {
       /// Get the slot index this seat is pointing to.
       ///
       /// If the seat is not `Seat::Active` we can't trust the
       /// `SeatInfo`. In that case `None` is returned, otherwise
       /// `Some(SeatInfo::slot)` is returned.
> +    pub(super) fn slot(&self) -> Option<u8> {
> +        match self {
> +            Self::Active(info) => Some(info.slot),
> +            _ => None,
> +        }
> +    }
> +}
> +

/// Trait describing the slot-related operations.
> +pub(crate) trait SlotOperations {
> +    type SlotData;
> +
> +    /// Called when a slot is being activated for a seat.
> +    ///
> +    /// This callback allows hardware-specific actions to be performed when a slot
> +    /// becomes active, such as updating hardware registers or invalidating caches.
> +    fn activate(&mut self, _slot_idx: usize, _slot_data: &Self::SlotData) -> Result {
> +        Ok(())
> +    }
> +
> +    /// Called when a slot is being evicted and freed.
> +    ///
> +    /// This callback allows hardware-specific cleanup when a slot is being
> +    /// completely freed, either explicitly or when an idle slot is being
> +    /// reused for a different seat. Any hardware state should be invalidated.
> +    fn evict(&mut self, _slot_idx: usize, _slot_data: &Self::SlotData) -> Result {
> +        Ok(())
> +    }
> +}
> +

/// Data attached to a slot.
> +struct SlotInfo<T> {
> +    /// Type specific data attached to a slot
> +    slot_data: T,
> +
> +    /// Sequence number from when this slot was last activated
> +    seqno: u64,
> +}
> +

/// Slot state.
> +#[derive(Default)]
> +enum Slot<T> {
    /// Slot is free.
    ///
    /// All slots start in this state when the slot manager is created.
> +    #[default]
> +    Free,

    /// Slot is active.
    ///
    /// When is that state, the slot is guaranteed to stay active
    /// for as long as the resource bound to it has its seat in the
    /// `Seat::Active` state. No new resource can be bound to it.
> +    Active(SlotInfo<T>),

    /// Slot is idle.
    ///
    /// Happens when the underlying resource has been flagged
    /// `Seat::Idle`. When in that state, the slot manager is allowed
    /// to evict the resource and re-assign the slot to someone else.
    /// This process involves updating the `SlotInfo::seqno` which
    /// will be checked against the `SeatInfo::seqno` in case the idle
    /// resource wants to become active again.
> +    Idle(SlotInfo<T>),
> +}
> +

/// Generic slot manager object.
///
/// It abstracts away all the churn around activeness/idleness tracking
/// and let the implementer of the SlotOperations trait focus on how to
/// make a resource active or evict it.
> +pub(crate) struct SlotManager<T: SlotOperations, const MAX_SLOTS: usize> {
> +    /// Manager specific data
> +    manager: T,
> +
> +    /// Number of slots actually available
> +    slot_count: usize,
> +
> +    /// Slots
> +    slots: [Slot<T::SlotData>; MAX_SLOTS],
> +
> +    /// Sequence number incremented each time a Seat is successfully activated
> +    use_seqno: u64,
> +}
> +
> +// A `Seat` protected by the same lock that is used to wrap the `SlotManager`.

Should this be

/// A `Seat` ....

?

> +type LockedSeat<T, const MAX_SLOTS: usize> = LockedBy<Seat, SlotManager<T, MAX_SLOTS>>;
> +
> +impl<T: SlotOperations, const MAX_SLOTS: usize> Unpin for SlotManager<T, MAX_SLOTS> {}

Do we really need to explicitly flag this type Unpin? I thought this
was the default if the struct is not pinned (and it's not AFAICT).

> +
> +impl<T: SlotOperations, const MAX_SLOTS: usize> SlotManager<T, MAX_SLOTS> {
    /// Creates a new slot manager.
> +    pub(crate) fn new(manager: T, slot_count: usize) -> Result<Self> {
> +        if slot_count == 0 {
> +            return Err(EINVAL);
> +        }
> +        if slot_count > MAX_SLOTS {
> +            return Err(EINVAL);
> +        }
> +        Ok(Self {
> +            manager,
> +            slot_count,
> +            slots: [const { Slot::Free }; MAX_SLOTS],
> +            use_seqno: 1,
> +        })
> +    }
> +
> +    fn record_active_slot(
> +        &mut self,
> +        slot_idx: usize,
> +        locked_seat: &LockedSeat<T, MAX_SLOTS>,
> +        slot_data: T::SlotData,
> +    ) -> Result {
> +        let cur_seqno = self.use_seqno;
> +
> +        *locked_seat.access_mut(self) = Seat::Active(SeatInfo {
> +            slot: slot_idx as u8,
> +            seqno: cur_seqno,
> +        });
> +
> +        self.slots[slot_idx] = Slot::Active(SlotInfo {
> +            slot_data,
> +            seqno: cur_seqno,
> +        });
> +
> +        self.use_seqno += 1;
> +        Ok(())
> +    }
> +
> +    fn activate_slot(
> +        &mut self,
> +        slot_idx: usize,
> +        locked_seat: &LockedSeat<T, MAX_SLOTS>,
> +        slot_data: T::SlotData,
> +    ) -> Result {
> +        self.manager.activate(slot_idx, &slot_data)?;
> +        self.record_active_slot(slot_idx, locked_seat, slot_data)
> +    }
> +
> +    fn allocate_slot(
> +        &mut self,
> +        locked_seat: &LockedSeat<T, MAX_SLOTS>,
> +        slot_data: T::SlotData,
> +    ) -> Result {
> +        let slots = &self.slots[..self.slot_count];
> +
> +        let mut idle_slot_idx = None;
> +        let mut idle_slot_seqno: u64 = 0;
> +
> +        for (slot_idx, slot) in slots.iter().enumerate() {
> +            match slot {
> +                Slot::Free => {
> +                    return self.activate_slot(slot_idx, locked_seat, slot_data);
> +                }
> +                Slot::Idle(slot_info) => {
> +                    if idle_slot_idx.is_none() || slot_info.seqno < idle_slot_seqno {
> +                        idle_slot_idx = Some(slot_idx);
> +                        idle_slot_seqno = slot_info.seqno;
> +                    }
> +                }
> +                Slot::Active(_) => (),
> +            }
> +        }
> +
> +        match idle_slot_idx {
> +            Some(slot_idx) => {
> +                // Lazily evict idle slot just before it is reused
> +                if let Slot::Idle(slot_info) = &self.slots[slot_idx] {
> +                    self.manager.evict(slot_idx, &slot_info.slot_data)?;
> +                }
> +                self.activate_slot(slot_idx, locked_seat, slot_data)
> +            }
> +            None => {
> +                pr_err!(
> +                    "Slot allocation failed: all {} slots in use\n",
> +                    self.slot_count
> +                );
> +                Err(EBUSY)
> +            }
> +        }
> +    }
> +
> +    fn idle_slot(&mut self, slot_idx: usize, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Result {
> +        let slot = take(&mut self.slots[slot_idx]);
> +
> +        if let Slot::Active(slot_info) = slot {
> +            self.slots[slot_idx] = Slot::Idle(SlotInfo {
> +                slot_data: slot_info.slot_data,
> +                seqno: slot_info.seqno,
> +            })
> +        };
> +
> +        *locked_seat.access_mut(self) = match locked_seat.access(self) {
> +            Seat::Active(seat_info) | Seat::Idle(seat_info) => Seat::Idle(SeatInfo {
> +                slot: seat_info.slot,
> +                seqno: seat_info.seqno,
> +            }),
> +            Seat::NoSeat => Seat::NoSeat,
> +        };
> +        Ok(())
> +    }
> +
> +    fn evict_slot(&mut self, slot_idx: usize, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Result {
> +        match &self.slots[slot_idx] {
> +            Slot::Active(slot_info) | Slot::Idle(slot_info) => {
> +                self.manager.evict(slot_idx, &slot_info.slot_data)?;
> +                take(&mut self.slots[slot_idx]);
> +            }
> +            _ => (),
> +        }
> +
> +        *locked_seat.access_mut(self) = Seat::NoSeat;
> +        Ok(())
> +    }
> +
> +    // Checks and updates the seat state based on the slot it points to
> +    // (if any). Returns a Seat with a value matching the slot state.
> +    fn check_seat(&mut self, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Seat {
> +        let new_seat = match locked_seat.access(self) {
> +            Seat::Active(seat_info) => {
> +                let old_slot_idx = seat_info.slot as usize;
> +                let slot = &self.slots[old_slot_idx];
> +
> +                if kernel::warn_on!(
> +                    !matches!(slot, Slot::Active(slot_info) if slot_info.seqno == seat_info.seqno)
> +                ) {
> +                    Seat::NoSeat
> +                } else {
> +                    Seat::Active(SeatInfo {
> +                        slot: seat_info.slot,
> +                        seqno: seat_info.seqno,
> +                    })
> +                }
> +            }
> +
> +            Seat::Idle(seat_info) => {
> +                let old_slot_idx = seat_info.slot as usize;
> +                let slot = &self.slots[old_slot_idx];
> +
> +                if !matches!(slot, Slot::Idle(slot_info) if slot_info.seqno == seat_info.seqno) {
> +                    Seat::NoSeat
> +                } else {
> +                    Seat::Idle(SeatInfo {
> +                        slot: seat_info.slot,
> +                        seqno: seat_info.seqno,
> +                    })
> +                }
> +            }
> +
> +            _ => Seat::NoSeat,
> +        };
> +
> +        // FIXME: Annoying manual copy. The original idea was to not add Copy+Clone to SeatInfo,
> +        // so that only slot.rs can change the seat state, but there might be better solutions
> +        // to prevent that.

Okay, I guess we want some inputs from Daniel and/or Alice on that one.

> +        match &new_seat {
> +            Seat::Active(seat_info) => {
> +                *locked_seat.access_mut(self) = Seat::Active(SeatInfo {
> +                    slot: seat_info.slot,
> +                    seqno: seat_info.seqno,
> +                })
> +            }
> +            Seat::Idle(seat_info) => {
> +                *locked_seat.access_mut(self) = Seat::Idle(SeatInfo {
> +                    slot: seat_info.slot,
> +                    seqno: seat_info.seqno,
> +                })
> +            }
> +            _ => *locked_seat.access_mut(self) = Seat::NoSeat,
> +        }
> +
> +        new_seat
> +    }
> +

    /// Make a resource active on any available/reclaimable slot.
    ///
    /// Will return an error if no slot is available/reclaimable, or if
    /// the reclaim failed.
> +    pub(crate) fn activate(
> +        &mut self,
> +        locked_seat: &LockedSeat<T, MAX_SLOTS>,
> +        slot_data: T::SlotData,
> +    ) -> Result {
> +        let seat = self.check_seat(locked_seat);
> +        match seat {
> +            Seat::Active(seat_info) | Seat::Idle(seat_info) => {
> +                // With lazy eviction, if seqno matches, the hardware state is still
> +                // valid for both Active and Idle slots, so just update our records
> +                self.record_active_slot(seat_info.slot as usize, locked_seat, slot_data)
> +            }
> +            _ => self.allocate_slot(locked_seat, slot_data),
> +        }
> +    }
> +
    /// Flag a resource idle.
    ///
    /// The slot manager can decide to reclaim the slot this resource
    /// was bound to at any point after function returns.
> +    // The idle() method will be used when we start adding support for user VMs
> +    #[expect(dead_code)]
> +    pub(crate) fn idle(&mut self, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Result {
> +        let seat = self.check_seat(locked_seat);
> +        if let Seat::Active(seat_info) = seat {
> +            self.idle_slot(seat_info.slot as usize, locked_seat)?;
> +        }
> +        Ok(())
> +    }
> +
> +    /// Evict a seat from its slot, freeing up the hardware resource.

I think I'd go:

    /// Evict a resource from its slot, and make this slot free again
    /// for other users.

> +    pub(crate) fn evict(&mut self, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Result {
> +        let seat = self.check_seat(locked_seat);
> +
> +        match seat {
> +            Seat::Active(seat_info) | Seat::Idle(seat_info) => {
> +                let slot_idx = seat_info.slot as usize;
> +
> +                self.evict_slot(slot_idx, locked_seat)?;
> +            }
> +            _ => (),
> +        }
> +
> +        Ok(())
> +    }
> +}
> +
> +impl<T: SlotOperations, const MAX_SLOTS: usize> Deref for SlotManager<T, MAX_SLOTS> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.manager
> +    }
> +}
> +
> +impl<T: SlotOperations, const MAX_SLOTS: usize> DerefMut for SlotManager<T, MAX_SLOTS> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        &mut self.manager
> +    }
> +}
> diff --git a/drivers/gpu/drm/tyr/tyr.rs b/drivers/gpu/drm/tyr/tyr.rs
> index 6eaa2135fe07..f54b997355e0 100644
> --- a/drivers/gpu/drm/tyr/tyr.rs
> +++ b/drivers/gpu/drm/tyr/tyr.rs
> @@ -12,6 +12,7 @@
>  mod gem;
>  mod gpu;
>  mod regs;
> +mod slot;
>  
>  kernel::module_platform_driver! {
>      type: TyrPlatformDeviceData,