[RFC 03/18] drm/sched: Remove one local variable

[Christian König]

Am 09.01.25 um 14:20 schrieb Tvrtko Ursulin:
>
> On 09/01/2025 12:49, Christian König wrote:
>> Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
>>> It is not helping readability nor it is required to keep the line 
>>> length
>>> in check.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>> Cc: Christian König <christian.koenig at amd.com>
>>> Cc: Danilo Krummrich <dakr at redhat.com>
>>> Cc: Matthew Brost <matthew.brost at intel.com>
>>> Cc: Philipp Stanner <pstanner at redhat.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_main.c | 5 +----
>>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index 1734c17aeea5..01e0d6e686d1 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -1175,7 +1175,6 @@ static void drm_sched_run_job_work(struct 
>>> work_struct *w)
>>>           container_of(w, struct drm_gpu_scheduler, work_run_job);
>>>       struct drm_sched_entity *entity;
>>>       struct dma_fence *fence;
>>> -    struct drm_sched_fence *s_fence;
>>>       struct drm_sched_job *sched_job;
>>>       int r;
>>> @@ -1194,15 +1193,13 @@ static void drm_sched_run_job_work(struct 
>>> work_struct *w)
>>>           return;
>>>       }
>>> -    s_fence = sched_job->s_fence;
>>> -
>>>       atomic_add(sched_job->credits, &sched->credit_count);
>>>       drm_sched_job_begin(sched_job);
>>>       trace_drm_run_job(sched_job, entity);
>>>       fence = sched->ops->run_job(sched_job);
>>>       complete_all(&entity->entity_idle);
>>> -    drm_sched_fence_scheduled(s_fence, fence);
>>> +    drm_sched_fence_scheduled(sched_job->s_fence, fence);
>>
>> Originally that was not for readability but for correctness.
>>
>> As soon as complete_all(&entity->entity_idle); was called the 
>> sched_job could have been released.
>
> And without a comment ouch.

That changed long long time ago and IIRC we did had a comment for that.

>
>> But we changed that so that the sched_job is released from a separate 
>> worker instead, so that is most likely not necessary any more.
>
> Very subtle. Especially given some drivers use unordered queue.

Hui? unordered queue? How should that work?

Job submission ordering is a mandatory requirement of the dma_fence.

>
> And for them sched_job is dereferenced a few more times in the block 
> below so not sure how it is safe.
>
> Move complete_all() to the end of it all?

Most likely good idea, yes.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>>       if (!IS_ERR_OR_NULL(fence)) {
>>>           /* Drop for original kref_init of the fence */
>>