On Fri, 20 Feb 2026 13:55:31 -0300 Daniel Almeida <[email protected]> wrote:
> > On 20 Feb 2026, at 13:21, Boris Brezillon <[email protected]> > > wrote: > > > > On Fri, 20 Feb 2026 12:25:43 -0300 > > Daniel Almeida <[email protected]> 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.
