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 29D6DCAC5BB for ; Wed, 1 Oct 2025 07:50:01 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7B7B210E342; Wed, 1 Oct 2025 07:50:00 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="NpvMZ/C/"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id D655A10E342 for ; Wed, 1 Oct 2025 07:49:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1759305000; x=1790841000; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=IHu4gsxPFL0J2fQWpgUm3fT4cMHpaMAUPX3e6Os6Zf0=; b=NpvMZ/C/0Aw1Gthe/GXRvGf+g7G7H0F0GYby76COWj3SpeXnbvY6i3Fa ZFdegbqGGw6Tf+eGas8fp3w0ZcfRvJV4/+KQVFfdpyrIqzh0UCzy9lT5D cxInl0PypCM1BKII+Z4+LMF6+qg2LWmP+fYbE+Me22ZTIINBWLP+SAjH9 jeBudArQEZ6SMlLmSaW7bQaz0IBWVXAv2PrcH7GWnLJKsh752Ay4c89US R5JkM7n5sp0IFYTxj3HFRajlaQhvfYQ2veDq0xbjERNfMHkpGkfuSu6Em YsrfCI6N90mueNVwOPSSR4uiKEJyW68Kg4jke+Vml/ocgG9lsmZNW6ZgH w==; X-CSE-ConnectionGUID: 2iZUm7K+T42nYBjf9/d+zQ== X-CSE-MsgGUID: 46sA7l/PTyuwPMLkMQfXYg== X-IronPort-AV: E=McAfee;i="6800,10657,11569"; a="61280812" X-IronPort-AV: E=Sophos;i="6.18,306,1751266800"; d="scan'208";a="61280812" Received: from fmviesa003.fm.intel.com ([10.60.135.143]) by orvoesa112.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Oct 2025 00:49:59 -0700 X-CSE-ConnectionGUID: k76Og9lgTNSo71XLtlmuag== X-CSE-MsgGUID: 3dn2zWiXQsCvmlddf0wQZQ== X-ExtLoop1: 1 Received: from unknown (HELO [10.102.88.152]) ([10.102.88.152]) by fmviesa003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Oct 2025 00:49:58 -0700 Message-ID: <375ebdd2-6548-4997-8b19-923321472ccb@linux.intel.com> Date: Wed, 1 Oct 2025 09:49:55 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] accel/ivpu: Rework bind/unbind of imported buffers To: Maciej Falkowski , dri-devel@lists.freedesktop.org Cc: oded.gabbay@gmail.com, jeff.hugo@oss.qualcomm.com, lizhi.hou@amd.com, Jacek Lawrynowicz References: <20250925145059.1446243-1-maciej.falkowski@linux.intel.com> Content-Language: en-US From: Karol Wachowski Organization: Intel Technology Poland sp. z o.o. - ul. Slowackiego 173, 80-298 Gdansk - KRS 101882 - NIP 957-07-52-316 In-Reply-To: <20250925145059.1446243-1-maciej.falkowski@linux.intel.com> Content-Type: text/plain; charset=UTF-8 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" Reviewed-by: Karol Wachowski On 9/25/2025 4:50 PM, Maciej Falkowski wrote: > From: Jacek Lawrynowicz > > Ensure that imported buffers are properly mapped and unmapped in > the same way as regular buffers to properly handle buffers during > device's bind and unbind operations to prevent resource leaks and > inconsistent buffer states. > > Imported buffers are now dma_mapped before submission and > dma_unmapped in ivpu_bo_unbind(), guaranteeing they are unmapped > when the device is unbound. > > Add also imported buffers to vdev->bo_list for consistent unmapping > on device unbind. The bo->ctx_id is set in open() so imported > buffers have a valid context ID. > > Debug logs have been updated to match the new code structure. > The function ivpu_bo_pin() has been renamed to ivpu_bo_bind() > to better reflect its purpose, and unbind tests have been refactored > for improved coverage and clarity. > > Signed-off-by: Jacek Lawrynowicz > Signed-off-by: Maciej Falkowski > --- > drivers/accel/ivpu/ivpu_gem.c | 90 ++++++++++++++++++++++------------- > drivers/accel/ivpu/ivpu_gem.h | 2 +- > drivers/accel/ivpu/ivpu_job.c | 2 +- > 3 files changed, 60 insertions(+), 34 deletions(-) > > diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c > index cceb07232e12..0cb48aff396c 100644 > --- a/drivers/accel/ivpu/ivpu_gem.c > +++ b/drivers/accel/ivpu/ivpu_gem.c > @@ -28,8 +28,8 @@ static const struct drm_gem_object_funcs ivpu_gem_funcs; > static inline void ivpu_dbg_bo(struct ivpu_device *vdev, struct ivpu_bo *bo, const char *action) > { > ivpu_dbg(vdev, BO, > - "%6s: bo %8p vpu_addr %9llx size %8zu ctx %d has_pages %d dma_mapped %d mmu_mapped %d wc %d imported %d\n", > - action, bo, bo->vpu_addr, ivpu_bo_size(bo), bo->ctx_id, > + "%6s: bo %8p size %9zu ctx %d vpu_addr %9llx pages %d sgt %d mmu_mapped %d wc %d imported %d\n", > + action, bo, ivpu_bo_size(bo), bo->ctx_id, bo->vpu_addr, > (bool)bo->base.pages, (bool)bo->base.sgt, bo->mmu_mapped, bo->base.map_wc, > (bool)drm_gem_is_imported(&bo->base.base)); > } > @@ -44,22 +44,46 @@ static inline void ivpu_bo_unlock(struct ivpu_bo *bo) > dma_resv_unlock(bo->base.base.resv); > } > > +static struct sg_table *ivpu_bo_map_attachment(struct ivpu_device *vdev, struct ivpu_bo *bo) > +{ > + struct sg_table *sgt = bo->base.sgt; > + > + drm_WARN_ON(&vdev->drm, !bo->base.base.import_attach); > + > + ivpu_bo_lock(bo); > + > + if (!sgt) { > + sgt = dma_buf_map_attachment(bo->base.base.import_attach, DMA_BIDIRECTIONAL); > + if (IS_ERR(sgt)) > + ivpu_err(vdev, "Failed to map BO in IOMMU: %ld\n", PTR_ERR(sgt)); > + else > + bo->base.sgt = sgt; > + } > + > + ivpu_bo_unlock(bo); > + > + return sgt; > +} > + > /* > - * ivpu_bo_pin() - pin the backing physical pages and map them to VPU. > + * ivpu_bo_bind() - pin the backing physical pages and map them to VPU. > * > * This function pins physical memory pages, then maps the physical pages > * to IOMMU address space and finally updates the VPU MMU page tables > * to allow the VPU to translate VPU address to IOMMU address. > */ > -int __must_check ivpu_bo_pin(struct ivpu_bo *bo) > +int __must_check ivpu_bo_bind(struct ivpu_bo *bo) > { > struct ivpu_device *vdev = ivpu_bo_to_vdev(bo); > struct sg_table *sgt; > int ret = 0; > > - ivpu_dbg_bo(vdev, bo, "pin"); > + ivpu_dbg_bo(vdev, bo, "bind"); > > - sgt = drm_gem_shmem_get_pages_sgt(&bo->base); > + if (bo->base.base.import_attach) > + sgt = ivpu_bo_map_attachment(vdev, bo); > + else > + sgt = drm_gem_shmem_get_pages_sgt(&bo->base); > if (IS_ERR(sgt)) { > ret = PTR_ERR(sgt); > ivpu_err(vdev, "Failed to map BO in IOMMU: %d\n", ret); > @@ -100,7 +124,9 @@ ivpu_bo_alloc_vpu_addr(struct ivpu_bo *bo, struct ivpu_mmu_context *ctx, > ret = ivpu_mmu_context_insert_node(ctx, range, ivpu_bo_size(bo), &bo->mm_node); > if (!ret) { > bo->ctx = ctx; > + bo->ctx_id = ctx->id; > bo->vpu_addr = bo->mm_node.start; > + ivpu_dbg_bo(vdev, bo, "vaddr"); > } else { > ivpu_err(vdev, "Failed to add BO to context %u: %d\n", ctx->id, ret); > } > @@ -116,7 +142,7 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) > { > struct ivpu_device *vdev = ivpu_bo_to_vdev(bo); > > - lockdep_assert(dma_resv_held(bo->base.base.resv) || !kref_read(&bo->base.base.refcount)); > + dma_resv_assert_held(bo->base.base.resv); > > if (bo->mmu_mapped) { > drm_WARN_ON(&vdev->drm, !bo->ctx); > @@ -135,9 +161,14 @@ static void ivpu_bo_unbind_locked(struct ivpu_bo *bo) > return; > > if (bo->base.sgt) { > - dma_unmap_sgtable(vdev->drm.dev, bo->base.sgt, DMA_BIDIRECTIONAL, 0); > - sg_free_table(bo->base.sgt); > - kfree(bo->base.sgt); > + if (bo->base.base.import_attach) { > + dma_buf_unmap_attachment(bo->base.base.import_attach, > + bo->base.sgt, DMA_BIDIRECTIONAL); > + } else { > + dma_unmap_sgtable(vdev->drm.dev, bo->base.sgt, DMA_BIDIRECTIONAL, 0); > + sg_free_table(bo->base.sgt); > + kfree(bo->base.sgt); > + } > bo->base.sgt = NULL; > } > } > @@ -163,6 +194,7 @@ void ivpu_bo_unbind_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_m > > struct drm_gem_object *ivpu_gem_create_object(struct drm_device *dev, size_t size) > { > + struct ivpu_device *vdev = to_ivpu_device(dev); > struct ivpu_bo *bo; > > if (size == 0 || !PAGE_ALIGNED(size)) > @@ -177,6 +209,11 @@ struct drm_gem_object *ivpu_gem_create_object(struct drm_device *dev, size_t siz > > INIT_LIST_HEAD(&bo->bo_list_node); > > + mutex_lock(&vdev->bo_list_lock); > + list_add_tail(&bo->bo_list_node, &vdev->bo_list); > + mutex_unlock(&vdev->bo_list_lock); > + > + ivpu_dbg(vdev, BO, " alloc: bo %8p size %9zu\n", bo, size); > return &bo->base.base; > } > > @@ -185,7 +222,6 @@ struct drm_gem_object *ivpu_gem_prime_import(struct drm_device *dev, > { > struct device *attach_dev = dev->dev; > struct dma_buf_attachment *attach; > - struct sg_table *sgt; > struct drm_gem_object *obj; > int ret; > > @@ -195,16 +231,10 @@ struct drm_gem_object *ivpu_gem_prime_import(struct drm_device *dev, > > get_dma_buf(dma_buf); > > - sgt = dma_buf_map_attachment_unlocked(attach, DMA_BIDIRECTIONAL); > - if (IS_ERR(sgt)) { > - ret = PTR_ERR(sgt); > - goto fail_detach; > - } > - > - obj = drm_gem_shmem_prime_import_sg_table(dev, attach, sgt); > + obj = drm_gem_shmem_prime_import_sg_table(dev, attach, NULL); > if (IS_ERR(obj)) { > ret = PTR_ERR(obj); > - goto fail_unmap; > + goto fail_detach; > } > > obj->import_attach = attach; > @@ -212,8 +242,6 @@ struct drm_gem_object *ivpu_gem_prime_import(struct drm_device *dev, > > return obj; > > -fail_unmap: > - dma_buf_unmap_attachment_unlocked(attach, sgt, DMA_BIDIRECTIONAL); > fail_detach: > dma_buf_detach(dma_buf, attach); > dma_buf_put(dma_buf); > @@ -221,7 +249,7 @@ struct drm_gem_object *ivpu_gem_prime_import(struct drm_device *dev, > return ERR_PTR(ret); > } > > -static struct ivpu_bo *ivpu_bo_alloc(struct ivpu_device *vdev, u64 size, u32 flags, u32 ctx_id) > +static struct ivpu_bo *ivpu_bo_alloc(struct ivpu_device *vdev, u64 size, u32 flags) > { > struct drm_gem_shmem_object *shmem; > struct ivpu_bo *bo; > @@ -239,16 +267,9 @@ static struct ivpu_bo *ivpu_bo_alloc(struct ivpu_device *vdev, u64 size, u32 fla > return ERR_CAST(shmem); > > bo = to_ivpu_bo(&shmem->base); > - bo->ctx_id = ctx_id; > bo->base.map_wc = flags & DRM_IVPU_BO_WC; > bo->flags = flags; > > - mutex_lock(&vdev->bo_list_lock); > - list_add_tail(&bo->bo_list_node, &vdev->bo_list); > - mutex_unlock(&vdev->bo_list_lock); > - > - ivpu_dbg_bo(vdev, bo, "alloc"); > - > return bo; > } > > @@ -282,6 +303,8 @@ static void ivpu_gem_bo_free(struct drm_gem_object *obj) > > ivpu_dbg_bo(vdev, bo, "free"); > > + drm_WARN_ON(&vdev->drm, list_empty(&bo->bo_list_node)); > + > mutex_lock(&vdev->bo_list_lock); > list_del(&bo->bo_list_node); > mutex_unlock(&vdev->bo_list_lock); > @@ -291,7 +314,10 @@ static void ivpu_gem_bo_free(struct drm_gem_object *obj) > drm_WARN_ON(&vdev->drm, ivpu_bo_size(bo) == 0); > drm_WARN_ON(&vdev->drm, bo->base.vaddr); > > + ivpu_bo_lock(bo); > ivpu_bo_unbind_locked(bo); > + ivpu_bo_unlock(bo); > + > drm_WARN_ON(&vdev->drm, bo->mmu_mapped); > drm_WARN_ON(&vdev->drm, bo->ctx); > > @@ -327,7 +353,7 @@ int ivpu_bo_create_ioctl(struct drm_device *dev, void *data, struct drm_file *fi > if (size == 0) > return -EINVAL; > > - bo = ivpu_bo_alloc(vdev, size, args->flags, file_priv->ctx.id); > + bo = ivpu_bo_alloc(vdev, size, args->flags); > if (IS_ERR(bo)) { > ivpu_err(vdev, "Failed to allocate BO: %pe (ctx %u size %llu flags 0x%x)", > bo, file_priv->ctx.id, args->size, args->flags); > @@ -361,7 +387,7 @@ ivpu_bo_create(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx, > drm_WARN_ON(&vdev->drm, !PAGE_ALIGNED(range->end)); > drm_WARN_ON(&vdev->drm, !PAGE_ALIGNED(size)); > > - bo = ivpu_bo_alloc(vdev, size, flags, IVPU_GLOBAL_CONTEXT_MMU_SSID); > + bo = ivpu_bo_alloc(vdev, size, flags); > if (IS_ERR(bo)) { > ivpu_err(vdev, "Failed to allocate BO: %pe (vpu_addr 0x%llx size %llu flags 0x%x)", > bo, range->start, size, flags); > @@ -372,7 +398,7 @@ ivpu_bo_create(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx, > if (ret) > goto err_put; > > - ret = ivpu_bo_pin(bo); > + ret = ivpu_bo_bind(bo); > if (ret) > goto err_put; > > diff --git a/drivers/accel/ivpu/ivpu_gem.h b/drivers/accel/ivpu/ivpu_gem.h > index 3a208f3bf0a6..54452eb8a41f 100644 > --- a/drivers/accel/ivpu/ivpu_gem.h > +++ b/drivers/accel/ivpu/ivpu_gem.h > @@ -24,7 +24,7 @@ struct ivpu_bo { > bool mmu_mapped; > }; > > -int ivpu_bo_pin(struct ivpu_bo *bo); > +int ivpu_bo_bind(struct ivpu_bo *bo); > void ivpu_bo_unbind_all_bos_from_context(struct ivpu_device *vdev, struct ivpu_mmu_context *ctx); > > struct drm_gem_object *ivpu_gem_create_object(struct drm_device *dev, size_t size); > diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c > index 044268d0fc87..17273c68f84c 100644 > --- a/drivers/accel/ivpu/ivpu_job.c > +++ b/drivers/accel/ivpu/ivpu_job.c > @@ -751,7 +751,7 @@ ivpu_job_prepare_bos_for_submit(struct drm_file *file, struct ivpu_job *job, u32 > > job->bos[i] = to_ivpu_bo(obj); > > - ret = ivpu_bo_pin(job->bos[i]); > + ret = ivpu_bo_bind(job->bos[i]); > if (ret) > return ret; > }