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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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