[PATCH 4/5] drm/amd/sched: NULL out the s_fence field after run_job

[nicolai.haehnle@xxxxxxx (Nicolai Hähnle)]

On 28.09.2017 20:39, Andres Rodriguez wrote:
> 
> 
> On 2017-09-28 10:55 AM, Nicolai Hähnle wrote:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> amd_sched_process_job drops the fence reference, so NULL out the s_fence
>> field before adding it as a callback to guard against accidentally using
>> s_fence after it may have be freed.
>>
>> Signed-off-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
>> Acked-by: Christian König <christian.koenig at amd.com>
>> ---
>> Â  drivers/gpu/drm/amd/scheduler/gpu_scheduler.c | 3 +++
>> Â  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c 
>> b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>> index e793312e351c..54eb77cffd9b 100644
>> --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>> +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>> @@ -604,20 +604,23 @@ static int amd_sched_main(void *param)
>> Â Â Â Â Â Â Â Â Â  if (!sched_job)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â  continue;
>> Â Â Â Â Â Â Â Â Â  s_fence = sched_job->s_fence;
>> Â Â Â Â Â Â Â Â Â  atomic_inc(&sched->hw_rq_count);
>> Â Â Â Â Â Â Â Â Â  amd_sched_job_begin(sched_job);
>> Â Â Â Â Â Â Â Â Â  fence = sched->ops->run_job(sched_job);
>> Â Â Â Â Â Â Â Â Â  amd_sched_fence_scheduled(s_fence);
>> +
>> +Â Â Â Â Â Â Â  sched_job->s_fence = NULL;
> 
> Minor optional nitpick here. Could this be moved somewhere closer to 
> where the fence reference is actually dropped? Alternatively, could a 
> comment be added to specify which function call results in the reference 
> ownership transfer?

Sure, I can add a comment. (It's amd_sched_process_job, which is called 
directly or indirectly in all the branches of the following if-statement.)


> Whether a change is made or not, this series is
> Reviewed-by: Andres Rodriguez <andresx7 at gmail.com>

Thanks.


> Currently running piglit to check if this fixes the occasional soft 
> hangs I was getting where all tests complete except one.

You may be running into this Mesa issue:

https://patchwork.freedesktop.org/patch/179535/

Cheers,
Nicolai


> 
>> +
>> Â Â Â Â Â Â Â Â Â  if (fence) {
>> Â Â Â Â Â Â Â Â Â Â Â Â Â  s_fence->parent = dma_fence_get(fence);
>> Â Â Â Â Â Â Â Â Â Â Â Â Â  r = dma_fence_add_callback(fence, &s_fence->cb,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  amd_sched_process_job);
>> Â Â Â Â Â Â Â Â Â Â Â Â Â  if (r == -ENOENT)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  amd_sched_process_job(fence, &s_fence->cb);
>> Â Â Â Â Â Â Â Â Â Â Â Â Â  else if (r)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  DRM_ERROR("fence add callback failed (%d)\n",
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  r);
>> Â Â Â Â Â Â Â Â Â Â Â Â Â  dma_fence_put(fence);
>>