[Intel-gfx] [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 at chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at 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