From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 061CCCEACEF for ; Mon, 17 Nov 2025 15:46:47 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 6131A10E3E1; Mon, 17 Nov 2025 15:46:46 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=collabora.com header.i=@collabora.com header.b="BXGlW1TO"; dkim-atps=neutral Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5DB2910E3E1 for ; Mon, 17 Nov 2025 15:46:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1763394404; bh=RjRAGalUSuZOAPkgTLxvvc9ygl4RedSR4hQAgYVJHMQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=BXGlW1TORxlCzGpYwgEBkzG7XBdR5ZMT/LZy6lpXFRirNNn603Wd7kgG8uSNUKfio 1UxI+/YKlEBzZFLuLbQf8dOucLdHntev05Xhvx5XEjbiQo06lM47pP8VkXaB+HzV6z 3XjwA5ELGWrgd3mvbaQ//GwHPVNY9uLD5oxcoLdoMYTrxoc+C9VtgNW29560b+Pfuu 6S8bYr1vu+9fjp1Rb+7ud4e6vXdOBqVT4lJ3VmBbueB63aaVVMp/1ETQ6eRlI1+L26 OnxMfJsRN7zvOCxnAMsiPnsoad6+W3ZUPIcrTTicIawPIRhR4/1npmG6kwFVx50h2P +6vjhFEih3YMA== Received: from fedora (unknown [IPv6:2a01:e0a:2c:6930:d919:a6e:5ea1:8a9f]) (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 A6ADA17E130A; Mon, 17 Nov 2025 16:46:43 +0100 (CET) Date: Mon, 17 Nov 2025 16:46:39 +0100 From: Boris Brezillon To: Steven Price Cc: Liviu Dudau , =?UTF-8?B?QWRyacOhbg==?= Larumbe , dri-devel@lists.freedesktop.org, Akash Goel , Karunika Choo , kernel@collabora.com Subject: Re: [PATCH v2 2/6] drm/panthor: Kill lock_region() Message-ID: <20251117164639.5018b9ca@fedora> In-Reply-To: References: <20251113103953.1519935-1-boris.brezillon@collabora.com> <20251113103953.1519935-3-boris.brezillon@collabora.com> Organization: Collabora X-Mailer: Claws Mail 4.3.1 (GTK 3.24.51; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Mon, 17 Nov 2025 12:44:58 +0000 Steven Price wrote: > On 13/11/2025 10:39, Boris Brezillon wrote: > > The meat in lock_region() is about packing a region range into a > > single u64. The rest is just a regular reg write plus a > > as_send_cmd_and_wait() call that can easily be inlined in > > mmu_hw_do_operation_locked(). > > > > v2: > > - New patch > > > > Signed-off-by: Boris Brezillon > > --- > > drivers/gpu/drm/panthor/panthor_mmu.c | 14 +++++--------- > > 1 file changed, 5 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c > > index 186048fc2c25..f109c1588186 100644 > > --- a/drivers/gpu/drm/panthor/panthor_mmu.c > > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c > > @@ -538,11 +538,9 @@ static int as_send_cmd_and_wait(struct panthor_device *ptdev, u32 as_nr, u32 cmd > > return status; > > } > > > > -static int lock_region(struct panthor_device *ptdev, u32 as_nr, > > - u64 region_start, u64 size) > > +static u64 pack_region_range(u64 region_start, u64 size) > > { > > u8 region_width; > > - u64 region; > > u64 region_end = region_start + size; > > > > if (!size) > Extra context: > > return 0; > Rather than skipping the lock for size==0 you are now performing a lock > with LOCKADDR=0. The best documentation I can find for that says (for > LOCKADDR_SIZE in the Midgard architecture): "Values in the range 0 to 10 > are undefined and should not be used". > > I think later versions upped the minimum to 14 (log2(1<<15)-1) for > reasons due to supporting larger page sizes. > > While I suspect this might work (since we don't actually need a lock > region when size==0) I don't think we should be relying on undefined > behaviour. > > I think the simple solution is to move the size==0 check up into the caller. Right. I addressed that in patch 4 (which basically removes mmu_hw_do_operation_locked()), but I can do it here too. > > Thanks, > Steve > > > @@ -565,11 +563,7 @@ static int lock_region(struct panthor_device *ptdev, u32 as_nr, > > */ > > region_start &= GENMASK_ULL(63, region_width); > > > > - region = region_width | region_start; > > - > > - /* Lock the region that needs to be updated */ > > - gpu_write64(ptdev, AS_LOCKADDR(as_nr), region); > > - return as_send_cmd_and_wait(ptdev, as_nr, AS_COMMAND_LOCK); > > + return region_width | region_start; > > } > > > > static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr, > > @@ -602,7 +596,9 @@ static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr, > > * power it up > > */ > > > > - ret = lock_region(ptdev, as_nr, iova, size); > > + /* Lock the region that needs to be updated */ > > + gpu_write64(ptdev, AS_LOCKADDR(as_nr), pack_region_range(iova, size)); > > + ret = as_send_cmd_and_wait(ptdev, as_nr, AS_COMMAND_LOCK); > > if (ret) > > return ret; > > >