[PATCH] drm/i915: Try harder to finish the idle-worker

[PATCH] drm/i915: Try harder to finish the idle-worker

[Chris Wilson]

If a worker requeues itself, it may switch to a different kworker pool,
which flush_work() considers as complete. To be strict, we then need to
keep flushing the work until it is no longer pending.

References: https://bugs.freedesktop.org/show_bug.cgi?id=102456
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  3 +--
 drivers/gpu/drm/i915/i915_gem.c     |  3 +--
 drivers/gpu/drm/i915/i915_utils.h   | 13 +++++++++++++
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 48572b157222..1dd687a74879 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4202,8 +4202,7 @@ fault_irq_set(struct drm_i915_private *i915,
 	mutex_unlock(&i915->drm.struct_mutex);
 
 	/* Flush idle worker to disarm irq */
-	while (flush_delayed_work(&i915->gt.idle_work))
-		;
+	drain_delayed_work(&i915->gt.idle_work);
 
 	return 0;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e4cc08bc518c..1829d3fa15d1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4582,8 +4582,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	/* As the idle_work is rearming if it detects a race, play safe and
 	 * repeat the flush until it is definitely idle.
 	 */
-	while (flush_delayed_work(&dev_priv->gt.idle_work))
-		;
+	drain_delayed_work(&dev_priv->gt.idle_work);
 
 	/* Assert that we sucessfully flushed all the work and
 	 * reset the GPU back to its idle, low power state.
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 12fc250b47b9..4f7ffa0976b1 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -119,4 +119,17 @@ static inline void __list_del_many(struct list_head *head,
 	WRITE_ONCE(head->next, first);
 }
 
+/*
+ * Wait until the work is finally complete, even if it tries to postpone
+ * by requeueing itself. Note, that if the worker never cancels itself,
+ * we will spin forever.
+ */
+static inline void drain_delayed_work(struct delayed_work *dw)
+{
+	do {
+		while (flush_delayed_work(dw))
+			;
+	} while (delayed_work_pending(dw));
+}
+
 #endif /* !__I915_UTILS_H */
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: Try harder to finish the idle-worker

[Patchwork]

== Series Details ==

Series: drm/i915: Try harder to finish the idle-worker
URL   : https://patchwork.freedesktop.org/series/29690/
State : warning

== Summary ==

Series 29690v1 drm/i915: Try harder to finish the idle-worker
https://patchwork.freedesktop.org/api/1.0/series/29690/revisions/1/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                skip       -> PASS       (fi-skl-x1585l) fdo#101781
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a:
                pass       -> DMESG-WARN (fi-byt-j1900)

fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781

fi-bdw-5557u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:455s
fi-bdw-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:446s
fi-blb-e6850     total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  time:364s
fi-bsw-n3050     total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  time:556s
fi-bwr-2160      total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 time:254s
fi-bxt-j4205     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:523s
fi-byt-j1900     total:288  pass:253  dwarn:2   dfail:0   fail:0   skip:33  time:531s
fi-byt-n2820     total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  time:516s
fi-elk-e7500     total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  time:442s
fi-glk-2a        total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:610s
fi-hsw-4770      total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:451s
fi-hsw-4770r     total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:424s
fi-ilk-650       total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:421s
fi-ivb-3520m     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:508s
fi-ivb-3770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:478s
fi-kbl-7500u     total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  time:512s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:597s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:600s
fi-pnv-d510      total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:524s
fi-skl-6260u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:472s
fi-skl-6700k     total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:537s
fi-skl-6770hq    total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:484s
fi-skl-gvtdvm    total:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  time:443s
fi-skl-x1585l    total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:510s
fi-snb-2520m     total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  time:550s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:2   skip:38  time:406s

8b962a9a16b3a4e3cc2a56cba82ee9c0dc0941b5 drm-tip: 2017y-09m-01d-13h-56m-06s UTC integration manifest
17a1e4e11085 drm/i915: Try harder to finish the idle-worker

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5563/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Try harder to finish the idle-worker

[Daniel Vetter]

On Fri, Sep 01, 2017 at 03:11:23PM +0100, Chris Wilson wrote:
> If a worker requeues itself, it may switch to a different kworker pool,
> which flush_work() considers as complete. To be strict, we then need to
> keep flushing the work until it is no longer pending.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=102456
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>

Shouldn't this be a thing the workqueue subsystem exposes? Adding Tejun et
al.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  3 +--
>  drivers/gpu/drm/i915/i915_gem.c     |  3 +--
>  drivers/gpu/drm/i915/i915_utils.h   | 13 +++++++++++++
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 48572b157222..1dd687a74879 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4202,8 +4202,7 @@ fault_irq_set(struct drm_i915_private *i915,
>  	mutex_unlock(&i915->drm.struct_mutex);
>  
>  	/* Flush idle worker to disarm irq */
> -	while (flush_delayed_work(&i915->gt.idle_work))
> -		;
> +	drain_delayed_work(&i915->gt.idle_work);
>  
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e4cc08bc518c..1829d3fa15d1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4582,8 +4582,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  	/* As the idle_work is rearming if it detects a race, play safe and
>  	 * repeat the flush until it is definitely idle.
>  	 */
> -	while (flush_delayed_work(&dev_priv->gt.idle_work))
> -		;
> +	drain_delayed_work(&dev_priv->gt.idle_work);
>  
>  	/* Assert that we sucessfully flushed all the work and
>  	 * reset the GPU back to its idle, low power state.
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index 12fc250b47b9..4f7ffa0976b1 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -119,4 +119,17 @@ static inline void __list_del_many(struct list_head *head,
>  	WRITE_ONCE(head->next, first);
>  }
>  
> +/*
> + * Wait until the work is finally complete, even if it tries to postpone
> + * by requeueing itself. Note, that if the worker never cancels itself,
> + * we will spin forever.
> + */
> +static inline void drain_delayed_work(struct delayed_work *dw)
> +{
> +	do {
> +		while (flush_delayed_work(dw))
> +			;
> +	} while (delayed_work_pending(dw));
> +}
> +
>  #endif /* !__I915_UTILS_H */
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Try harder to finish the idle-worker

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> If a worker requeues itself, it may switch to a different kworker pool,
> which flush_work() considers as complete. To be strict, we then need to
> keep flushing the work until it is no longer pending.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=102456
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  3 +--
>  drivers/gpu/drm/i915/i915_gem.c     |  3 +--
>  drivers/gpu/drm/i915/i915_utils.h   | 13 +++++++++++++
>  3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 48572b157222..1dd687a74879 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4202,8 +4202,7 @@ fault_irq_set(struct drm_i915_private *i915,
>  	mutex_unlock(&i915->drm.struct_mutex);
>  
>  	/* Flush idle worker to disarm irq */
> -	while (flush_delayed_work(&i915->gt.idle_work))
> -		;
> +	drain_delayed_work(&i915->gt.idle_work);
>  
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e4cc08bc518c..1829d3fa15d1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4582,8 +4582,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  	/* As the idle_work is rearming if it detects a race, play safe and
>  	 * repeat the flush until it is definitely idle.
>  	 */
> -	while (flush_delayed_work(&dev_priv->gt.idle_work))
> -		;
> +	drain_delayed_work(&dev_priv->gt.idle_work);
>  
>  	/* Assert that we sucessfully flushed all the work and
>  	 * reset the GPU back to its idle, low power state.
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index 12fc250b47b9..4f7ffa0976b1 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -119,4 +119,17 @@ static inline void __list_del_many(struct list_head *head,
>  	WRITE_ONCE(head->next, first);
>  }
>  
> +/*
> + * Wait until the work is finally complete, even if it tries to postpone
> + * by requeueing itself. Note, that if the worker never cancels itself,
> + * we will spin forever.
> + */
> +static inline void drain_delayed_work(struct delayed_work *dw)
> +{
> +	do {
> +		while (flush_delayed_work(dw))
> +			;
> +	} while (delayed_work_pending(dw));

So we end spinning if someone doesn't let go of mutex.

Add a timeout for doing this and let it slide into
suspend with a error message anyways instead of spinning
forever?

-Mika

> +}
> +
>  #endif /* !__I915_UTILS_H */
> -- 
> 2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Try harder to finish the idle-worker

[Chris Wilson]

Quoting Mika Kuoppala (2017-09-04 11:19:09)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > +/*
> > + * Wait until the work is finally complete, even if it tries to postpone
> > + * by requeueing itself. Note, that if the worker never cancels itself,
> > + * we will spin forever.
> > + */
> > +static inline void drain_delayed_work(struct delayed_work *dw)
> > +{
> > +     do {
> > +             while (flush_delayed_work(dw))
> > +                     ;
> > +     } while (delayed_work_pending(dw));
> 
> So we end spinning if someone doesn't let go of mutex.
> 
> Add a timeout for doing this and let it slide into
> suspend with a error message anyways instead of spinning
> forever?

Such error messages already exist (NMI). But for us if we skip flushing
the work, we leave the driver in a very inconsistent state and expect to
blow up on resume. It's a lose-lose.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Try harder to finish the idle-worker

[Chris Wilson]

Quoting Daniel Vetter (2017-09-04 09:35:49)
> On Fri, Sep 01, 2017 at 03:11:23PM +0100, Chris Wilson wrote:
> > If a worker requeues itself, it may switch to a different kworker pool,
> > which flush_work() considers as complete. To be strict, we then need to
> > keep flushing the work until it is no longer pending.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=102456
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> Shouldn't this be a thing the workqueue subsystem exposes? Adding Tejun et
> al.

The semantics are so horrible that I wouldn't suggest that the core
should expose such a nasty trap. You have to be absolutely certain that
your work stops requeueing itself.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Try harder to finish the idle-worker

[Tejun Heo]

On Mon, Sep 04, 2017 at 10:35:49AM +0200, Daniel Vetter wrote:
> On Fri, Sep 01, 2017 at 03:11:23PM +0100, Chris Wilson wrote:
> > If a worker requeues itself, it may switch to a different kworker pool,
> > which flush_work() considers as complete. To be strict, we then need to
> > keep flushing the work until it is no longer pending.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=102456
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> Shouldn't this be a thing the workqueue subsystem exposes? Adding Tejun et
> al.
> -Daniel

Can't you use cancel[_delayed]_work_sync()?

Thanks.

-- 
tejun
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Try harder to finish the idle-worker

[Chris Wilson]

Quoting Tejun Heo (2017-09-05 14:36:28)
> On Mon, Sep 04, 2017 at 10:35:49AM +0200, Daniel Vetter wrote:
> > On Fri, Sep 01, 2017 at 03:11:23PM +0100, Chris Wilson wrote:
> > > If a worker requeues itself, it may switch to a different kworker pool,
> > > which flush_work() considers as complete. To be strict, we then need to
> > > keep flushing the work until it is no longer pending.
> > > 
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=102456
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > 
> > Shouldn't this be a thing the workqueue subsystem exposes? Adding Tejun et
> > al.
> > -Daniel
> 
> Can't you use cancel[_delayed]_work_sync()?

We then need a loop like:

	do {
		if (cancel_delayed_work_sync(wrk))
			do_work(wrk);
		else
			break;
	} while (1);

We do want the flush semantics.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Try harder to finish the idle-worker

[Tejun Heo]

Hello,

On Tue, Sep 05, 2017 at 02:43:14PM +0100, Chris Wilson wrote:
> > Can't you use cancel[_delayed]_work_sync()?
> 
> We then need a loop like:
> 
> 	do {
> 		if (cancel_delayed_work_sync(wrk))
> 			do_work(wrk);
> 		else
> 			break;
> 	} while (1);
> 
> We do want the flush semantics.

I see.  Heh, I don't know.  One thing you can try to do is putting
them on a separate workqueue and use drain_workqueue() or
destroy_workqueue() on it.  Those functions expect there to be some
requeueing but warns if there are too much (non configurable now but
we can add if necessary).

Thanks.

-- 
tejun
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx