[PATCH] drm/i915: Report request restarts for both execlists/guc

[PATCH] drm/i915: Report request restarts for both execlists/guc

[Chris Wilson]

As we now share the execlist_port[] tracking for both execlists/guc, we
can reset the inflight count on both and report which requests are being
restarted.

Suggested-by: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d3612969098f..961f4a2ad498 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1147,14 +1147,11 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
 	return ret;
 }
 
-static u32 port_seqno(struct execlist_port *port)
-{
-	return port->request ? port->request->global_seqno : 0;
-}
-
 static int gen8_init_common_ring(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
+	struct execlist_port *port = engine->execlist_port;
+	unsigned int n;
 	int ret;
 
 	ret = intel_mocs_init_engine(engine);
@@ -1175,16 +1172,22 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
 
 	/* After a GPU reset, we may have requests to replay */
 	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
-	if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) {
-		DRM_DEBUG_DRIVER("Restarting %s from requests [0x%x, 0x%x]\n",
-				 engine->name,
-				 port_seqno(&engine->execlist_port[0]),
-				 port_seqno(&engine->execlist_port[1]));
-		engine->execlist_port[0].count = 0;
-		engine->execlist_port[1].count = 0;
-		execlists_submit_ports(engine);
+
+	for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
+		if (!port[n].request)
+			break;
+
+		DRM_DEBUG_DRIVER("Restarting %s:%d from 0x%x\n",
+				 engine->name, n,
+				 port[n].request->global_seqno);
+
+		/* Discard the current inflight count */
+		port[n].count = 0;
 	}
 
+	if (!i915.enable_guc_submission && !execlists_elsp_idle(engine))
+		execlists_submit_ports(engine);
+
 	return 0;
 }
 
-- 
2.11.0

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

✓ Fi.CI.BAT: success for drm/i915: Report request restarts for both execlists/guc

[Patchwork]

== Series Details ==

Series: drm/i915: Report request restarts for both execlists/guc
URL   : https://patchwork.freedesktop.org/series/23502/
State : success

== Summary ==

Series 23502v1 drm/i915: Report request restarts for both execlists/guc
https://patchwork.freedesktop.org/api/1.0/series/23502/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125

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

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:428s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:437s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:582s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:506s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:487s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:484s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:411s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:408s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:422s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:492s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:462s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:458s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:656s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:451s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:573s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:461s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:490s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:433s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:533s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:401s

babeb6f2b1a90ac0ec1a47bc43a4671649a8fc20 drm-tip: 2017y-04m-25d-10h-04m-12s UTC integration manifest
3dcc455 drm/i915: Report request restarts for both execlists/guc

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4542/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Report request restarts for both execlists/guc

[Tvrtko Ursulin]

On 25/04/2017 11:38, Chris Wilson wrote:
> As we now share the execlist_port[] tracking for both execlists/guc, we
> can reset the inflight count on both and report which requests are being
> restarted.
>
> Suggested-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d3612969098f..961f4a2ad498 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1147,14 +1147,11 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
>  	return ret;
>  }
>
> -static u32 port_seqno(struct execlist_port *port)
> -{
> -	return port->request ? port->request->global_seqno : 0;
> -}
> -
>  static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
> +	struct execlist_port *port = engine->execlist_port;
> +	unsigned int n;
>  	int ret;
>
>  	ret = intel_mocs_init_engine(engine);
> @@ -1175,16 +1172,22 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>
>  	/* After a GPU reset, we may have requests to replay */
>  	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> -	if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) {
> -		DRM_DEBUG_DRIVER("Restarting %s from requests [0x%x, 0x%x]\n",
> -				 engine->name,
> -				 port_seqno(&engine->execlist_port[0]),
> -				 port_seqno(&engine->execlist_port[1]));
> -		engine->execlist_port[0].count = 0;
> -		engine->execlist_port[1].count = 0;
> -		execlists_submit_ports(engine);
> +
> +	for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
> +		if (!port[n].request)
> +			break;

At some point we'll maybe want to start thinking about the 
for_each_port_request or something.

> +
> +		DRM_DEBUG_DRIVER("Restarting %s:%d from 0x%x\n",
> +				 engine->name, n,
> +				 port[n].request->global_seqno);
> +
> +		/* Discard the current inflight count */
> +		port[n].count = 0;
>  	}
>
> +	if (!i915.enable_guc_submission && !execlists_elsp_idle(engine))
> +		execlists_submit_ports(engine);
> +
>  	return 0;
>  }
>
>

Looks okay to me. Someone has plans to start using counts in guc mode?

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH] drm/i915: Report request restarts for both execlists/guc

[Chris Wilson]

On Tue, Apr 25, 2017 at 01:21:47PM +0100, Tvrtko Ursulin wrote:
> 
> On 25/04/2017 11:38, Chris Wilson wrote:
> >As we now share the execlist_port[] tracking for both execlists/guc, we
> >can reset the inflight count on both and report which requests are being
> >restarted.
> >
> >Suggested-by: Michel Thierry <michel.thierry@intel.com>
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Michel Thierry <michel.thierry@intel.com>
> >Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> > drivers/gpu/drm/i915/intel_lrc.c | 29 ++++++++++++++++-------------
> > 1 file changed, 16 insertions(+), 13 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index d3612969098f..961f4a2ad498 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -1147,14 +1147,11 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
> > 	return ret;
> > }
> >
> >-static u32 port_seqno(struct execlist_port *port)
> >-{
> >-	return port->request ? port->request->global_seqno : 0;
> >-}
> >-
> > static int gen8_init_common_ring(struct intel_engine_cs *engine)
> > {
> > 	struct drm_i915_private *dev_priv = engine->i915;
> >+	struct execlist_port *port = engine->execlist_port;
> >+	unsigned int n;
> > 	int ret;
> >
> > 	ret = intel_mocs_init_engine(engine);
> >@@ -1175,16 +1172,22 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
> >
> > 	/* After a GPU reset, we may have requests to replay */
> > 	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> >-	if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) {
> >-		DRM_DEBUG_DRIVER("Restarting %s from requests [0x%x, 0x%x]\n",
> >-				 engine->name,
> >-				 port_seqno(&engine->execlist_port[0]),
> >-				 port_seqno(&engine->execlist_port[1]));
> >-		engine->execlist_port[0].count = 0;
> >-		engine->execlist_port[1].count = 0;
> >-		execlists_submit_ports(engine);
> >+
> >+	for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
> >+		if (!port[n].request)
> >+			break;
> 
> At some point we'll maybe want to start thinking about the
> for_each_port_request or something.

Something.

> >+
> >+		DRM_DEBUG_DRIVER("Restarting %s:%d from 0x%x\n",
> >+				 engine->name, n,
> >+				 port[n].request->global_seqno);
> >+
> >+		/* Discard the current inflight count */
> >+		port[n].count = 0;
> > 	}
> >
> >+	if (!i915.enable_guc_submission && !execlists_elsp_idle(engine))
> >+		execlists_submit_ports(engine);
> >+
> > 	return 0;
> > }
> >
> >
> 
> Looks okay to me. Someone has plans to start using counts in guc mode?

Spoilers. I moved the submission out of a few locks to reduce lock
contention (queued_spin_lock_slowpath exists for guc!), makes the CPU
numbers look better, but bxt/guc is still 3x higher latency. I just hope
it is broken firmware.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for drm/i915: Report request restarts for both execlists/guc

[Chris Wilson]

On Tue, Apr 25, 2017 at 11:58:46AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Report request restarts for both execlists/guc
> URL   : https://patchwork.freedesktop.org/series/23502/
> State : success
> 
> == Summary ==
> 
> Series 23502v1 drm/i915: Report request restarts for both execlists/guc
> https://patchwork.freedesktop.org/api/1.0/series/23502/revisions/1/mbox/
> 
> Test gem_exec_flush:
>         Subgroup basic-batch-kernel-default-uc:
>                 fail       -> PASS       (fi-snb-2600) fdo#100007
> Test gem_exec_suspend:
>         Subgroup basic-s4-devices:
>                 pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125
> 
> fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
> fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

Pushed, thanks for the review.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Report request restarts for both execlists/guc

[Michel Thierry]

On 25/04/17 05:30, Chris Wilson wrote:
> On Tue, Apr 25, 2017 at 01:21:47PM +0100, Tvrtko Ursulin wrote:
>>
>> On 25/04/2017 11:38, Chris Wilson wrote:
>>> As we now share the execlist_port[] tracking for both execlists/guc, we
>>> can reset the inflight count on both and report which requests are being
>>> restarted.
>>>

Thanks, one less patch for me (and I arrived late to the party, I see 
it's already merged).

>>> Suggested-by: Michel Thierry <michel.thierry@intel.com>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Michel Thierry <michel.thierry@intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_lrc.c | 29 ++++++++++++++++-------------
>>> 1 file changed, 16 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index d3612969098f..961f4a2ad498 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -1147,14 +1147,11 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
>>>     return ret;
>>> }
>>>
>>> -static u32 port_seqno(struct execlist_port *port)
>>> -{
>>> -    return port->request ? port->request->global_seqno : 0;
>>> -}
>>> -
>>> static int gen8_init_common_ring(struct intel_engine_cs *engine)
>>> {
>>>     struct drm_i915_private *dev_priv = engine->i915;
>>> +    struct execlist_port *port = engine->execlist_port;
>>> +    unsigned int n;
>>>     int ret;
>>>
>>>     ret = intel_mocs_init_engine(engine);
>>> @@ -1175,16 +1172,22 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>>>
>>>     /* After a GPU reset, we may have requests to replay */
>>>     clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
>>> -    if (!i915.enable_guc_submission && !execlists_elsp_idle(engine)) {
>>> -            DRM_DEBUG_DRIVER("Restarting %s from requests [0x%x, 0x%x]\n",
>>> -                             engine->name,
>>> -                             port_seqno(&engine->execlist_port[0]),
>>> -                             port_seqno(&engine->execlist_port[1]));
>>> -            engine->execlist_port[0].count = 0;
>>> -            engine->execlist_port[1].count = 0;
>>> -            execlists_submit_ports(engine);
>>> +
>>> +    for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
>>> +            if (!port[n].request)
>>> +                    break;
>>
>> At some point we'll maybe want to start thinking about the
>> for_each_port_request or something.
>
> Something.
>
>>> +
>>> +            DRM_DEBUG_DRIVER("Restarting %s:%d from 0x%x\n",
>>> +                             engine->name, n,
>>> +                             port[n].request->global_seqno);
>>> +
>>> +            /* Discard the current inflight count */
>>> +            port[n].count = 0;
>>>     }
>>>
>>> +    if (!i915.enable_guc_submission && !execlists_elsp_idle(engine))
>>> +            execlists_submit_ports(engine);
>>> +
>>>     return 0;
>>> }
>>>
>>>
>>
>> Looks okay to me. Someone has plans to start using counts in guc mode?
>
> Spoilers. I moved the submission out of a few locks to reduce lock
> contention (queued_spin_lock_slowpath exists for guc!), makes the CPU
> numbers look better, but bxt/guc is still 3x higher latency. I just hope
> it is broken firmware.

Beating a dead horse?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx