diff mbox series

[drm-misc-next,5/5] drm/imagination: vm: make use of GPUVM's drm_exec helper

Message ID 20231124233650.152653-6-dakr@redhat.com (mailing list archive)
State New, archived
Headers show
Series PowerVR VM fixes | expand

Commit Message

Danilo Krummrich Nov. 24, 2023, 11:36 p.m. UTC
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 <dakr@redhat.com>
---
 drivers/gpu/drm/imagination/pvr_vm.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

Comments

Donald Robson Nov. 28, 2023, 10:47 a.m. UTC | #1
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 <dakr@redhat.com>
> ---
>  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)
Danilo Krummrich Nov. 28, 2023, 7:02 p.m. UTC | #2
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 <dakr@redhat.com>
>> ---
>>   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)
diff mbox series

Patch

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)