Re: [RFC PATCH] drm/scheduler: Add drm_sched_job_cleanup

["Koenig, Christian" <Christian.Koenig@amd.com>]

Am 24.10.18 um 16:55 schrieb Jordan Crouse:
> On Wed, Oct 24, 2018 at 07:10:23PM +0530, Sharat Masetty wrote:
>> Hi,
>>
>> Can you please review this and share your thoughts on this approach. We
>> primarily need a clean way to release the finished fence in case the job
>> was not pushed to the queue. So, this patch adds the new API to clean up
>> the resources allocated in sched_job_init()
>>
>> Additionally I also move the fence_put(finished_fence) from the
>> scheduler to the drivers handler of free_job(). The drivers get to use
>> this new API. This is done so that the layer creating the sched object is
>> the one freeing up the resources as well.
>>
>> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c    |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c   |  2 ++
>>   drivers/gpu/drm/etnaviv/etnaviv_sched.c   |  2 ++
>>   drivers/gpu/drm/msm/msm_sched.c           |  8 +++-----
> Just a nit - msm_sched.c isn't a thing yet. Can you re-generate a patch against
> drm-next so that if folks like this they can get it merged sooner?

Not a bad idea, but first of all as Jordan commented please rebase this 
on drm-next maybe even Alex amd-staging-drm-next branch.

Additional to that you missed one more occurrence where this needs to be 
applied in amdgpu_cs_submit().

Regards,
Christian.

>
> Jordan
>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 11 +++++++++--
>>   drivers/gpu/drm/v3d/v3d_sched.c           |  2 ++
>>   include/drm/gpu_scheduler.h               |  1 +
>>   7 files changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 9c85a90..9a83152 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -1198,7 +1198,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>>   	r = amdgpu_ctx_add_fence(p->ctx, ring, p->fence, &seq);
>>   	if (r) {
>>   		dma_fence_put(p->fence);
>> -		dma_fence_put(&job->base.s_fence->finished);
>> +		drm_sched_job_cleanup(&job->base);
>>   		amdgpu_job_free(job);
>>   		amdgpu_mn_unlock(p->mn);
>>   		return r;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> index 2bd5676..5d252d4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> @@ -100,6 +100,8 @@ static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
>>   {
>>   	struct amdgpu_job *job = container_of(s_job, struct amdgpu_job, base);
>>   
>> +	drm_sched_job_cleanup(s_job);
>> +
>>   	amdgpu_ring_priority_put(job->ring, s_job->s_priority);
>>   	dma_fence_put(job->fence);
>>   	amdgpu_sync_free(&job->sync);
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> index 50d6b88..30398c5 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
>> @@ -127,6 +127,8 @@ static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
>>   {
>>   	struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job);
>>   
>> +	drm_sched_job_cleanup(sched_job);
>> +
>>   	etnaviv_submit_put(submit);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/msm/msm_sched.c b/drivers/gpu/drm/msm/msm_sched.c
>> index 70b7713..748acc6 100644
>> --- a/drivers/gpu/drm/msm/msm_sched.c
>> +++ b/drivers/gpu/drm/msm/msm_sched.c
>> @@ -235,6 +235,8 @@ static void msm_sched_free_job(struct drm_sched_job *sched_job)
>>   	struct msm_gpu *gpu = submit->gpu;
>>   	int i;
>>   
>> +	drm_sched_job_cleanup(sched_job);
>> +
>>   	mutex_lock(&gpu->dev->struct_mutex);
>>   
>>   	for (i = 0; i < submit->nr_bos; i++) {
>> @@ -300,11 +302,7 @@ int msm_sched_job_init(struct drm_sched_job *sched_job)
>>   	mutex_unlock(&ring->fence_idr_lock);
>>   
>>   	if (submit->out_fence_id < 0) {
>> -		/*
>> -		 * TODO: The scheduler's finished fence leaks here since the
>> -		 * job will not be pushed to the queue. Need to update scheduler
>> -		 * to fix this cleanly(?)
>> -		 */
>> +		drm_sched_job_cleanup(sched_job);
>>   		dma_fence_put(submit->out_fence);
>>   		submit->out_fence = NULL;
>>   		return -ENOMEM;
>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> index f5534ff..bbf9315 100644
>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> @@ -214,7 +214,6 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
>>   						 finish_cb);
>>   	drm_sched_fence_finished(job->s_fence);
>>   	WARN_ON(job->s_fence->parent);
>> -	dma_fence_put(&job->s_fence->finished);
>>   	job->sched->ops->free_job(job);
>>   }
>>   
>> @@ -480,7 +479,6 @@ static void drm_sched_job_finish(struct work_struct *work)
>>   			drm_sched_queue_delayed_work(next);
>>   	}
>>   	spin_unlock(&sched->job_list_lock);
>> -	dma_fence_put(&s_job->s_fence->finished);
>>   	sched->ops->free_job(s_job);
>>   }
>>   
>> @@ -636,6 +634,15 @@ int drm_sched_job_init(struct drm_sched_job *job,
>>   EXPORT_SYMBOL(drm_sched_job_init);
>>   
>>   /**
>> + * Cleanup sched_job resources
>> + */
>> +void drm_sched_job_cleanup(struct drm_sched_job *job)
>> +{
>> +	dma_fence_put(&job->s_fence->finished);
>> +}
>> +EXPORT_SYMBOL(drm_sched_job_cleanup);
>> +
>> +/**
>>    * Return ture if we can push more jobs to the hw.
>>    */
>>   static bool drm_sched_ready(struct drm_gpu_scheduler *sched)
>> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
>> index b07bece..4abd73c 100644
>> --- a/drivers/gpu/drm/v3d/v3d_sched.c
>> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
>> @@ -35,6 +35,8 @@
>>   {
>>   	struct v3d_job *job = to_v3d_job(sched_job);
>>   
>> +	drm_sched_job_cleanup(sched_job);
>> +
>>   	v3d_exec_put(job->exec);
>>   }
>>   
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 5c59c38..2ec3ddf 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -213,6 +213,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
>>   		       struct drm_gpu_scheduler *sched,
>>   		       struct drm_sched_entity *entity,
>>   		       void *owner);
>> +void drm_sched_job_cleanup(struct drm_sched_job *job);
>>   void drm_sched_hw_job_reset(struct drm_gpu_scheduler *sched,
>>   			    struct drm_sched_job *job);
>>   void drm_sched_job_recovery(struct drm_gpu_scheduler *sched);
>> -- 
>> 1.9.1
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel