[PATCH] drm/i915/execlists: Clear user-active flag on preemption completion

[PATCH] drm/i915/execlists: Clear user-active flag on preemption completion

[Chris Wilson]

When cancelling the requests and clearing out the ports following a
successful preemption completion, also clear the active flag. I had
assumed that all preemptions would be followed by an immediate dequeue
(preserving the active user flag), but under rare circumstances we may
be triggering a preemption for the second port only for it to have
completed before the preemotion kicks in; leaving execlists->active set
even though the system is now idle.

We can clear the flag inside the common execlists_cancel_port_requests()
as the other users also expect the semantics of active being cleared.

Fixes: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 08d8ac9d1f8f..f9edfe4540e2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -577,6 +577,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		 * know the next preemption status we see corresponds
 		 * to this ELSP update.
 		 */
+		GEM_BUG_ON(!execlists_is_active(execlists,
+						EXECLISTS_ACTIVE_USER));
 		GEM_BUG_ON(!port_count(&port[0]));
 		if (port_count(&port[0]) > 1)
 			goto unlock;
@@ -738,6 +740,8 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
 		memset(port, 0, sizeof(*port));
 		port++;
 	}
+
+	execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER);
 }
 
 static void clear_gtiir(struct intel_engine_cs *engine)
@@ -1042,6 +1046,11 @@ static void execlists_submission_tasklet(unsigned long data)
 
 	if (fw)
 		intel_uncore_forcewake_put(dev_priv, execlists->fw_domains);
+
+	/* If the engine is now idle, so should be the flag; and vice versa. */
+	GEM_BUG_ON(execlists_is_active(&engine->execlists,
+				       EXECLISTS_ACTIVE_USER) ==
+		   !port_isset(engine->execlists.port));
 }
 
 static void queue_request(struct intel_engine_cs *engine,
-- 
2.16.3

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

✓ Fi.CI.BAT: success for drm/i915/execlists: Clear user-active flag on preemption completion

[Patchwork]

== Series Details ==

Series: drm/i915/execlists: Clear user-active flag on preemption completion
URL   : https://patchwork.freedesktop.org/series/40617/
State : success

== Summary ==

Series 40617v1 drm/i915/execlists: Clear user-active flag on preemption completion
https://patchwork.freedesktop.org/api/1.0/series/40617/revisions/1/mbox/

---- Known issues:

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                incomplete -> PASS       (fi-bxt-dsi) fdo#103927
                incomplete -> PASS       (fi-hsw-4770) fdo#104944

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#104944 https://bugs.freedesktop.org/show_bug.cgi?id=104944

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:433s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:442s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:378s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:539s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:295s
fi-bxt-dsi       total:285  pass:255  dwarn:0   dfail:0   fail:0   skip:30  time:519s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:515s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:520s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:505s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:410s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:510s
fi-elk-e7500     total:285  pass:225  dwarn:1   dfail:0   fail:0   skip:59  time:433s
fi-gdg-551       total:285  pass:176  dwarn:0   dfail:0   fail:1   skip:108 time:318s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:534s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:407s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:422s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:474s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:429s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:476s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:515s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:652s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:437s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:539s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:502s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:487s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:430s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:444s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:590s
Blacklisted hosts:
fi-cfl-s3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:565s
fi-cnl-psr       total:224  pass:198  dwarn:0   dfail:0   fail:1   skip:24 
fi-glk-j4005     total:285  pass:226  dwarn:30  dfail:0   fail:0   skip:29  time:488s

94f5d9189e61055e246c31106b3810dc17ddee9c drm-tip: 2018y-03m-23d-23h-41m-40s UTC integration manifest
8277b4a917d9 drm/i915/execlists: Clear user-active flag on preemption completion

== Logs ==

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

✗ Fi.CI.IGT: failure for drm/i915/execlists: Clear user-active flag on preemption completion

[Patchwork]

== Series Details ==

Series: drm/i915/execlists: Clear user-active flag on preemption completion
URL   : https://patchwork.freedesktop.org/series/40617/
State : failure

== Summary ==

---- Possible new issues:

Test drv_selftest:
        Subgroup live_hangcheck:
                pass       -> INCOMPLETE (shard-apl)

---- Known issues:

Test kms_flip:
        Subgroup 2x-flip-vs-expired-vblank:
                pass       -> FAIL       (shard-hsw) fdo#102887 +1
        Subgroup 2x-plain-flip-ts-check-interruptible:
                fail       -> PASS       (shard-hsw) fdo#100368
        Subgroup modeset-vs-vblank-race-interruptible:
                dmesg-fail -> PASS       (shard-hsw) fdo#103060 +2
Test kms_rotation_crc:
        Subgroup sprite-rotation-180:
                fail       -> PASS       (shard-snb) fdo#103925
Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-apl) fdo#99912

fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-apl        total:3466 pass:1801 dwarn:1   dfail:0   fail:7   skip:1655 time:12452s
shard-hsw        total:3484 pass:1773 dwarn:1   dfail:0   fail:2   skip:1707 time:11665s
shard-snb        total:3484 pass:1363 dwarn:1   dfail:0   fail:3   skip:2117 time:7032s
Blacklisted hosts:
shard-kbl        total:3466 pass:1920 dwarn:5   dfail:1   fail:10  skip:1529 time:9637s

== Logs ==

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

Re: [PATCH] drm/i915/execlists: Clear user-active flag on preemption completion

[Mika Kuoppala]

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

> When cancelling the requests and clearing out the ports following a
> successful preemption completion, also clear the active flag. I had
> assumed that all preemptions would be followed by an immediate dequeue
> (preserving the active user flag), but under rare circumstances we may
> be triggering a preemption for the second port only for it to have
> completed before the preemotion kicks in; leaving execlists->active set
> even though the system is now idle.
>
> We can clear the flag inside the common execlists_cancel_port_requests()
> as the other users also expect the semantics of active being cleared.
>
> Fixes: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 08d8ac9d1f8f..f9edfe4540e2 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -577,6 +577,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  		 * know the next preemption status we see corresponds
>  		 * to this ELSP update.
>  		 */
> +		GEM_BUG_ON(!execlists_is_active(execlists,
> +						EXECLISTS_ACTIVE_USER));

We have a similar type of check in function exit.
But that would trigger only if we are lite restoring to port[0]. 

So more coverage with this and being explicit...

>  		GEM_BUG_ON(!port_count(&port[0]));
>  		if (port_count(&port[0]) > 1)
>  			goto unlock;
> @@ -738,6 +740,8 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
>  		memset(port, 0, sizeof(*port));
>  		port++;
>  	}
> +
> +	execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER);
>  }
>  
>  static void clear_gtiir(struct intel_engine_cs *engine)
> @@ -1042,6 +1046,11 @@ static void execlists_submission_tasklet(unsigned long data)
>  
>  	if (fw)
>  		intel_uncore_forcewake_put(dev_priv, execlists->fw_domains);
> +
> +	/* If the engine is now idle, so should be the flag; and vice versa. */
> +	GEM_BUG_ON(execlists_is_active(&engine->execlists,
> +				       EXECLISTS_ACTIVE_USER) ==
> +		   !port_isset(engine->execlists.port));

But this here looks like we could get rid of the
	GEM_BUG_ON(port_isset(execlists->port) &&
		   !execlists_is_active(execlists, EXECLISTS_ACTIVE_USER));

on end of dequeue and trust this master check you added here.
-Mika

>  }
>  
>  static void queue_request(struct intel_engine_cs *engine,
> -- 
> 2.16.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: Clear user-active flag on preemption completion

[Chris Wilson]

Quoting Mika Kuoppala (2018-03-27 09:18:55)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > When cancelling the requests and clearing out the ports following a
> > successful preemption completion, also clear the active flag. I had
> > assumed that all preemptions would be followed by an immediate dequeue
> > (preserving the active user flag), but under rare circumstances we may
> > be triggering a preemption for the second port only for it to have
> > completed before the preemotion kicks in; leaving execlists->active set
> > even though the system is now idle.
> >
> > We can clear the flag inside the common execlists_cancel_port_requests()
> > as the other users also expect the semantics of active being cleared.
> >
> > Fixes: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 08d8ac9d1f8f..f9edfe4540e2 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -577,6 +577,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >                * know the next preemption status we see corresponds
> >                * to this ELSP update.
> >                */
> > +             GEM_BUG_ON(!execlists_is_active(execlists,
> > +                                             EXECLISTS_ACTIVE_USER));
> 
> We have a similar type of check in function exit.

Yes.

> But that would trigger only if we are lite restoring to port[0]. 
> 
> So more coverage with this and being explicit...

And my purpose here was to reinforce the notion that execlists *must* be
active if we still have an active ELSP[0] (same level as asserting
port_count).

> 
> >               GEM_BUG_ON(!port_count(&port[0]));
> >               if (port_count(&port[0]) > 1)
> >                       goto unlock;
> > @@ -738,6 +740,8 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
> >               memset(port, 0, sizeof(*port));
> >               port++;
> >       }
> > +
> > +     execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER);
> >  }
> >  
> >  static void clear_gtiir(struct intel_engine_cs *engine)
> > @@ -1042,6 +1046,11 @@ static void execlists_submission_tasklet(unsigned long data)
> >  
> >       if (fw)
> >               intel_uncore_forcewake_put(dev_priv, execlists->fw_domains);
> > +
> > +     /* If the engine is now idle, so should be the flag; and vice versa. */
> > +     GEM_BUG_ON(execlists_is_active(&engine->execlists,
> > +                                    EXECLISTS_ACTIVE_USER) ==
> > +                !port_isset(engine->execlists.port));
> 
> But this here looks like we could get rid of the
>         GEM_BUG_ON(port_isset(execlists->port) &&
>                    !execlists_is_active(execlists, EXECLISTS_ACTIVE_USER));
> 
> on end of dequeue and trust this master check you added here.

We could but there's a plan to split this path up a bit, and I want to
move that earlier check around.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: Clear user-active flag on preemption completion

[Mika Kuoppala]

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

> When cancelling the requests and clearing out the ports following a
> successful preemption completion, also clear the active flag. I had
> assumed that all preemptions would be followed by an immediate dequeue
> (preserving the active user flag), but under rare circumstances we may
> be triggering a preemption for the second port only for it to have
> completed before the preemotion kicks in; leaving execlists->active set
> even though the system is now idle.
>
> We can clear the flag inside the common execlists_cancel_port_requests()
> as the other users also expect the semantics of active being cleared.
>
> Fixes: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 08d8ac9d1f8f..f9edfe4540e2 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -577,6 +577,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  		 * know the next preemption status we see corresponds
>  		 * to this ELSP update.
>  		 */
> +		GEM_BUG_ON(!execlists_is_active(execlists,
> +						EXECLISTS_ACTIVE_USER));
>  		GEM_BUG_ON(!port_count(&port[0]));
>  		if (port_count(&port[0]) > 1)
>  			goto unlock;
> @@ -738,6 +740,8 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
>  		memset(port, 0, sizeof(*port));
>  		port++;
>  	}
> +
> +	execlists_clear_active(execlists, EXECLISTS_ACTIVE_USER);
>  }
>  
>  static void clear_gtiir(struct intel_engine_cs *engine)
> @@ -1042,6 +1046,11 @@ static void execlists_submission_tasklet(unsigned long data)
>  
>  	if (fw)
>  		intel_uncore_forcewake_put(dev_priv, execlists->fw_domains);
> +
> +	/* If the engine is now idle, so should be the flag; and vice versa. */
> +	GEM_BUG_ON(execlists_is_active(&engine->execlists,
> +				       EXECLISTS_ACTIVE_USER) ==
> +		   !port_isset(engine->execlists.port));
>  }
>  
>  static void queue_request(struct intel_engine_cs *engine,
> -- 
> 2.16.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: Clear user-active flag on preemption completion

[Chris Wilson]

Quoting Mika Kuoppala (2018-03-27 10:22:11)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > When cancelling the requests and clearing out the ports following a
> > successful preemption completion, also clear the active flag. I had
> > assumed that all preemptions would be followed by an immediate dequeue
> > (preserving the active user flag), but under rare circumstances we may
> > be triggering a preemption for the second port only for it to have
> > completed before the preemotion kicks in; leaving execlists->active set
> > even though the system is now idle.
> >
> > We can clear the flag inside the common execlists_cancel_port_requests()
> > as the other users also expect the semantics of active being cleared.
> >
> > Fixes: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Applied to have one less random failure around preemption. Unlikely CI
will hit as we simply don't apply enough stress.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx