From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from sender4-pp-f112.zoho.com (sender4-pp-f112.zoho.com [136.143.188.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 98026A930 for ; Sun, 22 Feb 2026 18:47:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=pass smtp.client-ip=136.143.188.112 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1771786036; cv=pass; b=IKfHqg421d0gS/9PgoHwTTsevX6tYUz95RM0jxYGQBt+ywLU1DQdz8I9UlernlbHojivQabEKPR/K6bC8p5MBkCWKxLt+16wKt3XgLlZr679mY0IfXckn8EqG5drUyiKfeC888zhL+G86X6WbBVFrpY5tiJiSo3djx3Jsk9h2h0= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1771786036; c=relaxed/simple; bh=yLPU0XuXmr/nl8fYCuH0XnHyT/1ystk4QjQD62Vq628=; h=Content-Type:Mime-Version:Subject:From:In-Reply-To:Date:Cc: Message-Id:References:To; b=J4rZV3qzU6j87A1gEvCowj99UmCSZtfos3NFkV8vm4lKZbM/Vr5ah0TUP6FhAXGIH3xSHdY88a/zMKXbY8oN5+37Fl79Qcajj0q0W1Sn2q+YY1/cCiGzBzeRO+0T4OFO4fDRJSYKImxV+Z3h7xtlY2QkV+tS7XNLpk1/kZoxN40= ARC-Authentication-Results:i=2; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (1024-bit key) header.d=collabora.com header.i=daniel.almeida@collabora.com header.b=NRRiPpEc; arc=pass smtp.client-ip=136.143.188.112 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=collabora.com header.i=daniel.almeida@collabora.com header.b="NRRiPpEc" ARC-Seal: i=1; a=rsa-sha256; t=1771786026; cv=none; d=zohomail.com; s=zohoarc; b=blj0lbcKxk/rPCdW/NTs8tjIg+0a1m92rWF11DHIhPOmWIPpFVS+dGCkY32fZrZJnNSKf9P2tD1XJTfSbIip485bCx10HPFTIlSTBU9lT1jiPRZZ6SuSpf4GKrd0pYTFEbYfKdz4aMp43bSu5gk9pBb4ZqjJA5GjNNaNE7k808U= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1771786026; h=Content-Type:Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To; bh=ZcfktLejpbom6eLvi7iT6XCrb4oFOoFJP1yOuF4OcbI=; b=DAqJdUlWrAacrAR60jmBhrSQf7yf/6MlpUiRMStRLTLC60tm7+K2G8F3eEUAQRyKAALoHxckxPtPOzm9I2GOHwT5ifdE90UVliMuzPpTKmq/GwUnAbavAU8AsIILa1zBXcu4iwzx4YCfSV8FVj6skuz06iQTa2ZYMS7yDvJ3UxY= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass header.i=collabora.com; spf=pass smtp.mailfrom=daniel.almeida@collabora.com; dmarc=pass header.from= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1771786026; s=zohomail; d=collabora.com; i=daniel.almeida@collabora.com; h=Content-Type:Mime-Version:Subject:Subject:From:From:In-Reply-To:Date:Date:Cc:Cc:Content-Transfer-Encoding:Message-Id:Message-Id:References:To:To:Reply-To; bh=ZcfktLejpbom6eLvi7iT6XCrb4oFOoFJP1yOuF4OcbI=; b=NRRiPpEcpLH+AyoXwgrQkAD4ID7QvfQNobswNeByMJ3DUtP42HwOvgmm5+S0SCCq KWjSJ2Uv8H1g33v8bhzs1ONpOErrcuZ3EIHf8h/6LmQLnikTQ/60Uw7Tq4yVKswAz2A +wRSDNs1txL97ySGiuRAoqQvEFT7yIk/0znmSPXE= Received: by mx.zohomail.com with SMTPS id 17717860241071004.3358029822639; Sun, 22 Feb 2026 10:47:04 -0800 (PST) Content-Type: text/plain; charset=utf-8 Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\)) Subject: Re: [PATCH 07/12] drm/tyr: Add generic slot manager From: Daniel Almeida In-Reply-To: <20260222185743.02dabb26@fedora> Date: Sun, 22 Feb 2026 15:46:48 -0300 Cc: Deborah Brouwer , dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org, aliceryhl@google.com, beata.michalska@arm.com, lyude@redhat.com Content-Transfer-Encoding: quoted-printable Message-Id: References: <20260212013713.304343-1-deborah.brouwer@collabora.com> <20260212013713.304343-8-deborah.brouwer@collabora.com> <20260212111113.68778819@fedora> <1B40312F-694C-4154-ABF7-F96CE5576C0E@collabora.com> <20260220172154.4fe93c6d@fedora> <20260222185743.02dabb26@fedora> To: Boris Brezillon X-Mailer: Apple Mail (2.3826.700.81) X-ZohoMailClient: External > On 22 Feb 2026, at 14:57, Boris Brezillon = wrote: >=20 > On Fri, 20 Feb 2026 13:55:31 -0300 > Daniel Almeida wrote: >=20 >>> On 20 Feb 2026, at 13:21, Boris Brezillon = wrote: >>>=20 >>> On Fri, 20 Feb 2026 12:25:43 -0300 >>> Daniel Almeida wrote: >>>=20 >>>>>> + >>>>>> + // 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) -> Seat { >>>>>> + let new_seat =3D match locked_seat.access(self) { >>>>>> + Seat::Active(seat_info) =3D> { >>>>>> + let old_slot_idx =3D seat_info.slot as usize; >>>>>> + let slot =3D &self.slots[old_slot_idx]; >>>>>> + >>>>>> + if kernel::warn_on!( >>>>>> + !matches!(slot, Slot::Active(slot_info) if = slot_info.seqno =3D=3D seat_info.seqno) >>>>>> + ) { >>>>>> + Seat::NoSeat >>>>>> + } else { >>>>>> + Seat::Active(SeatInfo { >>>>>> + slot: seat_info.slot, >>>>>> + seqno: seat_info.seqno, >>>>>> + }) >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + Seat::Idle(seat_info) =3D> { >>>>>> + let old_slot_idx =3D seat_info.slot as usize; >>>>>> + let slot =3D &self.slots[old_slot_idx]; >>>>>> + >>>>>> + if !matches!(slot, Slot::Idle(slot_info) if = slot_info.seqno =3D=3D seat_info.seqno) { >>>>>> + Seat::NoSeat >>>>>> + } else { >>>>>> + Seat::Idle(SeatInfo { >>>>>> + slot: seat_info.slot, >>>>>> + seqno: seat_info.seqno, >>>>>> + }) >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + _ =3D> 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. =20 >>>>>=20 >>>>> Okay, I guess we want some inputs from Daniel and/or Alice on that = one. =20 >>>>=20 >>>> Hm, I'd say we shouldn't implement Clone to avoid any possibility = of holding on >>>> to stale state by duplicating it. =20 >>>=20 >>> Okay, so basically what we have now. >>>=20 >>>>=20 >>>> Why do we need to return Seat from this function? Can't we simply = write >>>> locked_seat in place and not return anything? =20 >>>=20 >>> 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())`. =20 >>=20 >>=20 >> I am referring to this change: >>=20 >> --- 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 >> } >>=20 >> // Checks and updates the seat state based on the slot it points = to >> - fn check_seat(&mut self, locked_seat: &LockedSeat) = -> Seat { >> + fn check_seat(&mut self, locked_seat: &LockedSeat) = { >> let new_seat =3D match locked_seat.access(self) { >> Seat::Active(seat_info) =3D> { >> let old_slot_idx =3D seat_info.slot as usize; >> @@ -278,26 +278,7 @@ fn check_seat(&mut self, locked_seat: = &LockedSeat) -> Seat { >> _ =3D> Seat::NoSeat, >> }; >>=20 >> - // 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) =3D> { >> - *locked_seat.access_mut(self) =3D = Seat::Active(SeatInfo { >> - slot: seat_info.slot, >> - seqno: seat_info.seqno, >> - }) >> - } >> - Seat::Idle(seat_info) =3D> { >> - *locked_seat.access_mut(self) =3D = Seat::Idle(SeatInfo { >> - slot: seat_info.slot, >> - seqno: seat_info.seqno, >> - }) >> - } >> - _ =3D> *locked_seat.access_mut(self) =3D Seat::NoSeat, >> - } >> - >> - new_seat >> + *locked_seat.access_mut(self) =3D new_seat; >=20 > 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. >=20 >> } >>=20 >> Or even shorter: >>=20 >> fn check_seat(&mut self, locked_seat: &LockedSeat) { >> let (slot_idx, seqno, is_active) =3D match = locked_seat.access(self) { >> Seat::Active(info) =3D> (info.slot as usize, info.seqno, = true), >> Seat::Idle(info) =3D> (info.slot as usize, info.seqno, = false), >> _ =3D> return, >> }; >>=20 >> let valid =3D if is_active { >> !kernel::warn_on!(!matches!(&self.slots[slot_idx], = Slot::Active(s) if s.seqno =3D=3D seqno)) >> } else { >> matches!(&self.slots[slot_idx], Slot::Idle(s) if s.seqno = =3D=3D seqno) >> }; >>=20 >> if !valid { >> *locked_seat.access_mut(self) =3D Seat::NoSeat; >> } >> } >=20 > 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=E2=80=99t be alive at the same time. When the borrow = checker rejects your code, it shows you why incompatible borrows overlap (i.e.: check the =E2=80=9Cfi= rst used here=E2=80=A6later used here=E2=80=9D part of the error). >=20 >>=20 >> 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. >=20 > 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)=20 ..and using it. That=E2=80=99s because we=E2=80=99re taking &mut self as = a proxy in LockedBy, so we=E2=80=99re sure we have exclusive access in this scope. If you don=E2=80=99t have any complaints about the code I sent (i.e.: = convoluted, wrong logic, etc), I suggest switching to it. =E2=80=94 Daniel