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

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

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?

>      }
> 
> 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.

> 
> 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.