[Freedreno] [PATCH 2/2] drm/scheduler: Add a start_timeout_notify() backend function

[Koenig, Christian]

Am 02.11.18 um 14:25 schrieb Sharat Masetty:
>
>
> On 11/2/2018 4:09 PM, Koenig, Christian wrote:
>> Am 02.11.18 um 11:31 schrieb Sharat Masetty:
>>> Add an optional backend function op which will let the scheduler 
>>> clients
>>> know when the timeout got scheduled on the scheduler instance. This 
>>> will
>>> help drivers with multiple schedulers(one per ring) measure time 
>>> spent on
>>> the ring accurately, eventually helping with better timeout detection.
>>>
>>> Signed-off-by: Sharat Masetty <smasetty at codeaurora.org>
>>
>> Well, NAK. drm_sched_start_timeout() is called whenever the timer needs
>> to run, but that doesn't mean that the timer is started (e.g. it can
>> already be running).
>>
>> So the callback would be called multiple times and not reflect the
>> actual job run time.
>>
>> Additional to that it can be racy, e.g. we can complete multiple jobs at
>> a time before the timer is started again.
>>
>> If you want to accurately count how much time you spend on each job/ring
>> you need to do this by measuring the time inside your driver instead.
>>
>> E.g. for amdgpu I would get the time first in amdgpu_job_run() and then
>> again in amdgpu_job_free_cb() and calculate the difference.
> Hi Christian,
>
> Thank you for the comments and apologies if this was confusing. All I 
> want to determine(more accurately) is that when the scheduler instance 
> timer of say 500 ms goes off, is if the ring(associated with the 
> scheduler instance) actually spent 500 ms on the hardware - and for 
> this I need to know in the driver space when the timer actually started.
>
> In msm hardware we have ring preemption support enabled and the kernel 
> driver triggers a preemption switch to a higher priority ring if there 
> is work available on that ring for the GPU to work on. So in the 
> presence of preemption it is possible that a lower priority ring did 
> not actually get to spend the full 500 ms and this is what I am trying 
> to catch with this callback.
>
> I am *not* trying to profile per job time consumption with this.
>
> > Well, NAK. drm_sched_start_timeout() is called whenever the timer needs
> > to run, but that doesn't mean that the timer is started (e.g. it can
> > already be running).
>
> Regarding the case where the timer may already be running - good 
> point, but it should be easy to address the scenario. I will check the 
> output
> of schedule_delayed_work() and only call the callback(new proposed) if 
> the timer was really scheduled.

Yeah, that should work.

>
> In summary, when this timedout_job() callback is called, it is assumed 
> that the job actually did time out from the POV of the scheduler, but 
> this will not hold true with preemption switching and that is what I 
> am trying to better address with this patch.

Mhm, so what you actually need is to suspend the timeout when the lower 
priority ring is preempted and resume it when it is started again? I 
wonder if that wouldn't be simpler.

We have support for ring preemption as well, but not implemented yet. So 
it would be nice to have something that works for everybody.

But on the other hand a callback to notify the driver that the timer 
started isn't so bad either.

Regards,
Christian.

>
> Sharat
>>
>> Regards,
>> Christian.
>>
>>> ---
>>> Here is an example of how I plan to use this new function callback.
>>>
>>> [1] https://patchwork.freedesktop.org/patch/254227/
>>>
>>>    drivers/gpu/drm/scheduler/sched_main.c | 7 ++++++-
>>>    include/drm/gpu_scheduler.h            | 6 ++++++
>>>    2 files changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index c993d10..afd461e 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -192,8 +192,13 @@ bool drm_sched_dependency_optimized(struct 
>>> dma_fence* fence,
>>>    static void drm_sched_start_timeout(struct drm_gpu_scheduler *sched)
>>>    {
>>>        if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>>> -        !list_empty(&sched->ring_mirror_list))
>>> +        !list_empty(&sched->ring_mirror_list)) {
>>> +
>>>            schedule_delayed_work(&sched->work_tdr, sched->timeout);
>>> +
>>> +        if (sched->ops->start_timeout_notify)
>>> +            sched->ops->start_timeout_notify(sched);
>>> +    }
>>>    }
>>>
>>>    /* job_finish is called after hw fence signaled
>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>> index d87b268..faf28b4 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -239,6 +239,12 @@ struct drm_sched_backend_ops {
>>>             * and it's time to clean it up.
>>>         */
>>>        void (*free_job)(struct drm_sched_job *sched_job);
>>> +
>>> +    /*
>>> +     * (Optional) Called to let the driver know that a timeout 
>>> detection
>>> +     * timer has been started.
>>> +     */
>>> +    void (*start_timeout_notify)(struct drm_gpu_scheduler *sched);
>>>    };
>>>
>>>    /**
>>> -- 
>>> 1.9.1
>>>
>>
>> _______________________________________________
>> Freedreno mailing list
>> Freedreno at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/freedreno
>>
>