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

[Gupta, Anshuman]

> -----Original Message-----
> From: Deak, Imre <imre.deak at intel.com>
> Sent: Wednesday, June 2, 2021 7:31 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Fix incorrect assert about pending
> power domain async-put work
> 
> 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 for explaining it.
Patch looks good to me.
Reviewed-by: Anshuman Gupta <anshuman.gupta at intel.com>
> 
> > 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