| Message ID | 20201103081347.1000139-1-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/panfrost: Move the GPU reset bits outside the timeout handler | expand |
On Tue, Nov 03, 2020 at 09:13:47AM +0100, Boris Brezillon wrote: > We've fixed many races in panfrost_job_timedout() but some remain. > Instead of trying to fix it again, let's simplify the logic and move > the reset bits to a separate work scheduled when one of the queue > reports a timeout. > > v3: > - Replace the atomic_cmpxchg() by an atomic_xchg() (Robin Murphy) > - Add Steven's R-b > > v2: > - Use atomic_cmpxchg() to conditionally schedule the reset work (Steven Price) > > Fixes: 1a11a88cfd9a ("drm/panfrost: Fix job timeout handling") > Cc: <stable@vger.kernel.org> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > Reviewed-by: Steven Price <steven.price@arm.com> Sprinkling the dma_fence annotations over this would be really nice ... -Daniel > --- > drivers/gpu/drm/panfrost/panfrost_device.c | 1 - > drivers/gpu/drm/panfrost/panfrost_device.h | 6 +- > drivers/gpu/drm/panfrost/panfrost_job.c | 127 ++++++++++++--------- > 3 files changed, 79 insertions(+), 55 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c > index ea8d31863c50..a83b2ff5837a 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c > @@ -200,7 +200,6 @@ int panfrost_device_init(struct panfrost_device *pfdev) > struct resource *res; > > mutex_init(&pfdev->sched_lock); > - mutex_init(&pfdev->reset_lock); > INIT_LIST_HEAD(&pfdev->scheduled_jobs); > INIT_LIST_HEAD(&pfdev->as_lru_list); > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h > index 140e004a3790..597cf1459b0a 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.h > +++ b/drivers/gpu/drm/panfrost/panfrost_device.h > @@ -106,7 +106,11 @@ struct panfrost_device { > struct panfrost_perfcnt *perfcnt; > > struct mutex sched_lock; > - struct mutex reset_lock; > + > + struct { > + struct work_struct work; > + atomic_t pending; > + } reset; > > struct mutex shrinker_lock; > struct list_head shrinker_list; > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c > index 4902bc6624c8..9691d6248f6d 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > @@ -20,6 +20,8 @@ > #include "panfrost_gpu.h" > #include "panfrost_mmu.h" > > +#define JOB_TIMEOUT_MS 500 > + > #define job_write(dev, reg, data) writel(data, dev->iomem + (reg)) > #define job_read(dev, reg) readl(dev->iomem + (reg)) > > @@ -382,19 +384,37 @@ static bool panfrost_scheduler_stop(struct panfrost_queue_state *queue, > drm_sched_increase_karma(bad); > queue->stopped = true; > stopped = true; > + > + /* > + * Set the timeout to max so the timer doesn't get started > + * when we return from the timeout handler (restored in > + * panfrost_scheduler_start()). > + */ > + queue->sched.timeout = MAX_SCHEDULE_TIMEOUT; > } > mutex_unlock(&queue->lock); > > return stopped; > } > > +static void panfrost_scheduler_start(struct panfrost_queue_state *queue) > +{ > + if (WARN_ON(!queue->stopped)) > + return; > + > + mutex_lock(&queue->lock); > + /* Restore the original timeout before starting the scheduler. */ > + queue->sched.timeout = msecs_to_jiffies(JOB_TIMEOUT_MS); > + drm_sched_start(&queue->sched, true); > + queue->stopped = false; > + mutex_unlock(&queue->lock); > +} > + > static void panfrost_job_timedout(struct drm_sched_job *sched_job) > { > struct panfrost_job *job = to_panfrost_job(sched_job); > struct panfrost_device *pfdev = job->pfdev; > int js = panfrost_job_get_slot(job); > - unsigned long flags; > - int i; > > /* > * If the GPU managed to complete this jobs fence, the timeout is > @@ -415,56 +435,9 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job) > if (!panfrost_scheduler_stop(&pfdev->js->queue[js], sched_job)) > return; > > - 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; > - > - /* > - * If the queue is still active, make sure we wait for any > - * pending timeouts. > - */ > - if (!pfdev->js->queue[i].stopped) > - cancel_delayed_work_sync(&sched->work_tdr); > - > - /* > - * If the scheduler was not already stopped, there's a tiny > - * chance a timeout has expired just before we stopped it, and > - * drm_sched_stop() does not flush pending works. Let's flush > - * them now so the timeout handler doesn't get called in the > - * middle of a reset. > - */ > - if (panfrost_scheduler_stop(&pfdev->js->queue[i], NULL)) > - cancel_delayed_work_sync(&sched->work_tdr); > - > - /* > - * Now that we cancelled the pending timeouts, we can safely > - * reset the stopped state. > - */ > - pfdev->js->queue[i].stopped = false; > - } > - > - spin_lock_irqsave(&pfdev->js->job_lock, flags); > - for (i = 0; i < NUM_JOB_SLOTS; i++) { > - if (pfdev->jobs[i]) { > - pm_runtime_put_noidle(pfdev->dev); > - panfrost_devfreq_record_idle(&pfdev->pfdevfreq); > - 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); > - > - mutex_unlock(&pfdev->reset_lock); > - > - /* restart scheduler after GPU is usable again */ > - for (i = 0; i < NUM_JOB_SLOTS; i++) > - drm_sched_start(&pfdev->js->queue[i].sched, true); > + /* Schedule a reset if there's no reset in progress. */ > + if (!atomic_xchg(&pfdev->reset.pending, 1)) > + schedule_work(&pfdev->reset.work); > } > > static const struct drm_sched_backend_ops panfrost_sched_ops = { > @@ -531,11 +504,59 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data) > return IRQ_HANDLED; > } > > +static void panfrost_reset(struct work_struct *work) > +{ > + struct panfrost_device *pfdev = container_of(work, > + struct panfrost_device, > + reset.work); > + unsigned long flags; > + unsigned int i; > + > + for (i = 0; i < NUM_JOB_SLOTS; i++) { > + /* > + * We want pending timeouts to be handled before we attempt > + * to stop the scheduler. If we don't do that and the timeout > + * handler is in flight, it might have removed the bad job > + * from the list, and we'll lose this job if the reset handler > + * enters the critical section in panfrost_scheduler_stop() > + * before the timeout handler. > + * > + * Timeout is set to max to make sure the timer is not > + * restarted after the cancellation. > + */ > + pfdev->js->queue[i].sched.timeout = MAX_SCHEDULE_TIMEOUT; > + cancel_delayed_work_sync(&pfdev->js->queue[i].sched.work_tdr); > + panfrost_scheduler_stop(&pfdev->js->queue[i], NULL); > + } > + > + /* All timers have been stopped, we can safely reset the pending state. */ > + atomic_set(&pfdev->reset.pending, 0); > + > + spin_lock_irqsave(&pfdev->js->job_lock, flags); > + for (i = 0; i < NUM_JOB_SLOTS; i++) { > + if (pfdev->jobs[i]) { > + pm_runtime_put_noidle(pfdev->dev); > + panfrost_devfreq_record_idle(&pfdev->pfdevfreq); > + 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); > + panfrost_scheduler_start(&pfdev->js->queue[i]); > + } > +} > + > int panfrost_job_init(struct panfrost_device *pfdev) > { > struct panfrost_job_slot *js; > int ret, j, irq; > > + INIT_WORK(&pfdev->reset.work, panfrost_reset); > + > pfdev->js = js = devm_kzalloc(pfdev->dev, sizeof(*js), GFP_KERNEL); > if (!js) > return -ENOMEM; > @@ -560,7 +581,7 @@ int panfrost_job_init(struct panfrost_device *pfdev) > > ret = drm_sched_init(&js->queue[j].sched, > &panfrost_sched_ops, > - 1, 0, msecs_to_jiffies(500), > + 1, 0, msecs_to_jiffies(JOB_TIMEOUT_MS), > "pan_js"); > if (ret) { > dev_err(pfdev->dev, "Failed to create scheduler: %d.", ret); > -- > 2.26.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, 3 Nov 2020 11:25:40 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Nov 03, 2020 at 09:13:47AM +0100, Boris Brezillon wrote: > > We've fixed many races in panfrost_job_timedout() but some remain. > > Instead of trying to fix it again, let's simplify the logic and move > > the reset bits to a separate work scheduled when one of the queue > > reports a timeout. > > > > v3: > > - Replace the atomic_cmpxchg() by an atomic_xchg() (Robin Murphy) > > - Add Steven's R-b > > > > v2: > > - Use atomic_cmpxchg() to conditionally schedule the reset work (Steven Price) > > > > Fixes: 1a11a88cfd9a ("drm/panfrost: Fix job timeout handling") > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > > Reviewed-by: Steven Price <steven.price@arm.com> > > Sprinkling the dma_fence annotations over this would be really nice ... You mean something like that? --->8--- From 4f90ee2972eaec0332833ff6f9ea078acbfa899a Mon Sep 17 00:00:00 2001 From: Boris Brezillon <boris.brezillon@collabora.com> Date: Tue, 3 Nov 2020 12:01:09 +0100 Subject: [PATCH] drm/panfrost: Annotate dma_fence signalling Annotate dma_fence signalling to help lockdep catch deadlocks. Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- drivers/gpu/drm/panfrost/panfrost_job.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 569a099dc10e..046cb3677332 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -482,7 +482,9 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data) if (status & JOB_INT_MASK_DONE(j)) { struct panfrost_job *job; + bool cookie; + cookie = dma_fence_begin_signalling(); spin_lock(&pfdev->js->job_lock); job = pfdev->jobs[j]; /* Only NULL if job timeout occurred */ @@ -496,6 +498,7 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data) pm_runtime_put_autosuspend(pfdev->dev); } spin_unlock(&pfdev->js->job_lock); + dma_fence_end_signalling(cookie); } status &= ~mask;
On Tue, Nov 03, 2020 at 12:03:26PM +0100, Boris Brezillon wrote: > On Tue, 3 Nov 2020 11:25:40 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Tue, Nov 03, 2020 at 09:13:47AM +0100, Boris Brezillon wrote: > > > We've fixed many races in panfrost_job_timedout() but some remain. > > > Instead of trying to fix it again, let's simplify the logic and move > > > the reset bits to a separate work scheduled when one of the queue > > > reports a timeout. > > > > > > v3: > > > - Replace the atomic_cmpxchg() by an atomic_xchg() (Robin Murphy) > > > - Add Steven's R-b > > > > > > v2: > > > - Use atomic_cmpxchg() to conditionally schedule the reset work (Steven Price) > > > > > > Fixes: 1a11a88cfd9a ("drm/panfrost: Fix job timeout handling") > > > Cc: <stable@vger.kernel.org> > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > > > Reviewed-by: Steven Price <steven.price@arm.com> > > > > Sprinkling the dma_fence annotations over this would be really nice ... > > You mean something like that? That's just the irq annotations, i.e. the one that's already guaranteed by the irq vs. locks checks. So this does nothing. What I mean is annotating your new reset work (it's part of the critical path to complete batches, since it's holding up other batches that are stuck in the scheduler still), and the drm/scheduler annotations I've floated a while ago. The drm/scheduler annotations are stuck somewhat for lack of feedback from any of the driver teams using it though :-/ The thing is pulling something out into a worker of it's own generally doesn't fix any deadlocks, it just hides them from lockdep. So it would be good to make sure lockdep can see through your maze again. -Daniel > > --->8--- > From 4f90ee2972eaec0332833ff6f9ea078acbfa899a Mon Sep 17 00:00:00 2001 > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Tue, 3 Nov 2020 12:01:09 +0100 > Subject: [PATCH] drm/panfrost: Annotate dma_fence signalling > > Annotate dma_fence signalling to help lockdep catch deadlocks. > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > drivers/gpu/drm/panfrost/panfrost_job.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c > index 569a099dc10e..046cb3677332 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > @@ -482,7 +482,9 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data) > > if (status & JOB_INT_MASK_DONE(j)) { > struct panfrost_job *job; > + bool cookie; > > + cookie = dma_fence_begin_signalling(); > spin_lock(&pfdev->js->job_lock); > job = pfdev->jobs[j]; > /* Only NULL if job timeout occurred */ > @@ -496,6 +498,7 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data) > pm_runtime_put_autosuspend(pfdev->dev); > } > spin_unlock(&pfdev->js->job_lock); > + dma_fence_end_signalling(cookie); > } > > status &= ~mask;
On Tue, 3 Nov 2020 12:08:47 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Nov 03, 2020 at 12:03:26PM +0100, Boris Brezillon wrote: > > On Tue, 3 Nov 2020 11:25:40 +0100 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Tue, Nov 03, 2020 at 09:13:47AM +0100, Boris Brezillon wrote: > > > > We've fixed many races in panfrost_job_timedout() but some remain. > > > > Instead of trying to fix it again, let's simplify the logic and move > > > > the reset bits to a separate work scheduled when one of the queue > > > > reports a timeout. > > > > > > > > v3: > > > > - Replace the atomic_cmpxchg() by an atomic_xchg() (Robin Murphy) > > > > - Add Steven's R-b > > > > > > > > v2: > > > > - Use atomic_cmpxchg() to conditionally schedule the reset work (Steven Price) > > > > > > > > Fixes: 1a11a88cfd9a ("drm/panfrost: Fix job timeout handling") > > > > Cc: <stable@vger.kernel.org> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > > > > Reviewed-by: Steven Price <steven.price@arm.com> > > > > > > Sprinkling the dma_fence annotations over this would be really nice ... > > > > You mean something like that? > > That's just the irq annotations, i.e. the one that's already guaranteed by > the irq vs. locks checks. So this does nothing. > > What I mean is annotating your new reset work (it's part of the critical > path to complete batches, since it's holding up other batches that are > stuck in the scheduler still), and the drm/scheduler annotations I've > floated a while ago. The drm/scheduler annotations are stuck somewhat for > lack of feedback from any of the driver teams using it though :-/ > > The thing is pulling something out into a worker of it's own generally > doesn't fix any deadlocks, it just hides them from lockdep. Hm, except that's not exactly a deadlock we were trying to fix here (as in, not a situation where 2 threads try to acquire locks in different orders), just a situation where the scheduler stops dequeuing jobs because it ends up in an inconsistent state (which is caused by a bad/lack-of synchronization between timeout handlers). The problem here is that we have 3 schedulers (one per HW queue) but when a timeout occurs on one of them, we need to reset them all, thus requiring some synchronization between the different timeout works. Moving the reset logic to a separate work simplifies the synchronization. > So it would be > good to make sure lockdep can see through your maze again. Okay, but it's not clear to me which part of the panfrost_reset() function should be annotated. I mean, I probably call functions that can signal fences, but I don't call dma_signal_fence() directly. Are callers of the dma_sched_xxx() helpers expected to place such annotations?
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c index ea8d31863c50..a83b2ff5837a 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.c +++ b/drivers/gpu/drm/panfrost/panfrost_device.c @@ -200,7 +200,6 @@ int panfrost_device_init(struct panfrost_device *pfdev) struct resource *res; mutex_init(&pfdev->sched_lock); - mutex_init(&pfdev->reset_lock); INIT_LIST_HEAD(&pfdev->scheduled_jobs); INIT_LIST_HEAD(&pfdev->as_lru_list); diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h index 140e004a3790..597cf1459b0a 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.h +++ b/drivers/gpu/drm/panfrost/panfrost_device.h @@ -106,7 +106,11 @@ struct panfrost_device { struct panfrost_perfcnt *perfcnt; struct mutex sched_lock; - struct mutex reset_lock; + + struct { + struct work_struct work; + atomic_t pending; + } reset; struct mutex shrinker_lock; struct list_head shrinker_list; diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 4902bc6624c8..9691d6248f6d 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -20,6 +20,8 @@ #include "panfrost_gpu.h" #include "panfrost_mmu.h" +#define JOB_TIMEOUT_MS 500 + #define job_write(dev, reg, data) writel(data, dev->iomem + (reg)) #define job_read(dev, reg) readl(dev->iomem + (reg)) @@ -382,19 +384,37 @@ static bool panfrost_scheduler_stop(struct panfrost_queue_state *queue, drm_sched_increase_karma(bad); queue->stopped = true; stopped = true; + + /* + * Set the timeout to max so the timer doesn't get started + * when we return from the timeout handler (restored in + * panfrost_scheduler_start()). + */ + queue->sched.timeout = MAX_SCHEDULE_TIMEOUT; } mutex_unlock(&queue->lock); return stopped; } +static void panfrost_scheduler_start(struct panfrost_queue_state *queue) +{ + if (WARN_ON(!queue->stopped)) + return; + + mutex_lock(&queue->lock); + /* Restore the original timeout before starting the scheduler. */ + queue->sched.timeout = msecs_to_jiffies(JOB_TIMEOUT_MS); + drm_sched_start(&queue->sched, true); + queue->stopped = false; + mutex_unlock(&queue->lock); +} + static void panfrost_job_timedout(struct drm_sched_job *sched_job) { struct panfrost_job *job = to_panfrost_job(sched_job); struct panfrost_device *pfdev = job->pfdev; int js = panfrost_job_get_slot(job); - unsigned long flags; - int i; /* * If the GPU managed to complete this jobs fence, the timeout is @@ -415,56 +435,9 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job) if (!panfrost_scheduler_stop(&pfdev->js->queue[js], sched_job)) return; - 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; - - /* - * If the queue is still active, make sure we wait for any - * pending timeouts. - */ - if (!pfdev->js->queue[i].stopped) - cancel_delayed_work_sync(&sched->work_tdr); - - /* - * If the scheduler was not already stopped, there's a tiny - * chance a timeout has expired just before we stopped it, and - * drm_sched_stop() does not flush pending works. Let's flush - * them now so the timeout handler doesn't get called in the - * middle of a reset. - */ - if (panfrost_scheduler_stop(&pfdev->js->queue[i], NULL)) - cancel_delayed_work_sync(&sched->work_tdr); - - /* - * Now that we cancelled the pending timeouts, we can safely - * reset the stopped state. - */ - pfdev->js->queue[i].stopped = false; - } - - spin_lock_irqsave(&pfdev->js->job_lock, flags); - for (i = 0; i < NUM_JOB_SLOTS; i++) { - if (pfdev->jobs[i]) { - pm_runtime_put_noidle(pfdev->dev); - panfrost_devfreq_record_idle(&pfdev->pfdevfreq); - 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); - - mutex_unlock(&pfdev->reset_lock); - - /* restart scheduler after GPU is usable again */ - for (i = 0; i < NUM_JOB_SLOTS; i++) - drm_sched_start(&pfdev->js->queue[i].sched, true); + /* Schedule a reset if there's no reset in progress. */ + if (!atomic_xchg(&pfdev->reset.pending, 1)) + schedule_work(&pfdev->reset.work); } static const struct drm_sched_backend_ops panfrost_sched_ops = { @@ -531,11 +504,59 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data) return IRQ_HANDLED; } +static void panfrost_reset(struct work_struct *work) +{ + struct panfrost_device *pfdev = container_of(work, + struct panfrost_device, + reset.work); + unsigned long flags; + unsigned int i; + + for (i = 0; i < NUM_JOB_SLOTS; i++) { + /* + * We want pending timeouts to be handled before we attempt + * to stop the scheduler. If we don't do that and the timeout + * handler is in flight, it might have removed the bad job + * from the list, and we'll lose this job if the reset handler + * enters the critical section in panfrost_scheduler_stop() + * before the timeout handler. + * + * Timeout is set to max to make sure the timer is not + * restarted after the cancellation. + */ + pfdev->js->queue[i].sched.timeout = MAX_SCHEDULE_TIMEOUT; + cancel_delayed_work_sync(&pfdev->js->queue[i].sched.work_tdr); + panfrost_scheduler_stop(&pfdev->js->queue[i], NULL); + } + + /* All timers have been stopped, we can safely reset the pending state. */ + atomic_set(&pfdev->reset.pending, 0); + + spin_lock_irqsave(&pfdev->js->job_lock, flags); + for (i = 0; i < NUM_JOB_SLOTS; i++) { + if (pfdev->jobs[i]) { + pm_runtime_put_noidle(pfdev->dev); + panfrost_devfreq_record_idle(&pfdev->pfdevfreq); + 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); + panfrost_scheduler_start(&pfdev->js->queue[i]); + } +} + int panfrost_job_init(struct panfrost_device *pfdev) { struct panfrost_job_slot *js; int ret, j, irq; + INIT_WORK(&pfdev->reset.work, panfrost_reset); + pfdev->js = js = devm_kzalloc(pfdev->dev, sizeof(*js), GFP_KERNEL); if (!js) return -ENOMEM; @@ -560,7 +581,7 @@ int panfrost_job_init(struct panfrost_device *pfdev) ret = drm_sched_init(&js->queue[j].sched, &panfrost_sched_ops, - 1, 0, msecs_to_jiffies(500), + 1, 0, msecs_to_jiffies(JOB_TIMEOUT_MS), "pan_js"); if (ret) { dev_err(pfdev->dev, "Failed to create scheduler: %d.", ret);
We've fixed many races in panfrost_job_timedout() but some remain. Instead of trying to fix it again, let's simplify the logic and move the reset bits to a separate work scheduled when one of the queue reports a timeout. v3: - Replace the atomic_cmpxchg() by an atomic_xchg() (Robin Murphy) - Add Steven's R-b v2: - Use atomic_cmpxchg() to conditionally schedule the reset work (Steven Price) Fixes: 1a11a88cfd9a ("drm/panfrost: Fix job timeout handling") Cc: <stable@vger.kernel.org> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> Reviewed-by: Steven Price <steven.price@arm.com> --- drivers/gpu/drm/panfrost/panfrost_device.c | 1 - drivers/gpu/drm/panfrost/panfrost_device.h | 6 +- drivers/gpu/drm/panfrost/panfrost_job.c | 127 ++++++++++++--------- 3 files changed, 79 insertions(+), 55 deletions(-)