Re: [PATCH v2 4/8] drm/amdgpu: push down runtime pm handling in amdgpu_gem_create_ioctl

[Shahyan Soltani <shahyan.soltani@amd.com>]

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 <pierre-eric.pelloux-prayer@amd.com>
>>
>> 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 <pierre-eric.pelloux-prayer@amd.com>
>> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
>> ---
>>   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)