Hi,
Some major GPUVM changes landed just before v8 of the PowerVR series. Since v8
went in rather quickly (review process was finished otherwise) I haven't had the
chance to review the subsequent code changes.
Hence, this series with a few fixes in this context. Plus a minor GPUVM patch to
make the drm_gpuvm_prepare_* helpers useful for PowerVR.
- Danilo
Danilo Krummrich (5):
drm/imagination: vm: prevent duplicate drm_gpuvm_bo instances
drm/imagination: vm: check for drm_gpuvm_range_valid()
drm/imagination: vm: fix drm_gpuvm reference count
drm/gpuvm: fall back to drm_exec_lock_obj()
drm/imagination: vm: make use of GPUVM's drm_exec helper
drivers/gpu/drm/drm_gpuvm.c | 36 ++++++++++++++++++++--
drivers/gpu/drm/imagination/pvr_vm.c | 46 +++++++++++++++-------------
drivers/gpu/drm/imagination/pvr_vm.h | 3 +-
include/drm/drm_gpuvm.h | 23 ++------------
4 files changed, 63 insertions(+), 45 deletions(-)
base-commit: 46990918f35c1bf6e367cf8e0423e7344fec9fcb
--
2.42.0
Use drm_gpuvm_bo_obtain() instead of drm_gpuvm_bo_create(). The latter
should only be used in conjunction with drm_gpuvm_bo_obtain_prealloc().
drm_gpuvm_bo_obtain() re-uses existing instances of a given VM and BO
combination, whereas drm_gpuvm_bo_create() would always create a new
instance of struct drm_gpuvm_bo and hence leave us with duplicates.
Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/imagination/pvr_vm.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index 3ad1366294b9..09d481c575b0 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -224,6 +224,7 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
struct pvr_gem_object *pvr_obj, u64 offset,
u64 device_addr, u64 size)
{
+ struct drm_gem_object *obj = gem_from_pvr_gem(pvr_obj);
const bool is_user = vm_ctx == vm_ctx->pvr_dev->kernel_vm_ctx;
const u64 pvr_obj_size = pvr_gem_object_size(pvr_obj);
struct sg_table *sgt;
@@ -245,10 +246,11 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
bind_op->type = PVR_VM_BIND_TYPE_MAP;
- bind_op->gpuvm_bo = drm_gpuvm_bo_create(&vm_ctx->gpuvm_mgr,
- gem_from_pvr_gem(pvr_obj));
- if (!bind_op->gpuvm_bo)
- return -ENOMEM;
+ dma_resv_lock(obj->resv, NULL);
+ bind_op->gpuvm_bo = drm_gpuvm_bo_obtain(&vm_ctx->gpuvm_mgr, obj);
+ dma_resv_unlock(obj->resv);
+ if (IS_ERR(bind_op->gpuvm_bo))
+ return PTR_ERR(bind_op->gpuvm_bo);
bind_op->new_va = kzalloc(sizeof(*bind_op->new_va), GFP_KERNEL);
bind_op->prev_va = kzalloc(sizeof(*bind_op->prev_va), GFP_KERNEL);
--
2.42.0
Fall back to drm_exec_lock_obj() if num_fences is zero for the
drm_gpuvm_prepare_* function family.
Otherwise dma_resv_reserve_fences() would actually allocate slots even
though num_fences is zero.
Cc: Christian König <[email protected]>
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++++++++++++++++++++++---
include/drm/drm_gpuvm.h | 23 +++--------------------
2 files changed, 36 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index 54f5e8851de5..d1d1c2379e44 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -1085,6 +1085,36 @@ drm_gpuvm_put(struct drm_gpuvm *gpuvm)
}
EXPORT_SYMBOL_GPL(drm_gpuvm_put);
+static int
+exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj,
+ unsigned int num_fences)
+{
+ return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) :
+ drm_exec_lock_obj(exec, obj);
+}
+
+/**
+ * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
+ * @gpuvm: the &drm_gpuvm
+ * @exec: the &drm_exec context
+ * @num_fences: the amount of &dma_fences to reserve
+ *
+ * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object.
+ *
+ * Using this function directly, it is the drivers responsibility to call
+ * drm_exec_init() and drm_exec_fini() accordingly.
+ *
+ * Returns: 0 on success, negative error code on failure.
+ */
+int
+drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
+ struct drm_exec *exec,
+ unsigned int num_fences)
+{
+ return exec_prepare_obj(exec, gpuvm->r_obj, num_fences);
+}
+EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm);
+
static int
__drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
struct drm_exec *exec,
@@ -1095,7 +1125,7 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
int ret = 0;
for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) {
- ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences);
+ ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
if (ret)
break;
}
@@ -1116,7 +1146,7 @@ drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm,
drm_gpuvm_resv_assert_held(gpuvm);
list_for_each_entry(vm_bo, &gpuvm->extobj.list, list.entry.extobj) {
- ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences);
+ ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
if (ret)
break;
@@ -1186,7 +1216,7 @@ drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
drm_gpuvm_for_each_va_range(va, gpuvm, addr, end) {
struct drm_gem_object *obj = va->gem.obj;
- ret = drm_exec_prepare_obj(exec, obj, num_fences);
+ ret = exec_prepare_obj(exec, obj, num_fences);
if (ret)
return ret;
}
diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
index f94fec9a8517..b3f82ec7fb17 100644
--- a/include/drm/drm_gpuvm.h
+++ b/include/drm/drm_gpuvm.h
@@ -544,26 +544,9 @@ struct drm_gpuvm_exec {
} extra;
};
-/**
- * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
- * @gpuvm: the &drm_gpuvm
- * @exec: the &drm_exec context
- * @num_fences: the amount of &dma_fences to reserve
- *
- * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object.
- *
- * Using this function directly, it is the drivers responsibility to call
- * drm_exec_init() and drm_exec_fini() accordingly.
- *
- * Returns: 0 on success, negative error code on failure.
- */
-static inline int
-drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
- struct drm_exec *exec,
- unsigned int num_fences)
-{
- return drm_exec_prepare_obj(exec, gpuvm->r_obj, num_fences);
-}
+int drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
+ struct drm_exec *exec,
+ unsigned int num_fences);
int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
struct drm_exec *exec,
--
2.42.0
Extend pvr_device_addr_and_size_are_valid() by the corresponding GPUVM
sanity checks. This includes a, previously missing, overflow check for
the base address and size of the requested mapping.
Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/imagination/pvr_vm.c | 9 ++++++---
drivers/gpu/drm/imagination/pvr_vm.h | 3 ++-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index 09d481c575b0..1e89092c3dcc 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -239,7 +239,7 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
return -EINVAL;
}
- if (!pvr_device_addr_and_size_are_valid(device_addr, size) ||
+ if (!pvr_device_addr_and_size_are_valid(vm_ctx, device_addr, size) ||
offset & ~PAGE_MASK || size & ~PAGE_MASK ||
offset >= pvr_obj_size || offset_plus_size > pvr_obj_size)
return -EINVAL;
@@ -295,7 +295,7 @@ pvr_vm_bind_op_unmap_init(struct pvr_vm_bind_op *bind_op,
{
int err;
- if (!pvr_device_addr_and_size_are_valid(device_addr, size))
+ if (!pvr_device_addr_and_size_are_valid(vm_ctx, device_addr, size))
return -EINVAL;
bind_op->type = PVR_VM_BIND_TYPE_UNMAP;
@@ -505,6 +505,7 @@ pvr_device_addr_is_valid(u64 device_addr)
/**
* pvr_device_addr_and_size_are_valid() - Tests whether a device-virtual
* address and associated size are both valid.
+ * @vm_ctx: Target VM context.
* @device_addr: Virtual device address to test.
* @size: Size of the range based at @device_addr to test.
*
@@ -523,9 +524,11 @@ pvr_device_addr_is_valid(u64 device_addr)
* * %false otherwise.
*/
bool
-pvr_device_addr_and_size_are_valid(u64 device_addr, u64 size)
+pvr_device_addr_and_size_are_valid(struct pvr_vm_context *vm_ctx,
+ u64 device_addr, u64 size)
{
return pvr_device_addr_is_valid(device_addr) &&
+ drm_gpuvm_range_valid(&vm_ctx->gpuvm_mgr, device_addr, size) &&
size != 0 && (size & ~PVR_DEVICE_PAGE_MASK) == 0 &&
(device_addr + size <= PVR_PAGE_TABLE_ADDR_SPACE_SIZE);
}
diff --git a/drivers/gpu/drm/imagination/pvr_vm.h b/drivers/gpu/drm/imagination/pvr_vm.h
index cf8b97553dc8..f2a6463f2b05 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.h
+++ b/drivers/gpu/drm/imagination/pvr_vm.h
@@ -29,7 +29,8 @@ struct drm_exec;
/* Functions defined in pvr_vm.c */
bool pvr_device_addr_is_valid(u64 device_addr);
-bool pvr_device_addr_and_size_are_valid(u64 device_addr, u64 size);
+bool pvr_device_addr_and_size_are_valid(struct pvr_vm_context *vm_ctx,
+ u64 device_addr, u64 size);
struct pvr_vm_context *pvr_vm_create_context(struct pvr_device *pvr_dev,
bool is_userspace_context);
--
2.42.0
The driver specific reference count indicates whether the VM should be
teared down, whereas GPUVM's reference count indicates whether the VM
structure can finally be freed.
Hence, free the VM structure in pvr_gpuvm_free() and drop the last
GPUVM reference after tearing down the VM. Generally, this prevents
lifetime issues such as the VM being freed as long as drm_gpuvm_bo
structures still hold references to the VM.
Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/imagination/pvr_vm.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index 1e89092c3dcc..e0d74d9a6190 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -64,6 +64,12 @@ struct pvr_vm_context {
struct drm_gem_object dummy_gem;
};
+static inline
+struct pvr_vm_context *to_pvr_vm_context(struct drm_gpuvm *gpuvm)
+{
+ return container_of(gpuvm, struct pvr_vm_context, gpuvm_mgr);
+}
+
struct pvr_vm_context *pvr_vm_context_get(struct pvr_vm_context *vm_ctx)
{
if (vm_ctx)
@@ -535,7 +541,7 @@ pvr_device_addr_and_size_are_valid(struct pvr_vm_context *vm_ctx,
void pvr_gpuvm_free(struct drm_gpuvm *gpuvm)
{
-
+ kfree(to_pvr_vm_context(gpuvm));
}
static const struct drm_gpuvm_ops pvr_vm_gpuva_ops = {
@@ -655,12 +661,11 @@ pvr_vm_context_release(struct kref *ref_count)
WARN_ON(pvr_vm_unmap(vm_ctx, vm_ctx->gpuvm_mgr.mm_start,
vm_ctx->gpuvm_mgr.mm_range));
- drm_gpuvm_put(&vm_ctx->gpuvm_mgr);
pvr_mmu_context_destroy(vm_ctx->mmu_ctx);
drm_gem_private_object_fini(&vm_ctx->dummy_gem);
mutex_destroy(&vm_ctx->lock);
- kfree(vm_ctx);
+ drm_gpuvm_put(&vm_ctx->gpuvm_mgr);
}
/**
--
2.42.0
Make use of GPUVM's drm_exec helper functions preventing direct access
to GPUVM internal data structures, such as the external object list.
This is especially important to ensure following the locking rules
around the GPUVM external object list.
Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
Signed-off-by: Danilo Krummrich <[email protected]>
---
drivers/gpu/drm/imagination/pvr_vm.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index e0d74d9a6190..3f7888f5cc53 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -337,27 +337,21 @@ static int
pvr_vm_bind_op_lock_resvs(struct drm_exec *exec, struct pvr_vm_bind_op *bind_op)
{
drm_exec_until_all_locked(exec) {
- struct drm_gem_object *r_obj = &bind_op->vm_ctx->dummy_gem;
struct drm_gpuvm *gpuvm = &bind_op->vm_ctx->gpuvm_mgr;
struct pvr_gem_object *pvr_obj = bind_op->pvr_obj;
- struct drm_gpuvm_bo *gpuvm_bo;
/* Acquire lock on the vm_context's reserve object. */
- int err = drm_exec_lock_obj(exec, r_obj);
+ int err = drm_gpuvm_prepare_vm(gpuvm, exec, 0);
drm_exec_retry_on_contention(exec);
if (err)
return err;
/* Acquire lock on all BOs in the context. */
- list_for_each_entry(gpuvm_bo, &gpuvm->extobj.list,
- list.entry.extobj) {
- err = drm_exec_lock_obj(exec, gpuvm_bo->obj);
-
- drm_exec_retry_on_contention(exec);
- if (err)
- return err;
- }
+ err = drm_gpuvm_prepare_objects(gpuvm, exec, 0);
+ drm_exec_retry_on_contention(exec);
+ if (err)
+ return err;
/* Unmap operations don't have an object to lock. */
if (!pvr_obj)
--
2.42.0
Hi Christian,
do you think it makes sense to handle this in drm_exec_prepare_obj() or
even dma_resv_reserve_fences() even?
- Danilo
On 11/25/23 00:36, Danilo Krummrich wrote:
> Fall back to drm_exec_lock_obj() if num_fences is zero for the
> drm_gpuvm_prepare_* function family.
>
> Otherwise dma_resv_reserve_fences() would actually allocate slots even
> though num_fences is zero.
>
> Cc: Christian König <[email protected]>
> Signed-off-by: Danilo Krummrich <[email protected]>
> ---
> drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++++++++++++++++++++++---
> include/drm/drm_gpuvm.h | 23 +++--------------------
> 2 files changed, 36 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index 54f5e8851de5..d1d1c2379e44 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -1085,6 +1085,36 @@ drm_gpuvm_put(struct drm_gpuvm *gpuvm)
> }
> EXPORT_SYMBOL_GPL(drm_gpuvm_put);
>
> +static int
> +exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj,
> + unsigned int num_fences)
> +{
> + return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) :
> + drm_exec_lock_obj(exec, obj);
> +}
> +
> +/**
> + * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
> + * @gpuvm: the &drm_gpuvm
> + * @exec: the &drm_exec context
> + * @num_fences: the amount of &dma_fences to reserve
> + *
> + * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object.
> + *
> + * Using this function directly, it is the drivers responsibility to call
> + * drm_exec_init() and drm_exec_fini() accordingly.
> + *
> + * Returns: 0 on success, negative error code on failure.
> + */
> +int
> +drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
> + struct drm_exec *exec,
> + unsigned int num_fences)
> +{
> + return exec_prepare_obj(exec, gpuvm->r_obj, num_fences);
> +}
> +EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm);
> +
> static int
> __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
> struct drm_exec *exec,
> @@ -1095,7 +1125,7 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
> int ret = 0;
>
> for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) {
> - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences);
> + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
> if (ret)
> break;
> }
> @@ -1116,7 +1146,7 @@ drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm,
>
> drm_gpuvm_resv_assert_held(gpuvm);
> list_for_each_entry(vm_bo, &gpuvm->extobj.list, list.entry.extobj) {
> - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences);
> + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
> if (ret)
> break;
>
> @@ -1186,7 +1216,7 @@ drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
> drm_gpuvm_for_each_va_range(va, gpuvm, addr, end) {
> struct drm_gem_object *obj = va->gem.obj;
>
> - ret = drm_exec_prepare_obj(exec, obj, num_fences);
> + ret = exec_prepare_obj(exec, obj, num_fences);
> if (ret)
> return ret;
> }
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index f94fec9a8517..b3f82ec7fb17 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -544,26 +544,9 @@ struct drm_gpuvm_exec {
> } extra;
> };
>
> -/**
> - * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
> - * @gpuvm: the &drm_gpuvm
> - * @exec: the &drm_exec context
> - * @num_fences: the amount of &dma_fences to reserve
> - *
> - * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object.
> - *
> - * Using this function directly, it is the drivers responsibility to call
> - * drm_exec_init() and drm_exec_fini() accordingly.
> - *
> - * Returns: 0 on success, negative error code on failure.
> - */
> -static inline int
> -drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
> - struct drm_exec *exec,
> - unsigned int num_fences)
> -{
> - return drm_exec_prepare_obj(exec, gpuvm->r_obj, num_fences);
> -}
> +int drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
> + struct drm_exec *exec,
> + unsigned int num_fences);
>
> int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
> struct drm_exec *exec,
Hi,
Thank you for the patches.
On Sat, 2023-11-25 at 00:36 +0100, Danilo Krummrich wrote:
> Hi,
>
> Some major GPUVM changes landed just before v8 of the PowerVR series. Since v8
> went in rather quickly (review process was finished otherwise) I haven't had the
> chance to review the subsequent code changes.
>
> Hence, this series with a few fixes in this context. Plus a minor GPUVM patch to
> make the drm_gpuvm_prepare_* helpers useful for PowerVR.
>
> - Danilo
>
>
> Danilo Krummrich (5):
> drm/imagination: vm: prevent duplicate drm_gpuvm_bo instances
> drm/imagination: vm: check for drm_gpuvm_range_valid()
> drm/imagination: vm: fix drm_gpuvm reference count
> drm/gpuvm: fall back to drm_exec_lock_obj()
> drm/imagination: vm: make use of GPUVM's drm_exec helper
>
> drivers/gpu/drm/drm_gpuvm.c | 36 ++++++++++++++++++++--
> drivers/gpu/drm/imagination/pvr_vm.c | 46 +++++++++++++++-------------
> drivers/gpu/drm/imagination/pvr_vm.h | 3 +-
> include/drm/drm_gpuvm.h | 23 ++------------
> 4 files changed, 63 insertions(+), 45 deletions(-)
>
>
> base-commit: 46990918f35c1bf6e367cf8e0423e7344fec9fcb
For the series:
Tested-by: Frank Binns <[email protected]>
I'll leave it to Donald to do the review.
Thanks
Frank
Am 25.11.23 um 00:40 schrieb Danilo Krummrich:
> Hi Christian,
>
> do you think it makes sense to handle this in drm_exec_prepare_obj() or
> even dma_resv_reserve_fences() even?
IIRC for drm_exec the discussion has gone a bit back and forth between
handling 0 and having a separate function which doesn't allocate fences.
We ended up with the solution using separate calls since you either know
that you don't need fences (because you for example only need to look
something up) or you need fences and eventually calculate the number you
need dynamically and if you then end up with 0 it's a bug.
So to sum it up the conclusion was that it's more defensive to complain
about 0 fences to reserve (which reminds me that
dma_resv_reserve_fences() still needs to get a warning for 0 fences as
well).
Regards,
Christian.
>
> - Danilo
>
> On 11/25/23 00:36, Danilo Krummrich wrote:
>> Fall back to drm_exec_lock_obj() if num_fences is zero for the
>> drm_gpuvm_prepare_* function family.
>>
>> Otherwise dma_resv_reserve_fences() would actually allocate slots even
>> though num_fences is zero.
>>
>> Cc: Christian König <[email protected]>
>> Signed-off-by: Danilo Krummrich <[email protected]>
>> ---
>> drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++++++++++++++++++++++---
>> include/drm/drm_gpuvm.h | 23 +++--------------------
>> 2 files changed, 36 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
>> index 54f5e8851de5..d1d1c2379e44 100644
>> --- a/drivers/gpu/drm/drm_gpuvm.c
>> +++ b/drivers/gpu/drm/drm_gpuvm.c
>> @@ -1085,6 +1085,36 @@ drm_gpuvm_put(struct drm_gpuvm *gpuvm)
>> }
>> EXPORT_SYMBOL_GPL(drm_gpuvm_put);
>> +static int
>> +exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj,
>> + unsigned int num_fences)
>> +{
>> + return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) :
>> + drm_exec_lock_obj(exec, obj);
>> +}
>> +
>> +/**
>> + * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
>> + * @gpuvm: the &drm_gpuvm
>> + * @exec: the &drm_exec context
>> + * @num_fences: the amount of &dma_fences to reserve
>> + *
>> + * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object.
>> + *
>> + * Using this function directly, it is the drivers responsibility to
>> call
>> + * drm_exec_init() and drm_exec_fini() accordingly.
>> + *
>> + * Returns: 0 on success, negative error code on failure.
>> + */
>> +int
>> +drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
>> + struct drm_exec *exec,
>> + unsigned int num_fences)
>> +{
>> + return exec_prepare_obj(exec, gpuvm->r_obj, num_fences);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm);
>> +
>> static int
>> __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
>> struct drm_exec *exec,
>> @@ -1095,7 +1125,7 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm
>> *gpuvm,
>> int ret = 0;
>> for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) {
>> - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences);
>> + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
>> if (ret)
>> break;
>> }
>> @@ -1116,7 +1146,7 @@ drm_gpuvm_prepare_objects_locked(struct
>> drm_gpuvm *gpuvm,
>> drm_gpuvm_resv_assert_held(gpuvm);
>> list_for_each_entry(vm_bo, &gpuvm->extobj.list,
>> list.entry.extobj) {
>> - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences);
>> + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
>> if (ret)
>> break;
>> @@ -1186,7 +1216,7 @@ drm_gpuvm_prepare_range(struct drm_gpuvm
>> *gpuvm, struct drm_exec *exec,
>> drm_gpuvm_for_each_va_range(va, gpuvm, addr, end) {
>> struct drm_gem_object *obj = va->gem.obj;
>> - ret = drm_exec_prepare_obj(exec, obj, num_fences);
>> + ret = exec_prepare_obj(exec, obj, num_fences);
>> if (ret)
>> return ret;
>> }
>> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
>> index f94fec9a8517..b3f82ec7fb17 100644
>> --- a/include/drm/drm_gpuvm.h
>> +++ b/include/drm/drm_gpuvm.h
>> @@ -544,26 +544,9 @@ struct drm_gpuvm_exec {
>> } extra;
>> };
>> -/**
>> - * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
>> - * @gpuvm: the &drm_gpuvm
>> - * @exec: the &drm_exec context
>> - * @num_fences: the amount of &dma_fences to reserve
>> - *
>> - * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object.
>> - *
>> - * Using this function directly, it is the drivers responsibility to
>> call
>> - * drm_exec_init() and drm_exec_fini() accordingly.
>> - *
>> - * Returns: 0 on success, negative error code on failure.
>> - */
>> -static inline int
>> -drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
>> - struct drm_exec *exec,
>> - unsigned int num_fences)
>> -{
>> - return drm_exec_prepare_obj(exec, gpuvm->r_obj, num_fences);
>> -}
>> +int drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
>> + struct drm_exec *exec,
>> + unsigned int num_fences);
>> int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
>> struct drm_exec *exec,
>
Hi,
On 11/27/23 14:52, Christian König wrote:
> Am 25.11.23 um 00:40 schrieb Danilo Krummrich:
>> Hi Christian,
>>
>> do you think it makes sense to handle this in drm_exec_prepare_obj() or
>> even dma_resv_reserve_fences() even?
>
> IIRC for drm_exec the discussion has gone a bit back and forth between handling 0 and having a separate function which doesn't allocate fences.
>
> We ended up with the solution using separate calls since you either know that you don't need fences (because you for example only need to look something up) or you need fences and eventually calculate the number you need dynamically and if you then end up with 0 it's a bug.
I don't have a strong opinion on that. Though, in GPUVM I'd probably still need some wrapper like
+exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj,
+ unsigned int num_fences)
+{
+ return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) :
+ drm_exec_lock_obj(exec, obj);
+}
to prevent either duplicate code or rather unnecessary complicated abstractions.
And I'm not sure it's super nice that drm_gpuvm_prepare_objects() allows zero fences, whereas drm_exec_prepare_obj() does not.
>
> So to sum it up the conclusion was that it's more defensive to complain about 0 fences to reserve (which reminds me that dma_resv_reserve_fences() still needs to get a warning for 0 fences as well).
What's the logic you'd want to apply there? WARN() but still allocate at least 4 slots or WARN() and return doing nothing?
- Danilo
>
> Regards,
> Christian.
>
>>
>> - Danilo
>>
>> On 11/25/23 00:36, Danilo Krummrich wrote:
>>> Fall back to drm_exec_lock_obj() if num_fences is zero for the
>>> drm_gpuvm_prepare_* function family.
>>>
>>> Otherwise dma_resv_reserve_fences() would actually allocate slots even
>>> though num_fences is zero.
>>>
>>> Cc: Christian König <[email protected]>
>>> Signed-off-by: Danilo Krummrich <[email protected]>
>>> ---
>>> drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++++++++++++++++++++++---
>>> include/drm/drm_gpuvm.h | 23 +++--------------------
>>> 2 files changed, 36 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
>>> index 54f5e8851de5..d1d1c2379e44 100644
>>> --- a/drivers/gpu/drm/drm_gpuvm.c
>>> +++ b/drivers/gpu/drm/drm_gpuvm.c
>>> @@ -1085,6 +1085,36 @@ drm_gpuvm_put(struct drm_gpuvm *gpuvm)
>>> }
>>> EXPORT_SYMBOL_GPL(drm_gpuvm_put);
>>> +static int
>>> +exec_prepare_obj(struct drm_exec *exec, struct drm_gem_object *obj,
>>> + unsigned int num_fences)
>>> +{
>>> + return num_fences ? drm_exec_prepare_obj(exec, obj, num_fences) :
>>> + drm_exec_lock_obj(exec, obj);
>>> +}
>>> +
>>> +/**
>>> + * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
>>> + * @gpuvm: the &drm_gpuvm
>>> + * @exec: the &drm_exec context
>>> + * @num_fences: the amount of &dma_fences to reserve
>>> + *
>>> + * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object.
>>> + *
>>> + * Using this function directly, it is the drivers responsibility to call
>>> + * drm_exec_init() and drm_exec_fini() accordingly.
>>> + *
>>> + * Returns: 0 on success, negative error code on failure.
>>> + */
>>> +int
>>> +drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
>>> + struct drm_exec *exec,
>>> + unsigned int num_fences)
>>> +{
>>> + return exec_prepare_obj(exec, gpuvm->r_obj, num_fences);
>>> +}
>>> +EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm);
>>> +
>>> static int
>>> __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
>>> struct drm_exec *exec,
>>> @@ -1095,7 +1125,7 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
>>> int ret = 0;
>>> for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) {
>>> - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences);
>>> + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
>>> if (ret)
>>> break;
>>> }
>>> @@ -1116,7 +1146,7 @@ drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm,
>>> drm_gpuvm_resv_assert_held(gpuvm);
>>> list_for_each_entry(vm_bo, &gpuvm->extobj.list, list.entry.extobj) {
>>> - ret = drm_exec_prepare_obj(exec, vm_bo->obj, num_fences);
>>> + ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
>>> if (ret)
>>> break;
>>> @@ -1186,7 +1216,7 @@ drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
>>> drm_gpuvm_for_each_va_range(va, gpuvm, addr, end) {
>>> struct drm_gem_object *obj = va->gem.obj;
>>> - ret = drm_exec_prepare_obj(exec, obj, num_fences);
>>> + ret = exec_prepare_obj(exec, obj, num_fences);
>>> if (ret)
>>> return ret;
>>> }
>>> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
>>> index f94fec9a8517..b3f82ec7fb17 100644
>>> --- a/include/drm/drm_gpuvm.h
>>> +++ b/include/drm/drm_gpuvm.h
>>> @@ -544,26 +544,9 @@ struct drm_gpuvm_exec {
>>> } extra;
>>> };
>>> -/**
>>> - * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
>>> - * @gpuvm: the &drm_gpuvm
>>> - * @exec: the &drm_exec context
>>> - * @num_fences: the amount of &dma_fences to reserve
>>> - *
>>> - * Calls drm_exec_prepare_obj() for the GPUVMs dummy &drm_gem_object.
>>> - *
>>> - * Using this function directly, it is the drivers responsibility to call
>>> - * drm_exec_init() and drm_exec_fini() accordingly.
>>> - *
>>> - * Returns: 0 on success, negative error code on failure.
>>> - */
>>> -static inline int
>>> -drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
>>> - struct drm_exec *exec,
>>> - unsigned int num_fences)
>>> -{
>>> - return drm_exec_prepare_obj(exec, gpuvm->r_obj, num_fences);
>>> -}
>>> +int drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
>>> + struct drm_exec *exec,
>>> + unsigned int num_fences);
>>> int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
>>> struct drm_exec *exec,
>>
>
Hi Danilo,
Thanks for reviewing this file again and thanks for clarifying the use of these functions.
Reviewed-by: Donald Robson <[email protected]>
On Sat, 2023-11-25 at 00:36 +0100, Danilo Krummrich wrote:
> *** CAUTION: This email originates from a source not known to Imagination Technologies. Think before you click a link or open an attachment ***
>
> Use drm_gpuvm_bo_obtain() instead of drm_gpuvm_bo_create(). The latter
> should only be used in conjunction with drm_gpuvm_bo_obtain_prealloc().
>
> drm_gpuvm_bo_obtain() re-uses existing instances of a given VM and BO
> combination, whereas drm_gpuvm_bo_create() would always create a new
> instance of struct drm_gpuvm_bo and hence leave us with duplicates.
>
> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
> Signed-off-by: Danilo Krummrich <[email protected]>
> ---
> drivers/gpu/drm/imagination/pvr_vm.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 3ad1366294b9..09d481c575b0 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -224,6 +224,7 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
> struct pvr_gem_object *pvr_obj, u64 offset,
> u64 device_addr, u64 size)
> {
> + struct drm_gem_object *obj = gem_from_pvr_gem(pvr_obj);
> const bool is_user = vm_ctx == vm_ctx->pvr_dev->kernel_vm_ctx;
> const u64 pvr_obj_size = pvr_gem_object_size(pvr_obj);
> struct sg_table *sgt;
> @@ -245,10 +246,11 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
>
> bind_op->type = PVR_VM_BIND_TYPE_MAP;
>
> - bind_op->gpuvm_bo = drm_gpuvm_bo_create(&vm_ctx->gpuvm_mgr,
> - gem_from_pvr_gem(pvr_obj));
> - if (!bind_op->gpuvm_bo)
> - return -ENOMEM;
> + dma_resv_lock(obj->resv, NULL);
> + bind_op->gpuvm_bo = drm_gpuvm_bo_obtain(&vm_ctx->gpuvm_mgr, obj);
> + dma_resv_unlock(obj->resv);
> + if (IS_ERR(bind_op->gpuvm_bo))
> + return PTR_ERR(bind_op->gpuvm_bo);
>
> bind_op->new_va = kzalloc(sizeof(*bind_op->new_va), GFP_KERNEL);
> bind_op->prev_va = kzalloc(sizeof(*bind_op->prev_va), GFP_KERNEL);
Thanks Danilo. It's obvious now you've pointed it out!
Reviewed-by: Donald Robson <[email protected]>
On Sat, 2023-11-25 at 00:36 +0100, Danilo Krummrich wrote:
> *** CAUTION: This email originates from a source not known to Imagination Technologies. Think before you click a link or open an attachment ***
>
> The driver specific reference count indicates whether the VM should be
> teared down, whereas GPUVM's reference count indicates whether the VM
> structure can finally be freed.
>
> Hence, free the VM structure in pvr_gpuvm_free() and drop the last
> GPUVM reference after tearing down the VM. Generally, this prevents
> lifetime issues such as the VM being freed as long as drm_gpuvm_bo
> structures still hold references to the VM.
>
> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
> Signed-off-by: Danilo Krummrich <[email protected]>
> ---
> drivers/gpu/drm/imagination/pvr_vm.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 1e89092c3dcc..e0d74d9a6190 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -64,6 +64,12 @@ struct pvr_vm_context {
> struct drm_gem_object dummy_gem;
> };
>
> +static inline
> +struct pvr_vm_context *to_pvr_vm_context(struct drm_gpuvm *gpuvm)
> +{
> + return container_of(gpuvm, struct pvr_vm_context, gpuvm_mgr);
> +}
> +
> struct pvr_vm_context *pvr_vm_context_get(struct pvr_vm_context *vm_ctx)
> {
> if (vm_ctx)
> @@ -535,7 +541,7 @@ pvr_device_addr_and_size_are_valid(struct pvr_vm_context *vm_ctx,
>
> void pvr_gpuvm_free(struct drm_gpuvm *gpuvm)
> {
> -
> + kfree(to_pvr_vm_context(gpuvm));
> }
>
> static const struct drm_gpuvm_ops pvr_vm_gpuva_ops = {
> @@ -655,12 +661,11 @@ pvr_vm_context_release(struct kref *ref_count)
> WARN_ON(pvr_vm_unmap(vm_ctx, vm_ctx->gpuvm_mgr.mm_start,
> vm_ctx->gpuvm_mgr.mm_range));
>
> - drm_gpuvm_put(&vm_ctx->gpuvm_mgr);
> pvr_mmu_context_destroy(vm_ctx->mmu_ctx);
> drm_gem_private_object_fini(&vm_ctx->dummy_gem);
> mutex_destroy(&vm_ctx->lock);
>
> - kfree(vm_ctx);
> + drm_gpuvm_put(&vm_ctx->gpuvm_mgr);
> }
>
> /**
Thanks Danilo.
Reviewed-by: Donald Robson <[email protected]>
On Sat, 2023-11-25 at 00:36 +0100, Danilo Krummrich wrote:
> *** CAUTION: This email originates from a source not known to Imagination Technologies. Think before you click a link or open an attachment ***
>
> Extend pvr_device_addr_and_size_are_valid() by the corresponding GPUVM
> sanity checks. This includes a, previously missing, overflow check for
> the base address and size of the requested mapping.
>
> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
> Signed-off-by: Danilo Krummrich <[email protected]>
> ---
> drivers/gpu/drm/imagination/pvr_vm.c | 9 ++++++---
> drivers/gpu/drm/imagination/pvr_vm.h | 3 ++-
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 09d481c575b0..1e89092c3dcc 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -239,7 +239,7 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
> return -EINVAL;
> }
>
> - if (!pvr_device_addr_and_size_are_valid(device_addr, size) ||
> + if (!pvr_device_addr_and_size_are_valid(vm_ctx, device_addr, size) ||
> offset & ~PAGE_MASK || size & ~PAGE_MASK ||
> offset >= pvr_obj_size || offset_plus_size > pvr_obj_size)
> return -EINVAL;
> @@ -295,7 +295,7 @@ pvr_vm_bind_op_unmap_init(struct pvr_vm_bind_op *bind_op,
> {
> int err;
>
> - if (!pvr_device_addr_and_size_are_valid(device_addr, size))
> + if (!pvr_device_addr_and_size_are_valid(vm_ctx, device_addr, size))
> return -EINVAL;
>
> bind_op->type = PVR_VM_BIND_TYPE_UNMAP;
> @@ -505,6 +505,7 @@ pvr_device_addr_is_valid(u64 device_addr)
> /**
> * pvr_device_addr_and_size_are_valid() - Tests whether a device-virtual
> * address and associated size are both valid.
> + * @vm_ctx: Target VM context.
> * @device_addr: Virtual device address to test.
> * @size: Size of the range based at @device_addr to test.
> *
> @@ -523,9 +524,11 @@ pvr_device_addr_is_valid(u64 device_addr)
> * * %false otherwise.
> */
> bool
> -pvr_device_addr_and_size_are_valid(u64 device_addr, u64 size)
> +pvr_device_addr_and_size_are_valid(struct pvr_vm_context *vm_ctx,
> + u64 device_addr, u64 size)
> {
> return pvr_device_addr_is_valid(device_addr) &&
> + drm_gpuvm_range_valid(&vm_ctx->gpuvm_mgr, device_addr, size) &&
> size != 0 && (size & ~PVR_DEVICE_PAGE_MASK) == 0 &&
> (device_addr + size <= PVR_PAGE_TABLE_ADDR_SPACE_SIZE);
> }
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.h b/drivers/gpu/drm/imagination/pvr_vm.h
> index cf8b97553dc8..f2a6463f2b05 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.h
> +++ b/drivers/gpu/drm/imagination/pvr_vm.h
> @@ -29,7 +29,8 @@ struct drm_exec;
> /* Functions defined in pvr_vm.c */
>
> bool pvr_device_addr_is_valid(u64 device_addr);
> -bool pvr_device_addr_and_size_are_valid(u64 device_addr, u64 size);
> +bool pvr_device_addr_and_size_are_valid(struct pvr_vm_context *vm_ctx,
> + u64 device_addr, u64 size);
>
> struct pvr_vm_context *pvr_vm_create_context(struct pvr_device *pvr_dev,
> bool is_userspace_context);
Hi Danilo,
Apologies - I guess I should have submitted a patch to handle zero fences in your
locking functions with the final patch series.
On Sat, 2023-11-25 at 00:36 +0100, Danilo Krummrich wrote:
> *** CAUTION: This email originates from a source not known to Imagination Technologies. Think before you click a link or open an attachment ***
>
> Make use of GPUVM's drm_exec helper functions preventing direct access
> to GPUVM internal data structures, such as the external object list.
>
> This is especially important to ensure following the locking rules
> around the GPUVM external object list.
>
> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
> Signed-off-by: Danilo Krummrich <[email protected]>
> ---
> drivers/gpu/drm/imagination/pvr_vm.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index e0d74d9a6190..3f7888f5cc53 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -337,27 +337,21 @@ static int
> pvr_vm_bind_op_lock_resvs(struct drm_exec *exec, struct pvr_vm_bind_op *bind_op)
> {
> drm_exec_until_all_locked(exec) {
> - struct drm_gem_object *r_obj = &bind_op->vm_ctx->dummy_gem;
> struct drm_gpuvm *gpuvm = &bind_op->vm_ctx->gpuvm_mgr;
> struct pvr_gem_object *pvr_obj = bind_op->pvr_obj;
> - struct drm_gpuvm_bo *gpuvm_bo;
>
> /* Acquire lock on the vm_context's reserve object. */
> - int err = drm_exec_lock_obj(exec, r_obj);
> + int err = drm_gpuvm_prepare_vm(gpuvm, exec, 0);
>
> drm_exec_retry_on_contention(exec);
> if (err)
> return err;
>
> /* Acquire lock on all BOs in the context. */
> - list_for_each_entry(gpuvm_bo, &gpuvm->extobj.list,
> - list.entry.extobj) {
> - err = drm_exec_lock_obj(exec, gpuvm_bo->obj);
> -
> - drm_exec_retry_on_contention(exec);
> - if (err)
> - return err;
> - }
> + err = drm_gpuvm_prepare_objects(gpuvm, exec, 0);
> + drm_exec_retry_on_contention(exec);
> + if (err)
> + return err;
Before I discovered the problem when not reserving fences, I was trying to use
drm_gpuvm_exec_lock() with vm_exec->extra.fn() for the part below. Is there
a reason not to do that now?
Many thanks,
Donald
>
> /* Unmap operations don't have an object to lock. */
> if (!pvr_obj)
On Sat, 25 Nov 2023 00:36:38 +0100, Danilo Krummrich wrote:
> The driver specific reference count indicates whether the VM should be
> teared down, whereas GPUVM's reference count indicates whether the VM
> structure can finally be freed.
>
> Hence, free the VM structure in pvr_gpuvm_free() and drop the last
> GPUVM reference after tearing down the VM. Generally, this prevents
> lifetime issues such as the VM being freed as long as drm_gpuvm_bo
> structures still hold references to the VM.
>
> [...]
Applied to drm/drm-misc (drm-misc-next).
Thanks!
Maxime
On Sat, 25 Nov 2023 00:36:36 +0100, Danilo Krummrich wrote:
> Use drm_gpuvm_bo_obtain() instead of drm_gpuvm_bo_create(). The latter
> should only be used in conjunction with drm_gpuvm_bo_obtain_prealloc().
>
> drm_gpuvm_bo_obtain() re-uses existing instances of a given VM and BO
> combination, whereas drm_gpuvm_bo_create() would always create a new
> instance of struct drm_gpuvm_bo and hence leave us with duplicates.
>
> [...]
Applied to drm/drm-misc (drm-misc-next).
Thanks!
Maxime
On Sat, 25 Nov 2023 00:36:37 +0100, Danilo Krummrich wrote:
> Extend pvr_device_addr_and_size_are_valid() by the corresponding GPUVM
> sanity checks. This includes a, previously missing, overflow check for
> the base address and size of the requested mapping.
>
>
Applied to drm/drm-misc (drm-misc-next).
Thanks!
Maxime
On 11/28/23 11:47, Donald Robson wrote:
> Hi Danilo,
>
> Apologies - I guess I should have submitted a patch to handle zero fences in your
> locking functions with the final patch series.
>
> On Sat, 2023-11-25 at 00:36 +0100, Danilo Krummrich wrote:
>> *** CAUTION: This email originates from a source not known to Imagination Technologies. Think before you click a link or open an attachment ***
>>
>> Make use of GPUVM's drm_exec helper functions preventing direct access
>> to GPUVM internal data structures, such as the external object list.
>>
>> This is especially important to ensure following the locking rules
>> around the GPUVM external object list.
>>
>> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
>> Signed-off-by: Danilo Krummrich <[email protected]>
>> ---
>> drivers/gpu/drm/imagination/pvr_vm.c | 16 +++++-----------
>> 1 file changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
>> index e0d74d9a6190..3f7888f5cc53 100644
>> --- a/drivers/gpu/drm/imagination/pvr_vm.c
>> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
>> @@ -337,27 +337,21 @@ static int
>> pvr_vm_bind_op_lock_resvs(struct drm_exec *exec, struct pvr_vm_bind_op *bind_op)
>> {
>> drm_exec_until_all_locked(exec) {
>> - struct drm_gem_object *r_obj = &bind_op->vm_ctx->dummy_gem;
>> struct drm_gpuvm *gpuvm = &bind_op->vm_ctx->gpuvm_mgr;
>> struct pvr_gem_object *pvr_obj = bind_op->pvr_obj;
>> - struct drm_gpuvm_bo *gpuvm_bo;
>>
>> /* Acquire lock on the vm_context's reserve object. */
>> - int err = drm_exec_lock_obj(exec, r_obj);
>> + int err = drm_gpuvm_prepare_vm(gpuvm, exec, 0);
>>
>> drm_exec_retry_on_contention(exec);
>> if (err)
>> return err;
>>
>> /* Acquire lock on all BOs in the context. */
>> - list_for_each_entry(gpuvm_bo, &gpuvm->extobj.list,
>> - list.entry.extobj) {
>> - err = drm_exec_lock_obj(exec, gpuvm_bo->obj);
>> -
>> - drm_exec_retry_on_contention(exec);
>> - if (err)
>> - return err;
>> - }
>> + err = drm_gpuvm_prepare_objects(gpuvm, exec, 0);
>> + drm_exec_retry_on_contention(exec);
>> + if (err)
>> + return err;
>
> Before I discovered the problem when not reserving fences, I was trying to use
> drm_gpuvm_exec_lock() with vm_exec->extra.fn() for the part below. Is there
> a reason not to do that now?
No, that works - gonna change that.
- Danilo
>
> Many thanks,
> Donald
>
>>
>> /* Unmap operations don't have an object to lock. */
>> if (!pvr_obj)