[PATCH] drm/i915: Tidy up GEM_TRACE around execlists

[PATCH] drm/i915: Tidy up GEM_TRACE around execlists

[Chris Wilson]

Looking at the coordination of resets with the submission of execlists,
it will be useful to have a GEM_TRACE for when we issue the reset.
Whilst there tidy up the other GEM_TRACE to always include the engine
name, and be careful not to trust any pointers prior to asserts.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 64d49d5054b9..727d77b9f9d3 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -504,7 +504,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
 	ce->ring->tail &= (ce->ring->size - 1);
 	ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
 
-	GEM_TRACE("\n");
+	GEM_TRACE("%s\n", engine->name);
 	for (n = execlists_num_ports(&engine->execlists); --n; )
 		elsp_write(0, engine->execlists.elsp);
 
@@ -906,7 +906,7 @@ static void execlists_submission_tasklet(unsigned long data)
 			GEM_TRACE("%s out[0]: ctx=%d.%d, seqno=%x\n",
 				  engine->name,
 				  port->context_id, count,
-				  rq->global_seqno);
+				  rq ? rq->global_seqno : 0);
 			GEM_BUG_ON(count == 0);
 			if (--count == 0) {
 				GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
@@ -1544,6 +1544,9 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	struct intel_context *ce;
 	unsigned long flags;
 
+	GEM_TRACE("%s: seqno=%x\n",
+		  engine->name, request ? request->global_seqno : 0);
+
 	/*
 	 * Clear any pending interrupt state.
 	 *
-- 
2.15.1

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

✗ Fi.CI.BAT: failure for drm/i915: Tidy up GEM_TRACE around execlists

[Patchwork]

== Series Details ==

Series: drm/i915: Tidy up GEM_TRACE around execlists
URL   : https://patchwork.freedesktop.org/series/35611/
State : failure

== Summary ==

Applying: drm/i915: Tidy up GEM_TRACE around execlists
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_lrc.c).
error: could not build fake ancestor
Patch failed at 0001 drm/i915: Tidy up GEM_TRACE around execlists
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

[PATCH] drm/i915: Tidy up GEM_TRACE around execlists

[Chris Wilson]

Looking at the coordination of resets with the submission of execlists,
it will be useful to have a GEM_TRACE for when we issue the reset.
Whilst there tidy up the other GEM_TRACE to always include the engine
name, and be careful not to trust any pointers prior to asserts.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6205d320b86f..84029806cf3f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -504,7 +504,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
 	ce->ring->tail &= (ce->ring->size - 1);
 	ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
 
-	GEM_TRACE("\n");
+	GEM_TRACE("%s\n", engine->name);
 	for (n = execlists_num_ports(&engine->execlists); --n; )
 		elsp_write(0, engine->execlists.elsp);
 
@@ -906,7 +906,7 @@ static void execlists_submission_tasklet(unsigned long data)
 			GEM_TRACE("%s out[0]: ctx=%d.%d, seqno=%x\n",
 				  engine->name,
 				  port->context_id, count,
-				  rq->global_seqno);
+				  rq ? rq->global_seqno : 0);
 			GEM_BUG_ON(count == 0);
 			if (--count == 0) {
 				GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
@@ -1555,6 +1555,8 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	struct intel_context *ce;
 	unsigned long flags;
 
+	GEM_TRACE("%s: seqno=%x\n",
+		  engine->name, request ? request->global_seqno : 0);
 	spin_lock_irqsave(&engine->timeline->lock, flags);
 
 	/*
-- 
2.15.1

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

✗ Fi.CI.BAT: failure for drm/i915: Tidy up GEM_TRACE around execlists (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Tidy up GEM_TRACE around execlists (rev2)
URL   : https://patchwork.freedesktop.org/series/35611/
State : failure

== Summary ==

Series 35611v2 drm/i915: Tidy up GEM_TRACE around execlists
https://patchwork.freedesktop.org/api/1.0/series/35611/revisions/2/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> FAIL       (fi-elk-e7500) fdo#103989 +1
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-c:
                pass       -> FAIL       (fi-skl-6700k2)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:432s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:380s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:497s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:275s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:493s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:500s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:473s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:470s
fi-elk-e7500     total:224  pass:162  dwarn:15  dfail:0   fail:1   skip:45 
fi-gdg-551       total:288  pass:178  dwarn:1   dfail:0   fail:1   skip:108 time:261s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:526s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:404s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:415s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:383s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:480s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:424s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:485s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:521s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:520s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:590s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:441s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:530s
fi-skl-6700hq    total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:555s
fi-skl-6700k2    total:288  pass:263  dwarn:0   dfail:0   fail:1   skip:24  time:505s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:501s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:441s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:552s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:409s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:591s
fi-cnl-y         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:616s

e27ee23e076d5ac016fec87e3273be9cc91d9b20 drm-tip: 2017y-12m-19d-23h-07m-26s UTC integration manifest
3052b04d0dd4 drm/i915: Tidy up GEM_TRACE around execlists

== Logs ==

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

Re: [PATCH] drm/i915: Tidy up GEM_TRACE around execlists

[Tvrtko Ursulin]

On 20/12/2017 09:06, Chris Wilson wrote:
> Looking at the coordination of resets with the submission of execlists,
> it will be useful to have a GEM_TRACE for when we issue the reset.
> Whilst there tidy up the other GEM_TRACE to always include the engine
> name, and be careful not to trust any pointers prior to asserts.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 6205d320b86f..84029806cf3f 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -504,7 +504,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
>   	ce->ring->tail &= (ce->ring->size - 1);
>   	ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
>   
> -	GEM_TRACE("\n");
> +	GEM_TRACE("%s\n", engine->name);
>   	for (n = execlists_num_ports(&engine->execlists); --n; )
>   		elsp_write(0, engine->execlists.elsp);
>   
> @@ -906,7 +906,7 @@ static void execlists_submission_tasklet(unsigned long data)
>   			GEM_TRACE("%s out[0]: ctx=%d.%d, seqno=%x\n",
>   				  engine->name,
>   				  port->context_id, count,
> -				  rq->global_seqno);
> +				  rq ? rq->global_seqno : 0);

rq can be NULL here? What's with the --count below then, wouldn't it 
underflow since I assume rq == NULL means count == 0 as well?

>   			GEM_BUG_ON(count == 0);
>   			if (--count == 0) {
>   				GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
> @@ -1555,6 +1555,8 @@ static void reset_common_ring(struct intel_engine_cs *engine,
>   	struct intel_context *ce;
>   	unsigned long flags;
>   
> +	GEM_TRACE("%s: seqno=%x\n",
> +		  engine->name, request ? request->global_seqno : 0);

This one is legit.

>   	spin_lock_irqsave(&engine->timeline->lock, flags);
>   
>   	/*
> 

Regards,

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

Re: [PATCH] drm/i915: Tidy up GEM_TRACE around execlists

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-12-20 12:39:42)
> 
> On 20/12/2017 09:06, Chris Wilson wrote:
> > Looking at the coordination of resets with the submission of execlists,
> > it will be useful to have a GEM_TRACE for when we issue the reset.
> > Whilst there tidy up the other GEM_TRACE to always include the engine
> > name, and be careful not to trust any pointers prior to asserts.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 6205d320b86f..84029806cf3f 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -504,7 +504,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
> >       ce->ring->tail &= (ce->ring->size - 1);
> >       ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
> >   
> > -     GEM_TRACE("\n");
> > +     GEM_TRACE("%s\n", engine->name);
> >       for (n = execlists_num_ports(&engine->execlists); --n; )
> >               elsp_write(0, engine->execlists.elsp);
> >   
> > @@ -906,7 +906,7 @@ static void execlists_submission_tasklet(unsigned long data)
> >                       GEM_TRACE("%s out[0]: ctx=%d.%d, seqno=%x\n",
> >                                 engine->name,
> >                                 port->context_id, count,
> > -                               rq->global_seqno);
> > +                               rq ? rq->global_seqno : 0);
> 
> rq can be NULL here? What's with the --count below then, wouldn't it 
> underflow since I assume rq == NULL means count == 0 as well?

Exactly. If rq is NULL then count must be 0. However, we want to show the
output from before the BUG_ON so that we have it available in case the
BUG does explode.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Tidy up GEM_TRACE around execlists

[Tvrtko Ursulin]

On 20/12/2017 12:43, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-12-20 12:39:42)
>>
>> On 20/12/2017 09:06, Chris Wilson wrote:
>>> Looking at the coordination of resets with the submission of execlists,
>>> it will be useful to have a GEM_TRACE for when we issue the reset.
>>> Whilst there tidy up the other GEM_TRACE to always include the engine
>>> name, and be careful not to trust any pointers prior to asserts.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 6205d320b86f..84029806cf3f 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -504,7 +504,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
>>>        ce->ring->tail &= (ce->ring->size - 1);
>>>        ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
>>>    
>>> -     GEM_TRACE("\n");
>>> +     GEM_TRACE("%s\n", engine->name);
>>>        for (n = execlists_num_ports(&engine->execlists); --n; )
>>>                elsp_write(0, engine->execlists.elsp);
>>>    
>>> @@ -906,7 +906,7 @@ static void execlists_submission_tasklet(unsigned long data)
>>>                        GEM_TRACE("%s out[0]: ctx=%d.%d, seqno=%x\n",
>>>                                  engine->name,
>>>                                  port->context_id, count,
>>> -                               rq->global_seqno);
>>> +                               rq ? rq->global_seqno : 0);
>>
>> rq can be NULL here? What's with the --count below then, wouldn't it
>> underflow since I assume rq == NULL means count == 0 as well?
> 
> Exactly. If rq is NULL then count must be 0. However, we want to show the
> output from before the BUG_ON so that we have it available in case the
> BUG does explode.

Okay makes sense.

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: Tidy up GEM_TRACE around execlists

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-12-20 13:21:45)
> 
> On 20/12/2017 12:43, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-12-20 12:39:42)
> >>
> >> On 20/12/2017 09:06, Chris Wilson wrote:
> >>> Looking at the coordination of resets with the submission of execlists,
> >>> it will be useful to have a GEM_TRACE for when we issue the reset.
> >>> Whilst there tidy up the other GEM_TRACE to always include the engine
> >>> name, and be careful not to trust any pointers prior to asserts.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
> >>>    1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>> index 6205d320b86f..84029806cf3f 100644
> >>> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>> @@ -504,7 +504,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
> >>>        ce->ring->tail &= (ce->ring->size - 1);
> >>>        ce->lrc_reg_state[CTX_RING_TAIL+1] = ce->ring->tail;
> >>>    
> >>> -     GEM_TRACE("\n");
> >>> +     GEM_TRACE("%s\n", engine->name);
> >>>        for (n = execlists_num_ports(&engine->execlists); --n; )
> >>>                elsp_write(0, engine->execlists.elsp);
> >>>    
> >>> @@ -906,7 +906,7 @@ static void execlists_submission_tasklet(unsigned long data)
> >>>                        GEM_TRACE("%s out[0]: ctx=%d.%d, seqno=%x\n",
> >>>                                  engine->name,
> >>>                                  port->context_id, count,
> >>> -                               rq->global_seqno);
> >>> +                               rq ? rq->global_seqno : 0);
> >>
> >> rq can be NULL here? What's with the --count below then, wouldn't it
> >> underflow since I assume rq == NULL means count == 0 as well?
> > 
> > Exactly. If rq is NULL then count must be 0. However, we want to show the
> > output from before the BUG_ON so that we have it available in case the
> > BUG does explode.
> 
> Okay makes sense.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks, pushed to see what explodes next. If we are right, rq is NULL
here, which should be impossible. Exciting times.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx