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

[Deborah Brouwer <deborah.brouwer@collabora.com>]

On Sun, Feb 22, 2026 at 03:46:48PM -0300, Daniel Almeida wrote:
> 
> 
> > On 22 Feb 2026, at 14:57, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > 
> > On Fri, 20 Feb 2026 13:55:31 -0300
> > Daniel Almeida <daniel.almeida@collabora.com> wrote:
> > 
> >>> On 20 Feb 2026, at 13:21, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >>> 
> >>> On Fri, 20 Feb 2026 12:25:43 -0300
> >>> Daniel Almeida <daniel.almeida@collabora.com> wrote:
> >>> 
> >>>>>> +
> >>>>>> +    // 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.    
> >>>> 
> >>>> Hm, I'd say we shouldn't implement Clone to avoid any possibility of holding on
> >>>> to stale state by duplicating it.  
> >>> 
> >>> Okay, so basically what we have now.
> >>> 
> >>>> 
> >>>> Why do we need to return Seat from this function? Can't we simply write
> >>>> locked_seat in place and not return anything?  
> >>> 
> >>> We do both actually. IIRC, the reason is that LockedBy borrows &self if
> >>> we want to read the locked_seat, which prevents us from calling methods
> >>> taking a &mut ref from a `match(locked_seat.access())`.  
> >> 
> >> 
> >> I am referring to this change:
> >> 
> >> --- a/drivers/gpu/drm/tyr/slot.rs
> >> +++ b/drivers/gpu/drm/tyr/slot.rs
> >> @@ -242,8 +242,8 @@ fn evict_slot(&mut self, slot_idx: usize, locked_seat: &LockedSeat<T, MAX_SLOTS>
> >>     }
> >> 
> >>     // Checks and updates the seat state based on the slot it points to
> >> -    fn check_seat(&mut self, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Seat {
> >> +    fn check_seat(&mut self, locked_seat: &LockedSeat<T, MAX_SLOTS>) {
> >>         let new_seat = match locked_seat.access(self) {
> >>             Seat::Active(seat_info) => {
> >>                 let old_slot_idx = seat_info.slot as usize;
> >> @@ -278,26 +278,7 @@ fn check_seat(&mut self, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Seat {
> >>             _ => 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.
> >> -        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
> >> +        *locked_seat.access_mut(self) = new_seat;
> > 
> > That one requires Copy support, or am I missing something?
> 
> No. Copy and Clone produce a new value and both the old and new values remain
> valid. The line above is moving a value into a new location, invalidating the
> previous one. This means that Copy is not required.
> 
> > 
> >>     }
> >> 
> >> Or even shorter:
> >> 
> >>    fn check_seat(&mut self, locked_seat: &LockedSeat<T, MAX_SLOTS>) {
> >>        let (slot_idx, seqno, is_active) = match locked_seat.access(self) {
> >>            Seat::Active(info) => (info.slot as usize, info.seqno, true),
> >>            Seat::Idle(info) => (info.slot as usize, info.seqno, false),
> >>            _ => return,
> >>        };
> >> 
> >>        let valid = if is_active {
> >>            !kernel::warn_on!(!matches!(&self.slots[slot_idx], Slot::Active(s) if s.seqno == seqno))
> >>        } else {
> >>            matches!(&self.slots[slot_idx], Slot::Idle(s) if s.seqno == seqno)
> >>        };
> >> 
> >>        if !valid {
> >>            *locked_seat.access_mut(self) = Seat::NoSeat;
> >>        }
> >>    }
> > 
> > Did you try that? Last I tried, I was hitting a wall because the caller
> > of check_seat() does a match on the updated seat, and inside this
> > match, it calls functions that need a &mut self, and the borrow checker
> > rightfully points the invalid &self then &mut self borrow pattern.
> 
> Yes, I compiled-tested all changes I suggested. My changes compile because they
> intentionally avoid doing what you said above.
> 
> The key here is that your borrows do not overlap anymore. The code I showed
> first borrows immutably, and then returns this tuple: (slot_idx, seqno, is_active).
> The immutable borrow then ends, since this tuple is copied (not borrowed) from
> its source. This benefits from the fact that primitive types are Copy.
> 
> Note that you can borrow both mutably and immutably in the same scope just fine. The
> borrows just can’t be alive at the same time. When the borrow checker rejects your code,
> it shows you why incompatible borrows overlap (i.e.: check the “first used here…later
> used here” part of the error).
> 
> > 
> >> 
> >> access vs access_mut() does not matter here: since the owner is &mut self
> >> anyways we know we have exclusive access to the LockedSeat throughout the whole
> >> function.
> > 
> > I agree, but LockedBy is picky, and last I tried I couldn't make it
> > work without the annoying update+return-copy-of-seat dance you see
> > here. Maybe I missed something obvious and it does indeed work with
> > your suggested changes, dunno.
> 
> Rewriting things so they pass the borrow checker is common in Rust. Sometimes
> it can be done rather easily; other times the design is just broken and needs
> to be reworked. Luckily this one fell in the first category.
> 
> This benefits from the fact that no one can race us between reading this tuple
> 
> (slot_idx, seqno, is_active) 
> 
> ..and using it. That’s because we’re taking &mut self as a proxy in LockedBy, so
> we’re sure we have exclusive access in this scope.
> 
> If you don’t have any complaints about the code I sent (i.e.: convoluted, wrong
> logic, etc), I suggest switching to it.

I’ve tested the short check_seat() function and can confirm that it correctly
compares Seat and Slot seqno as well as emits the kernel warning when we have a
mismatch on an active Seat. So I’ll the simplified check_seat() function in v2
unless there are any more issues to address.

> 
> 
> — Daniel
>