On 9/21/26 11:06 AM, Christian König wrote:
On 9/21/26 16:50, Shahyan Soltani wrote:
On 9/21/26 10:40 AM, Shahyan Soltani wrote:
On 9/18/26 3:57 AM, Christian König wrote:
On 9/17/26 21:58, Shahyan Soltani wrote:
From: Pierre-Eric Pelloux-Prayer <[email protected]>
For now use the same logic as the existing one, but it's not 100%
correct since:
1) most operations don't need the GPUs
2) some operations that need it are actually deferred
Signed-off-by: Pierre-Eric Pelloux-Prayer <[email protected]>
Signed-off-by: Shahyan Soltani <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 4e4814caf61a..6edd03647c2d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -978,6 +978,14 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
if (r)
goto error;
+ /* FIXME: only PRT operations use the GPU to update mapping, but
+ * some operations are deferred to fences callbacks so it's not
+ * trivial to fix.
+ */
That comment is not correct, just drop it.
+ r = pm_runtime_resume_and_get(adev->dev);
+ if (r < 0)
+ return r;
+
switch (args->operation) {
case AMDGPU_VA_OP_MAP:
r = amdgpu_vm_bo_map(adev, bo_va, args->va_address,
@@ -1031,6 +1039,8 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void
*data,
}
+ pm_runtime_put_autosuspend(adev->dev);
That error handling is not correct, after the switch there are a bunch of
returns and gotos for cleanup.
I strongly suggest to use the guard approach here as well.
Regards,
Christian.
Thanks for the feedback, you caught something I completely missed. However I
have a few concerns
on what approaches I could take.
Attempting to use a guard in this situation would require the guard to be moved
to the top of the function,
resuming the system much early in the stack.
As far as I understand this is due to the guard not being able to succeed any gotos as
the "lock" variable
in the guard must be initialized before any subsequent goto.
For example:
goto error;
PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);
Oh, that is a very good point.
Is invalid as "lock" is initialized after the goto.
Trying to initialize lock beforehand doesn't work as it would count as a
redefinition of lock instead.
Would that still be preferred over creating a new label and trying something
like this instead?
Yes, as far as I can see the PM reference just needs to be moved before the
first goto, right?
When I now check the code, the first goto in function amdgpu_gem_va_ioctl() is
that here:
/* Add input syncobj fences (if any) for synchronization. */
r = amdgpu_gem_add_input_fence(filp,
args->input_fence_syncobj_handles,
args->num_syncobj_handles);
if (r)
goto error_put_gobj;
drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT |
DRM_EXEC_IGNORE_DUPLICATES, 0);
We can't grab the PM reference after drm_exec_init() anyway because that could
deadlock.
So that should still work as far as I can see, or do I miss anything?
Regards,
Christian.
Nope, you got everything. Just making sure if this approach was the one
that should be pursued.
I didn't notice the drm_exec in this case either.
Thanks for the help,
Shahyan
replace
+ r = pm_runtime_resume_and_get(adev->dev);
+ if (r < 0)
+ return r;
with
+ r = pm_runtime_resume_and_get(adev->dev);
+ if (r < 0)
+ goto newlabel;
}
...
+ newlabel:
+ pm_runtime_put_autosuspend(adev->dev);
error:
dma_fence_chain_free(timeline_chain);
if (timeline_syncobj)
drm_syncobj_put(timeline_syncobj);
drm_exec_fini(&exec);
error_put_gobj:
drm_gem_object_put(gobj);
return r;
}
Another approach I could try is wrap the entirePM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); and the
subsequent switch cases before the error handling into its own scope with {} However, I feel this method is a lot more
hacky then necessary. int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) { ... + { +
PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock); + + r = PM_RUNTIME_ACQUIRE_ERR(&lock); + if (r) +
goto error; switch (args->operation) { case AMDGPU_VA_OP_MAP: ... case AMDGPU_VA_OP_UNMAP: .. case
AMDGPU_VA_OP_CLEAR: ... case AMDGPU_VA_OP_REPLACE: ... default: ... } if (!r && !(args->flags &
AMDGPU_VM_DELAY_UPDATE) && (!adev->debug_vm || timeline_syncobj)) { ... } + } error: ... error_put_gobj:
... } If you have any ideas on what approach I should/could take it would be very much appreciated. Thanks, Shahyan
I'm not entirely sure what happened here...
But I meant to say wrapping the guard and the subsequent case handling into its
own scope.
For example:
int amdgpu_gem_va_ioctl() {
...
goto error;
+ {
+ PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev->dev, lock);
+ r = PM_RUNTIME_ACQUIRE_ERR(&lock);
+ if (r)
+ goto error;
...
+ }
error:
...
error_put_gobj:
...
}
Thanks,
Shahyan
+
error:
dma_fence_chain_free(timeline_chain);
if (timeline_syncobj)
