From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) (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 399563A8FE9 for ; Tue, 7 Apr 2026 10:44:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=148.251.105.195 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775558641; cv=none; b=CCU00rMm6lS9uU6IzAKspdL3FgBXL7QbqeCzZ29e5AOz07Nd6P+IHMigtsqT5Ae/R+v5l6V7VOc0c+HFIws4W7No+TU88XeuU3xFuYzHjS+sAF3wHT4mtEh2jZaASO/lQxo74uGLgQdtG+u+CNfc64nrY1Tr8LR1n2PvAZtBJ6Y= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775558641; c=relaxed/simple; bh=p1S/HVNwMAV8P+dvfRIHpJX9+NoRKf7j/cSNqRVpuKU=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=mRv4Stp+zzvuJsPk3m9avW/jFa5sPfXsOhao7xurdbxFX/6nMcEXK4XfJd3XBSuDpfCKzxWPKidT1+5329NFWK3/RpZ2g8A4Bn9keoblr0uRocf8XsjEh2R8SKBCn06gtjNFnnBD4W5W0stshEP++DbwrrEfqDN+gpXZkmQkxfk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=bYazggw5; arc=none smtp.client-ip=148.251.105.195 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 (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="bYazggw5" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1775558638; bh=p1S/HVNwMAV8P+dvfRIHpJX9+NoRKf7j/cSNqRVpuKU=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=bYazggw5dKqWf19ltuBmGwE4A/PHpOVbVd5WCKL6EPXaRaacB3MnKsoTPtx161HPp 3Zere+RPv4DeIz1iZ8ETmy4hbFEtpZGFQQl91R0NNu227MZafMb3jia1SypEFHwop8 MCOXT01TWSj8d67tUBHk4NO755a5A7DYxDxbY4asmAf+yoFH5lRDA21VwXyPb542J5 BDpgh40y3aAPqlfvZiKmk4+wJyjOF5bBXn6++3hgvqaVLfLcRSQjxmG/ErNVZmhbzp pdyGqHZQ4PrbfC+G/HBrNHemAiXtYfdqU1/rxayvmHVUqTXZEv5BEiKvzMlN8oT91E VFlFSnbfsrkWw== Received: from fedora (unknown [100.64.0.11]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by bali.collaboradmins.com (Postfix) with ESMTPSA id 0541D17E04C4; Tue, 7 Apr 2026 12:43:57 +0200 (CEST) Date: Tue, 7 Apr 2026 12:43:53 +0200 From: Boris Brezillon To: Liviu Dudau Cc: =?UTF-8?B?QWRyacOhbg==?= Larumbe , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Steven Price , kernel@collabora.com, Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter Subject: Re: [PATCH 1/2] drm/panthor: Extend VM locked region for remap case to be a superset Message-ID: <20260407124353.0364f536@fedora> In-Reply-To: References: <20260403172116.3424075-1-adrian.larumbe@collabora.com> Organization: Collabora X-Mailer: Claws Mail 4.3.1 (GTK 3.24.51; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, 7 Apr 2026 11:24:52 +0100 Liviu Dudau wrote: > On Fri, Apr 03, 2026 at 06:21:11PM +0100, Adri=C3=A1n Larumbe wrote: > > In the event of an sm_step_remap() that leads to a partial unmap of a > > transparent huge page, the new locked region required by an extended un= map > > might not be a superset of the original one. Then, if it leaves a porti= on > > of the initially requested one out, the ensuing map will trigger a warn= ing. > >=20 > > Signed-off-by: Adri=C3=A1n Larumbe > > Fixes: 8e7460eac786 ("drm/panthor: Support partial unmaps of huge pages= ") > > --- > > drivers/gpu/drm/panthor/panthor_mmu.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > >=20 > > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/pa= nthor/panthor_mmu.c > > index fa8b31df85c9..2b96359d3b94 100644 > > --- a/drivers/gpu/drm/panthor/panthor_mmu.c > > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c > > @@ -1709,6 +1709,19 @@ static int panthor_vm_lock_region(struct panthor= _vm *vm, u64 start, u64 size) > > start + size <=3D vm->locked_region.start + vm->locked_region.siz= e) > > return 0; > > =20 > > + /* sm_step_remap() may need a locked region that isn't a strict super= set > > + * of the original one because of having to extend unmap boundaries b= eyond > > + * it to deal with partial unmaps of transparent huge pages. What we = want > > + * in those cases is to lock the union of both regions. > > + */ > > + if (vm->locked_region.size) { =20 >=20 > Why is this check needed? We're updating the vm->locked_region.size later= anyway, and I think > we can cope with a locked region being of zero size when we are called, u= nless we consider that > to be a bug and we should check earlier for a zero value. It's here to detect if this is the initial lock (=3D=3D0), or the one that's done in sm_step_remap() (!=3D0). If we drop this conditional, the adjusted start will always be zero on the initial lock, because both vm->locked_region.start and vm->locked_region.size are zero in that case (see panthor_vm_unlock_region()). >=20 > > + u64 end =3D start + size; =20 >=20 > Like Boris pointed out, the calculations can be optimized so that we don'= t need this line. >=20 > > + > > + start =3D min(start, vm->locked_region.start); > > + size =3D max(vm->locked_region.start + > > + vm->locked_region.size, end) - start; =20 >=20 > If we have something like: >=20 > ..... [start .. start+size] ...... [vm->locked_region.start .. vm->locke= d_region.start + vm->locked_region.size] .... First off, that's not supposed to happen. The 3 cases that exist now are: [start .. start+size] [vm->locked_region.start .. vm->locked_region.start + vm->locked_region.s= ize] or [start .. start+size] [vm->locked_region.start .. vm->locked_region.start + vm->locked_region.siz= e] or [start .. start+size] [vm->locked_region.start .. vm->locked_region.start + vm->locked_region.si= ze] >=20 > we end up locking >=20 > ..... [start ................................................. vm->locke= d_region.start + vm->locked_region.size] .... >=20 > is that intended? We could add a WARN_ON() is there's no overlap between the previously locked region and the new one, but I'm not convinced this is something for panthor_vm_unlock_region() to enforce. Looks more like something the caller should check.