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.
>>
>> 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)
