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