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@xxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > > Reviewed-by: Steven Price <steven.price@xxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
> ---
> 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;
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
