Re: [PATCH v2 3/8] drm/amdgpu: rework runtime pm management for cs

["Christian König" <christian.koenig@amd.com>]

On 9/17/26 21:58, Shahyan Soltani wrote:
> From: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> 
> Creating a job will now wake up the GPU. The rpm ref will be dropped
> either in amdgpu_ib_schedule (because at this point the amdgpu_fence
> took a ref) or in amdgpu_job_free (for unsubmitted jobs).
> 
> Jobs submitted directly are a bit tricky because they might be fired
> from suspend/resume sequence and for now they're left unmodified.

Well that patch is still quite a mess.

I strongly suggest to just drop it completely and have the CS IOCTL have the necessary runtime_pm calls. See amdgpu_cs_ioctl().

Dropping the runtime_pm at the end of that is ok since autosuspend will wait for the submitted work to complete. We actually don't need to mess with the jobs nor the fences here.

Regards,
Christian.

> 
> 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_ib.c  | 15 +++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 42 +++++++++++++++++++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.h |  2 ++
>  3 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index 0f8b097001e3..3b55dd6ce8a8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -27,6 +27,7 @@
>   *          Christian König
>   */
>  #include <linux/seq_file.h>
> +#include <linux/pm_runtime.h>
>  #include <drm/amdgpu_drm.h>
>  
>  #include "amdgpu.h"
> @@ -297,8 +298,18 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, struct amdgpu_job *job,
>  
>  	amdgpu_fence_emit(ring, af, fence_flags);
>  	*f = &af->base;
> -	/* get a ref for the job */
> -	dma_fence_get(*f);
> +
> +	if (job) {
> +		/* get a ref for the job */
> +		dma_fence_get(*f);
> +		/* now that the fence has been sent to the hw, we can drop
> +		 * our pm ref.
> +		 */
> +		if (job->holds_pm_ref) {
> +			pm_runtime_put_autosuspend(adev->dev);
> +			job->holds_pm_ref = false;
> +		}
> +	}
>  
>  	if (ring->funcs->insert_end)
>  		ring->funcs->insert_end(ring);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 0056a93c400a..417f2619dfb0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -24,6 +24,7 @@
>  #include <linux/kthread.h>
>  #include <linux/wait.h>
>  #include <linux/sched.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <drm/drm_drv.h>
>  
> @@ -201,15 +202,24 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  		     unsigned int num_ibs, u64 drm_client_id,
>  		     gfp_t gfp_flags, struct amdgpu_job **job)
>  {
> +	bool take_pm_ref = entity && !adev->in_runpm;
>  	struct amdgpu_fence *af;
>  	int r;
>  
>  	if (num_ibs == 0)
>  		return -EINVAL;
>  
> +	if (take_pm_ref) {
> +		r = pm_runtime_resume_and_get(adev->dev);
> +		if (r < 0)
> +			return r;
> +	}
> +
>  	*job = kzalloc_flex(**job, ibs, num_ibs, gfp_flags);
> -	if (!*job)
> -		return -ENOMEM;
> +	if (!*job) {
> +		r = -ENOMEM;
> +		goto out_pm;
> +	}
>  
>  	af = kzalloc_obj(struct amdgpu_fence, gfp_flags);
>  	if (!af) {
> @@ -230,6 +240,7 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	amdgpu_sync_create(&(*job)->explicit_sync);
>  	(*job)->generation = amdgpu_vm_generation(adev, vm);
>  	(*job)->vm_pd_addr = AMDGPU_BO_INVALID_OFFSET;
> +	(*job)->holds_pm_ref = take_pm_ref;
>  
>  	if (!entity)
>  		return 0;
> @@ -245,6 +256,9 @@ int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  err_job:
>  	kfree(*job);
>  	*job = NULL;
> +out_pm:
> +	if (take_pm_ref)
> +		pm_runtime_put_autosuspend(adev->dev);
>  
>  	return r;
>  }
> @@ -270,6 +284,8 @@ int amdgpu_job_alloc_with_ib(struct amdgpu_device *adev,
>  		kfree((*job)->hw_vm_fence);
>  		kfree((*job)->hw_fence);
>  		kfree(*job);
> +		if ((*job)->holds_pm_ref)
> +			pm_runtime_put_autosuspend(adev->dev);
>  		*job = NULL;
>  	}
>  
> @@ -314,6 +330,17 @@ void amdgpu_job_free_resources(struct amdgpu_job *job)
>  static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
>  {
>  	struct amdgpu_job *job = to_amdgpu_job(s_job);
> +	struct amdgpu_ring *ring;
> +
> +	if (job->holds_pm_ref && job->base.sched) {
> +		ring = to_amdgpu_ring(job->base.sched);
> +		WARN_ON(!ring);
> +
> +		/* Should have been cleared on submit, but we have to drop
> +		 * it in case the job was cancelled.
> +		 */
> +		pm_runtime_put_autosuspend(ring->adev->dev);
> +	}
>  
>  	drm_sched_job_cleanup(s_job);
>  
> @@ -349,9 +376,20 @@ void amdgpu_job_set_gang_leader(struct amdgpu_job *job,
>  
>  void amdgpu_job_free(struct amdgpu_job *job)
>  {
> +	struct amdgpu_ring *ring;
> +
>  	if (job->base.entity)
>  		drm_sched_job_cleanup(&job->base);
>  
> +	if (job->holds_pm_ref && job->base.sched) {
> +		ring = to_amdgpu_ring(job->base.sched);
> +
> +		/* Should have been cleared on submit, but we have to drop
> +		 * it in case the job was cancelled.
> +		 */
> +		pm_runtime_put_autosuspend(ring->adev->dev);
> +	}
> +
>  	amdgpu_job_free_resources(job);
>  	amdgpu_sync_free(&job->explicit_sync);
>  	if (job->gang_submit != &job->base.s_fence->scheduled)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> index 7e03bd2ff639..e7ab7e38f6df 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> @@ -101,6 +101,8 @@ struct amdgpu_job {
>  	bool			enforce_isolation;
>  	bool			run_cleaner_shader;
>  
> +	bool			holds_pm_ref;
> +
>  	uint32_t		num_ibs;
>  	struct amdgpu_ib	ibs[];
>  };