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);
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?
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
+
error:
dma_fence_chain_free(timeline_chain);
if (timeline_syncobj)
