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

[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);
>>