[PATCH 1/2] drm/i915: Assert that the kernel_context is hw-id 0

[PATCH 1/2] drm/i915: Assert that the kernel_context is hw-id 0

[Chris Wilson]

For easy recognisability, we want the kernel context to have id 0 and
all user contexts to have non-zero ids.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 17f90c618208..77458da9627d 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -451,6 +451,11 @@ int i915_gem_context_init(struct drm_i915_private *dev_priv)
 		return PTR_ERR(ctx);
 	}
 
+	/* For easy recognisablity, we want the kernel context to be 0 and then
+	 * all user contexts will have non-zero hw_id.
+	 */
+	GEM_BUG_ON(ctx->hw_id);
+
 	i915_gem_context_clear_bannable(ctx);
 	ctx->priority = I915_PRIORITY_MIN; /* lowest priority; idle task */
 	dev_priv->kernel_context = ctx;
-- 
2.11.0

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

[PATCH 2/2] drm/i915: Assert that the context-switch completion matches our context

[Chris Wilson]

When execlists signals the context completion, it also provides the
context id for the status event. Assert that id matches the one we expect.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 432ee495dec2..6aa8761c333a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -595,6 +595,10 @@ static void intel_lrc_irq_handler(unsigned long data)
 			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
 				continue;
 
+			/* Check the context id for this event matches */
+			GEM_BUG_ON(readl(buf + 2 * idx + 1) !=
+				   port[0].request->ctx->hw_id);
+
 			GEM_BUG_ON(port[0].count == 0);
 			if (--port[0].count == 0) {
 				GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
-- 
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 series starting with [1/2] drm/i915: Assert that the kernel_context is hw-id 0

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Assert that the kernel_context is hw-id 0
URL   : https://patchwork.freedesktop.org/series/18353/
State : success

== Summary ==

Series 18353v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/18353/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                dmesg-warn -> PASS       (fi-snb-2520m)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:79   pass:66   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

f62d6104306f3ec835f3776434a70ca9bb6f820e drm-tip: 2017y-01m-21d-11h-28m-52s UTC integration manifest
c8088b8 drm/i915: Assert that the context-switch completion matches our context
f07385c drm/i915: Assert that the kernel_context is hw-id 0

== Logs ==

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

Re: [PATCH 1/2] drm/i915: Assert that the kernel_context is hw-id 0

[Joonas Lahtinen]

On la, 2017-01-21 at 17:29 +0000, Chris Wilson wrote:
> For easy recognisability, we want the kernel context to have id 0 and
> all user contexts to have non-zero ids.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Assert that the context-switch completion matches our context

[Joonas Lahtinen]

On la, 2017-01-21 at 17:29 +0000, Chris Wilson wrote:
> When execlists signals the context completion, it also provides the
> context id for the status event. Assert that id matches the one we expect.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> @@ -595,6 +595,10 @@ static void intel_lrc_irq_handler(unsigned long data)
>  			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
>  				continue;
>  
> +			/* Check the context id for this event matches */
> +			GEM_BUG_ON(readl(buf + 2 * idx + 1) !=
> +				   port[0].request->ctx->hw_id);
> +
>  			GEM_BUG_ON(port[0].count == 0);
>  			if (--port[0].count == 0) {
>  				GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);

This is good for now, Mika has to update it for the SVM code, though.

(Counterpart at intel_lr_context_descriptor_update).

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Assert that the context-switch completion matches our context

[Chris Wilson]

On Mon, Jan 23, 2017 at 10:26:00AM +0200, Joonas Lahtinen wrote:
> On la, 2017-01-21 at 17:29 +0000, Chris Wilson wrote:
> > When execlists signals the context completion, it also provides the
> > context id for the status event. Assert that id matches the one we expect.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > @@ -595,6 +595,10 @@ static void intel_lrc_irq_handler(unsigned long data)
> >  			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
> >  				continue;
> >  
> > +			/* Check the context id for this event matches */
> > +			GEM_BUG_ON(readl(buf + 2 * idx + 1) !=
> > +				   port[0].request->ctx->hw_id);
> > +
> >  			GEM_BUG_ON(port[0].count == 0);
> >  			if (--port[0].count == 0) {
> >  				GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
> 
> This is good for now, Mika has to update it for the SVM code, though.
> 
> (Counterpart at intel_lr_context_descriptor_update).

One suggestion you made was that hw_id might carry the pasid in its
upper 12bits (matching lrc_desc construction). Or an alternative is that
we use upper_32_bits(port[0]->request->ctx->engine[engine->id].lrc_desc)
here. That's probably a better description if that is what the hw is
doing - which I can't remember, I have vague memories of it being a
direct copy of the high dword, but the description of it I read over the
weekend just referred to it as context_id.

So does the context status carry the upper dword of elsp or just the
context id? Easy way to find out -- today everything say use group 1!
-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

[PATCH v2] drm/i915: Assert that the context-switch completion matches our context

[Chris Wilson]

When execlists signals the context completion, it also provides the
context id for the status event. Assert that id matches the one we expect.

v2: The upper dword of the context status is a duplicate of the upper
dword from elsp submission (i.e. includes the group id as well as the
context id). Include this check as well.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 432ee495dec2..963b1888d8a0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -595,6 +595,12 @@ static void intel_lrc_irq_handler(unsigned long data)
 			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
 				continue;
 
+			/* Check the context/desc id for this event matches */
+			GEM_BUG_ON(((readl(buf + 2 * idx + 1) & (MAX_CONTEXT_HW_ID-1)) !=
+				    port[0].request->ctx->hw_id));
+			GEM_BUG_ON(readl(buf + 2 * idx + 1) !=
+				   upper_32_bits(port[0].request->ctx->engine[engine->id].lrc_desc));
+
 			GEM_BUG_ON(port[0].count == 0);
 			if (--port[0].count == 0) {
 				GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
-- 
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 series starting with [1/2] drm/i915: Assert that the kernel_context is hw-id 0 (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Assert that the kernel_context is hw-id 0 (rev2)
URL   : https://patchwork.freedesktop.org/series/18353/
State : success

== Summary ==

Series 18353v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/18353/revisions/2/mbox/


fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:79   pass:66   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

3599f990bcb8b72f9ae062a97140439ab9df22b7 drm-tip: 2017y-01m-23d-09h-16m-39s UTC integration manifest
27b6c07 drm/i915: Assert that the context-switch completion matches our context
af83c01 drm/i915: Assert that the kernel_context is hw-id 0

== Logs ==

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

Re: [PATCH v2] drm/i915: Assert that the context-switch completion matches our context

[Tvrtko Ursulin]

On 23/01/2017 08:50, Chris Wilson wrote:
> When execlists signals the context completion, it also provides the
> context id for the status event. Assert that id matches the one we expect.
>
> v2: The upper dword of the context status is a duplicate of the upper
> dword from elsp submission (i.e. includes the group id as well as the
> context id). Include this check as well.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 432ee495dec2..963b1888d8a0 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -595,6 +595,12 @@ static void intel_lrc_irq_handler(unsigned long data)
>  			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
>  				continue;
>
> +			/* Check the context/desc id for this event matches */
> +			GEM_BUG_ON(((readl(buf + 2 * idx + 1) & (MAX_CONTEXT_HW_ID-1)) !=
> +				    port[0].request->ctx->hw_id));
> +			GEM_BUG_ON(readl(buf + 2 * idx + 1) !=
> +				   upper_32_bits(port[0].request->ctx->engine[engine->id].lrc_desc));
> +

Not sure that you need the first line since the lrc_desc includes the 
hw_id? Or perhaps you don't need the second one since lrc_desc is built 
from hw_id? :)

Regards,

Tvrtko


>  			GEM_BUG_ON(port[0].count == 0);
>  			if (--port[0].count == 0) {
>  				GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Assert that the context-switch completion matches our context

[Chris Wilson]

On Mon, Jan 23, 2017 at 10:26:15AM +0000, Tvrtko Ursulin wrote:
> 
> On 23/01/2017 08:50, Chris Wilson wrote:
> >When execlists signals the context completion, it also provides the
> >context id for the status event. Assert that id matches the one we expect.
> >
> >v2: The upper dword of the context status is a duplicate of the upper
> >dword from elsp submission (i.e. includes the group id as well as the
> >context id). Include this check as well.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/intel_lrc.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index 432ee495dec2..963b1888d8a0 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -595,6 +595,12 @@ static void intel_lrc_irq_handler(unsigned long data)
> > 			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
> > 				continue;
> >
> >+			/* Check the context/desc id for this event matches */
> >+			GEM_BUG_ON(((readl(buf + 2 * idx + 1) & (MAX_CONTEXT_HW_ID-1)) !=
> >+				    port[0].request->ctx->hw_id));
> >+			GEM_BUG_ON(readl(buf + 2 * idx + 1) !=
> >+				   upper_32_bits(port[0].request->ctx->engine[engine->id].lrc_desc));
> >+
> 
> Not sure that you need the first line since the lrc_desc includes
> the hw_id? Or perhaps you don't need the second one since lrc_desc
> is built from hw_id? :)

It was defense in depth :)

The first line is certain, that make sure the event matches
port[0].request.

The second line makes sure that everything matches my understanding, and
that lrc_desc is correct (or vice versa).
-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 v2] drm/i915: Assert that the context-switch completion matches our context

[Tvrtko Ursulin]

On 23/01/2017 10:30, Chris Wilson wrote:
> On Mon, Jan 23, 2017 at 10:26:15AM +0000, Tvrtko Ursulin wrote:
>>
>> On 23/01/2017 08:50, Chris Wilson wrote:
>>> When execlists signals the context completion, it also provides the
>>> context id for the status event. Assert that id matches the one we expect.
>>>
>>> v2: The upper dword of the context status is a duplicate of the upper
>>> dword from elsp submission (i.e. includes the group id as well as the
>>> context id). Include this check as well.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_lrc.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 432ee495dec2..963b1888d8a0 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -595,6 +595,12 @@ static void intel_lrc_irq_handler(unsigned long data)
>>> 			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
>>> 				continue;
>>>
>>> +			/* Check the context/desc id for this event matches */
>>> +			GEM_BUG_ON(((readl(buf + 2 * idx + 1) & (MAX_CONTEXT_HW_ID-1)) !=
>>> +				    port[0].request->ctx->hw_id));
>>> +			GEM_BUG_ON(readl(buf + 2 * idx + 1) !=
>>> +				   upper_32_bits(port[0].request->ctx->engine[engine->id].lrc_desc));
>>> +
>>
>> Not sure that you need the first line since the lrc_desc includes
>> the hw_id? Or perhaps you don't need the second one since lrc_desc
>> is built from hw_id? :)
>
> It was defense in depth :)
>
> The first line is certain, that make sure the event matches
> port[0].request.
>
> The second line makes sure that everything matches my understanding, and
> that lrc_desc is correct (or vice versa).

Too redundant IMO. lrc_desc correctness could/should be asserted 
somewhere else, not in the irq handler. I would keep one of them, 
probably the assert agains lrc_desc because that's the right layer for 
this place.

Regards,

Tvrtko

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

Re: [PATCH v2] drm/i915: Assert that the context-switch completion matches our context

[Chris Wilson]

On Mon, Jan 23, 2017 at 10:44:03AM +0000, Tvrtko Ursulin wrote:
> 
> On 23/01/2017 10:30, Chris Wilson wrote:
> >On Mon, Jan 23, 2017 at 10:26:15AM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 23/01/2017 08:50, Chris Wilson wrote:
> >>>When execlists signals the context completion, it also provides the
> >>>context id for the status event. Assert that id matches the one we expect.
> >>>
> >>>v2: The upper dword of the context status is a duplicate of the upper
> >>>dword from elsp submission (i.e. includes the group id as well as the
> >>>context id). Include this check as well.
> >>>
> >>>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >>>---
> >>>drivers/gpu/drm/i915/intel_lrc.c | 6 ++++++
> >>>1 file changed, 6 insertions(+)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>>index 432ee495dec2..963b1888d8a0 100644
> >>>--- a/drivers/gpu/drm/i915/intel_lrc.c
> >>>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>>@@ -595,6 +595,12 @@ static void intel_lrc_irq_handler(unsigned long data)
> >>>			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
> >>>				continue;
> >>>
> >>>+			/* Check the context/desc id for this event matches */
> >>>+			GEM_BUG_ON(((readl(buf + 2 * idx + 1) & (MAX_CONTEXT_HW_ID-1)) !=
> >>>+				    port[0].request->ctx->hw_id));
> >>>+			GEM_BUG_ON(readl(buf + 2 * idx + 1) !=
> >>>+				   upper_32_bits(port[0].request->ctx->engine[engine->id].lrc_desc));
> >>>+
> >>
> >>Not sure that you need the first line since the lrc_desc includes
> >>the hw_id? Or perhaps you don't need the second one since lrc_desc
> >>is built from hw_id? :)
> >
> >It was defense in depth :)
> >
> >The first line is certain, that make sure the event matches
> >port[0].request.
> >
> >The second line makes sure that everything matches my understanding, and
> >that lrc_desc is correct (or vice versa).
> 
> Too redundant IMO. lrc_desc correctness could/should be asserted
> somewhere else, not in the irq handler. I would keep one of them,
> probably the assert agains lrc_desc because that's the right layer
> for this place.

Preemptive r-b on the second then? Joonas?
-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 v2] drm/i915: Assert that the context-switch completion matches our context

[Mika Kuoppala]

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

> On Mon, Jan 23, 2017 at 10:44:03AM +0000, Tvrtko Ursulin wrote:
>> 
>> On 23/01/2017 10:30, Chris Wilson wrote:
>> >On Mon, Jan 23, 2017 at 10:26:15AM +0000, Tvrtko Ursulin wrote:
>> >>
>> >>On 23/01/2017 08:50, Chris Wilson wrote:
>> >>>When execlists signals the context completion, it also provides the
>> >>>context id for the status event. Assert that id matches the one we expect.
>> >>>
>> >>>v2: The upper dword of the context status is a duplicate of the upper
>> >>>dword from elsp submission (i.e. includes the group id as well as the
>> >>>context id). Include this check as well.
>> >>>
>> >>>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >>>Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> >>>---
>> >>>drivers/gpu/drm/i915/intel_lrc.c | 6 ++++++
>> >>>1 file changed, 6 insertions(+)
>> >>>
>> >>>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> >>>index 432ee495dec2..963b1888d8a0 100644
>> >>>--- a/drivers/gpu/drm/i915/intel_lrc.c
>> >>>+++ b/drivers/gpu/drm/i915/intel_lrc.c
>> >>>@@ -595,6 +595,12 @@ static void intel_lrc_irq_handler(unsigned long data)
>> >>>			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
>> >>>				continue;
>> >>>
>> >>>+			/* Check the context/desc id for this event matches */
>> >>>+			GEM_BUG_ON(((readl(buf + 2 * idx + 1) & (MAX_CONTEXT_HW_ID-1)) !=
>> >>>+				    port[0].request->ctx->hw_id));
>> >>>+			GEM_BUG_ON(readl(buf + 2 * idx + 1) !=
>> >>>+				   upper_32_bits(port[0].request->ctx->engine[engine->id].lrc_desc));
>> >>>+
>> >>
>> >>Not sure that you need the first line since the lrc_desc includes
>> >>the hw_id? Or perhaps you don't need the second one since lrc_desc
>> >>is built from hw_id? :)
>> >
>> >It was defense in depth :)
>> >
>> >The first line is certain, that make sure the event matches
>> >port[0].request.
>> >
>> >The second line makes sure that everything matches my understanding, and
>> >that lrc_desc is correct (or vice versa).
>> 
>> Too redundant IMO. lrc_desc correctness could/should be asserted
>> somewhere else, not in the irq handler. I would keep one of them,
>> probably the assert agains lrc_desc because that's the right layer
>> for this place.
>
> Preemptive r-b on the second then? Joonas?

I feel like coming late into party uninvited, but
you can add mine for the whole lrc_desc check.

-Mika

> -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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx