From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 874832D46C1 for ; Fri, 11 Jul 2025 13:30:28 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752240630; cv=none; b=fSnu5Mty3OBTMv5nT/l6cV6qzDDlPJMowSka6JfOgeuAeL8Ekd1k/6U41MuUYWQnu0Pkpmw3gpi6bFzsMu1cuqQfWu8zfPtEfrGhZEGjTfA8VHAjkcu+M6/jyl5+gFyKRWznwYfQK6uGLdUK2KhJfNb39wAdLdJpLwY6UzcpH2Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752240630; c=relaxed/simple; bh=OWq/B46bvHzJuSIugLEfaxgLXbeYVEteNBl7nRsZ8IE=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=ieK1Z4gQC22KC2a3dROY4bUhleptjG/GgAS9xyQ211+HAGBM67Gm4/DciysIBvZaxx1s/sxh4uziTW8b3cRHEI12c9WXM96o3T35QtDK0gUDdPXK+LaXCHtgNpbS9NJHQdGBnU6WVYGqG/c2NmJEfwEmMbfZWx8C+IVbRw4GDuA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 33A7F106F; Fri, 11 Jul 2025 06:30:17 -0700 (PDT) Received: from [10.1.33.14] (e122027.cambridge.arm.com [10.1.33.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D2E323F6A8; Fri, 11 Jul 2025 06:30:23 -0700 (PDT) Message-ID: Date: Fri, 11 Jul 2025 14:30:21 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 1/7] drm/panthor: Add support for atomic page table updates To: Caterina Shablia , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Frank Binns , Matt Coster , Karol Herbst , Lyude Paul , Danilo Krummrich , Boris Brezillon , Liviu Dudau , Lucas De Marchi , =?UTF-8?Q?Thomas_Hellstr=C3=B6m?= , Rodrigo Vivi Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, nouveau@lists.freedesktop.org, intel-xe@lists.freedesktop.org, asahi@lists.linux.dev, Asahi Lina References: <20250707170442.1437009-1-caterina.shablia@collabora.com> <20250707170442.1437009-2-caterina.shablia@collabora.com> From: Steven Price Content-Language: en-GB In-Reply-To: <20250707170442.1437009-2-caterina.shablia@collabora.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 07/07/2025 18:04, Caterina Shablia wrote: > From: Boris Brezillon > > Move the lock/flush_mem operations around the gpuvm_sm_map() calls so > we can implement true atomic page updates, where any access in the > locked range done by the GPU has to wait for the page table updates > to land before proceeding. > > This is needed for vkQueueBindSparse(), so we can replace the dummy > page mapped over the entire object by actual BO backed pages in an atomic > way. > > Signed-off-by: Boris Brezillon > Signed-off-by: Caterina Shablia > --- > drivers/gpu/drm/panthor/panthor_mmu.c | 65 +++++++++++++++++++++++++-- > 1 file changed, 62 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c > index b39ea6acc6a9..9caaba03c5eb 100644 > --- a/drivers/gpu/drm/panthor/panthor_mmu.c > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c > @@ -387,6 +387,15 @@ struct panthor_vm { > * flagged as faulty as a result. > */ > bool unhandled_fault; > + > + /** @locked_region: Information about the currently locked region currently. */ > + struct { > + /** @locked_region.start: Start of the locked region. */ > + u64 start; > + > + /** @locked_region.size: Size of the locked region. */ > + u64 size; > + } locked_region; > }; > > /** > @@ -775,6 +784,10 @@ int panthor_vm_active(struct panthor_vm *vm) > } > > ret = panthor_mmu_as_enable(vm->ptdev, vm->as.id, transtab, transcfg, vm->memattr); > + if (!ret && vm->locked_region.size) { > + lock_region(ptdev, vm->as.id, vm->locked_region.start, vm->locked_region.size); > + ret = wait_ready(ptdev, vm->as.id); > + } Do we need to do this? It seems odd to restore a MMU context and immediately set a lock region. Is there a good reason we can't just WARN_ON if there's a lock region set in panthor_vm_idle()? I think we need to briefly take vm->op_lock to ensure synchronisation but that doesn't seem a big issue. Or perhaps there's a good reason that I'm missing? > > out_make_active: > if (!ret) { > @@ -902,6 +915,9 @@ static int panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size) > struct io_pgtable_ops *ops = vm->pgtbl_ops; > u64 offset = 0; > > + drm_WARN_ON(&ptdev->base, > + (iova < vm->locked_region.start) || > + (iova + size > vm->locked_region.start + vm->locked_region.size)); > drm_dbg(&ptdev->base, "unmap: as=%d, iova=%llx, len=%llx", vm->as.id, iova, size); > > while (offset < size) { > @@ -915,13 +931,12 @@ static int panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size) > iova + offset + unmapped_sz, > iova + offset + pgsize * pgcount, > iova, iova + size); > - panthor_vm_flush_range(vm, iova, offset + unmapped_sz); > return -EINVAL; > } > offset += unmapped_sz; > } > > - return panthor_vm_flush_range(vm, iova, size); > + return 0; > } > > static int > @@ -938,6 +953,10 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot, > if (!size) > return 0; > > + drm_WARN_ON(&ptdev->base, > + (iova < vm->locked_region.start) || > + (iova + size > vm->locked_region.start + vm->locked_region.size)); > + > for_each_sgtable_dma_sg(sgt, sgl, count) { > dma_addr_t paddr = sg_dma_address(sgl); > size_t len = sg_dma_len(sgl); > @@ -985,7 +1004,7 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot, > offset = 0; > } > > - return panthor_vm_flush_range(vm, start_iova, iova - start_iova); > + return 0; > } > > static int flags_to_prot(u32 flags) > @@ -1654,6 +1673,38 @@ static const char *access_type_name(struct panthor_device *ptdev, > } > } > > +static int panthor_vm_lock_region(struct panthor_vm *vm, u64 start, u64 size) > +{ > + struct panthor_device *ptdev = vm->ptdev; > + int ret = 0; > + > + mutex_lock(&ptdev->mmu->as.slots_lock); > + drm_WARN_ON(&ptdev->base, vm->locked_region.start || vm->locked_region.size); > + vm->locked_region.start = start; > + vm->locked_region.size = size; > + if (vm->as.id >= 0) { > + lock_region(ptdev, vm->as.id, start, size); > + ret = wait_ready(ptdev, vm->as.id); > + } > + mutex_unlock(&ptdev->mmu->as.slots_lock); > + > + return ret; > +} > + > +static void panthor_vm_unlock_region(struct panthor_vm *vm) > +{ > + struct panthor_device *ptdev = vm->ptdev; > + > + mutex_lock(&ptdev->mmu->as.slots_lock); > + if (vm->as.id >= 0) { > + write_cmd(ptdev, vm->as.id, AS_COMMAND_FLUSH_MEM); > + drm_WARN_ON(&ptdev->base, wait_ready(ptdev, vm->as.id)); > + } > + vm->locked_region.start = 0; > + vm->locked_region.size = 0; > + mutex_unlock(&ptdev->mmu->as.slots_lock); > +} Do we need to include a drm_dev_enter() somewhere here? I note that panthor_vm_flush_range() has one and you've effectively replaced that code with the above. Thanks, Steve > + > static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status) > { > bool has_unhandled_faults = false; > @@ -2179,6 +2230,11 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op, > > mutex_lock(&vm->op_lock); > vm->op_ctx = op; > + > + ret = panthor_vm_lock_region(vm, op->va.addr, op->va.range); > + if (ret) > + goto out; > + > switch (op_type) { > case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: > if (vm->unusable) { > @@ -2199,6 +2255,9 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op, > break; > } > > + panthor_vm_unlock_region(vm); > + > +out: > if (ret && flag_vm_unusable_on_failure) > vm->unusable = true; >