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

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

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())`.

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