Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> >> >>>Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> >> >>>--- >> >>>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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
