> -----Original Message----- > From: Deak, Imre <imre.deak@xxxxxxxxx> > Sent: Wednesday, June 2, 2021 7:31 PM > To: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [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@xxxxxxxxx> > > > 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@xxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
