[Intel-gfx] [PATCH 2/3] drm/i915: Fix incorrect assert about pending power domain async-put work

[Imre Deak]

On Wed, Jun 02, 2021 at 02:35:28PM +0530, Anshuman Gupta wrote:
> On 2021-05-26 at 20:07:28 +0530, Imre Deak wrote:
> > It's possible that an already dequeued put_async_work() will release the
> > reference (*) that was put asynchronously after the dequeue happened.
> > This leaves an async-put work pending, without any reference to release.
> > A subsequent async-put may trigger the
> > 
> > drm_WARN_ON(!queue_delayed_work(&power_domains->async_put_work));
> > 
> > warn due to async_put_work() still pending. To avoid the warn, cancel
> > the pending async_put_work() when releasing the reference at (*) above.
>
> Not able to visulize the race here between __intel_display_power_put_async
> and intel_display_power_put_async_work() considering both are protected by
> power_domains->lock.
>
> queue_delayed_work_on() documentation says following about return value.
> "Return: %false if @work was already on a queue, %true otherwise."
> AFAIU from the doc, queue_delayed_work will return false only when
> work was in queue after dequeued put_async_work() it should return true ?

Yes, and so the WARN is triggered when __intel_display_power_put_async()
tries to queue a work when one is already pending in the queue:

1. get(A)
2. put_async(A)  -> queues put_async_work()
3. put_async_work() dequeued, blocking on power_domains->lock
4. get(A) -> grab_async_put_ref(), reacquires the ref that was put in 2.
5. put_async(A) -> queues put_async_work()
6. put_async_work() dequeued in 3. unblocks, releases the ref that was put in
   5., put_async_work() queued in 5. still pending in the queue, without
   any reference to release.
7. get(A)
8. put_async(A) -> tries to queue put_async_work(), but it's already
   pending -> WARN triggers.

The patch avoids the WARN in 8 by cancelling the queued work at 6.

> Thanks,
> Anshuman Gupta.
> > 
> > Fixes: https://gitlab.freedesktop.org/drm/intel/-/issues/3421
> > Fixes: https://gitlab.freedesktop.org/drm/intel/-/issues/2289
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display_power.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 2f7d1664c4738..a95bbf23e6b7c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -2263,6 +2263,12 @@ intel_display_power_put_async_work(struct work_struct *work)
> >  			fetch_and_zero(&power_domains->async_put_domains[1]);
> >  		queue_async_put_domains_work(power_domains,
> >  					     fetch_and_zero(&new_work_wakeref));
> > +	} else {
> > +		/*
> > +		 * Cancel the work that got queued after this one got dequeued,
> > +		 * since here we released the corresponding async-put reference.
> > +		 */
> > +		cancel_delayed_work(&power_domains->async_put_work);
> >  	}
> >  
> >  out_verify:
> > -- 
> > 2.27.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx