> On 22 Feb 2026, at 14:57, Boris Brezillon <[email protected]>
> wrote:
>
> 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?
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.
— Daniel
