On Thu, 1 Oct 2020 15:49:39 +0100
Steven Price <steven.price@xxxxxxx> wrote:
> On 01/10/2020 15:01, Boris Brezillon wrote:
> > If more than two or more jobs end up timeout-ing concurrently, only one
> > of them (the one attached to the scheduler acquiring the lock) is fully
> > handled. The other one remains in a dangling state where it's no longer
> > part of the scheduling queue, but still blocks something in scheduler
> > thus leading to repetitive timeouts when new jobs are queued.
> >
> > Let's make sure all bad jobs are properly handled by the thread acquiring
> > the lock.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/panfrost/panfrost_job.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> > index 30e7b7196dab..e87edca51d84 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > @@ -25,7 +25,7 @@
> >
> > struct panfrost_queue_state {
> > struct drm_gpu_scheduler sched;
> > -
> > + struct drm_sched_job *bad;
> > u64 fence_context;
> > u64 emit_seqno;
> > };
> > @@ -392,19 +392,29 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
> > job_read(pfdev, JS_TAIL_LO(js)),
> > sched_job);
> >
> > + /*
> > + * Collect the bad job here so it can be processed by the thread
> > + * acquiring the reset lock.
> > + */
> > + pfdev->js->queue[js].bad = sched_job;
> > +
> > if (!mutex_trylock(&pfdev->reset_lock))
> > return;
> >
> > for (i = 0; i < NUM_JOB_SLOTS; i++) {
> > struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
> >
> > - drm_sched_stop(sched, sched_job);
> > if (js != i)
> > /* Ensure any timeouts on other slots have finished */
> > cancel_delayed_work_sync(&sched->work_tdr);
> > - }
> >
> > - drm_sched_increase_karma(sched_job);
> > + drm_sched_stop(sched, pfdev->js->queue[i].bad);
>
> So I can see that the call to drm_sched_stop() needs to move below the
> cancel_delayed_work_sync() to ensure that the update to queue->bad is
> synchronised. What I'm not so sure about is whether it's possible for
> the scheduler to make progress between the 'cancel' and the 'stop' -
> there is a reason I wrote it the other way round...
Ok, I think see what you mean now. So, there's indeed a race between
the cancel_delayed_work_sync() and drm_sched_stop() calls, and a
timeout might go undetected because of that. This being said, the only
problem I see is that we would not increase karma on that job. In any
case the job will be re-queued, and unless we keep having timeouts on
the other queues it should be detected at some point. I can also try to
retrieve the deadline before canceling the delayed work and check it
before stopping the scheduler, but I'm not sure it's worth it.
>
> The hole for things to go round is clearly much smaller with this
> change, but I'm not sure it's completely plugged. Am I missing something?
>
> > +
> > + if (pfdev->js->queue[i].bad)
> > + drm_sched_increase_karma(pfdev->js->queue[i].bad);
> > +
> > + pfdev->js->queue[i].bad = NULL;
> > + }
> >
> > spin_lock_irqsave(&pfdev->js->job_lock, flags);
> > for (i = 0; i < NUM_JOB_SLOTS; i++) {
> >
>
> While we're on potential holes... some more context:
>
> > if (pfdev->jobs[i]) {
> > pm_runtime_put_noidle(pfdev->dev);
> > panfrost_devfreq_record_idle(pfdev);
> > pfdev->jobs[i] = NULL;
> > }
> > }
> > spin_unlock_irqrestore(&pfdev->js->job_lock, flags);
> >
> > panfrost_device_reset(pfdev);
> >
> > for (i = 0; i < NUM_JOB_SLOTS; i++)
> > drm_sched_resubmit_jobs(&pfdev->js->queue[i].sched);
> >
> > /* restart scheduler after GPU is usable again */
> > for (i = 0; i < NUM_JOB_SLOTS; i++)
> > drm_sched_start(&pfdev->js->queue[i].sched, true);
> >
> > mutex_unlock(&pfdev->reset_lock);
>
> I'm wondering whether the mutex_unlock() should actually happen before
> the drm_sched_start() - in the (admittedly very unlikely) case where a
> timeout occurs before all the drm_sched_start() calls have completed
> it's possible for the timeout to be completely missed because the mutex
> is still held.
>
> Thanks,
>
> Steve
