[Intel-gfx] [PATCH 2/2] drm/i915/guc: default to using GuC submission where possible

[Dave Gordon]

On 22/04/16 19:45, Chris Wilson wrote:
> On Fri, Apr 22, 2016 at 07:22:55PM +0100, Dave Gordon wrote:
>> This patch simply changes the default value of "enable_guc_submission"
>> from 0 (never) to -1 (auto). This means that GuC submission will be
>> used if the platform has a GuC, the GuC supports the request submission
>> protocol, and any required GuC firmwware was successfully loaded. If any
>> of these conditions are not met, the driver will fall back to using
>> execlist mode.
>
> There are several shortcomings yet, in particular you've decided to add
> new ABI...
>
> i915_guc_wq_check_space(): Do not return ETIMEDOUT. This function's
> return code is ABI.
>
> Your choices are either EAGAIN if you think the hardware will catch up, or
> more likely EIO and reset the hardware.

This layer doesn't have enough information to determine that we need a 
reset, so it'll have to be EAGAIN. TDR should catch the case where we 
continue to be unable to submit for an extended period, for whatever reason.

I don't like the spinwait anyway, so maybe we should just return the 
immediate result of sampling the WQ space once, and let the upper layers 
deal with how or whether to wait-and-retry.

> guc_add_workqueue_item: cannot fail
>
> It must be fixed such that it is a void function without possibility of
> failure.

The code clearly indicates that this "shouldn't happen", and yes, it 
would indicate an inconsistency in the internal state of the driver if 
the WARN-and-return-error path were taken.

However, the reason for including such a check is because we're crossing 
between logical submodules here. The GuC submission code requires that 
the LRC code (which is NOT GuC-specific) has previously called the 
precheck-for-space function and got the go-ahead signal. So this is 
essentially checking that the internal protocol has been followed 
correctly. If the LRC code were updated such that it might miss the 
precheck-for-space in some paths, this WARN-and-fail check would help 
identify the protocol error.

I can change it to a BUG() if you prefer!

> Not that you even successfully hooked up the failure paths in
> the current code.

The current code is broken at least because the whole callchain is 
declared as returning an int error code, only to find that the topmost 
level says that add_request/emit_request are not allowed to fail!

It would be rather better if the top level handled all errors in 
submission, even if it were only by dropping the request and resetting 
the engine!

> Same for guc_ring_doorbell.

The rationale for an error return is slightly different here, as this 
would not indicate an internal logic error in the driver, but rather a 
failure of communication between the driver and the doorbell hardware 
(or possibly the GuC firmware, but I don't think a GuC bug could cause 
more than one failure-to-update, hence the retry count is only 2).

If the doorbell hardware is not working correctly, there is nothing we 
can do about it here; the only option is to report the failure up to 
some layer that can recover, probably by resetting the GPU.

> And what exactly is that atomic64_cmpxchg() serialising with? There are
> no other CPUs contending with the write, and neither does the GuC (and I
> doubt it is taking any notice of the lock cmpxchg). Using cmpxchg where
> a single WRITE_ONCE() of a 32bit value wins the perf prize for hotest
> instruction and function in the kernel.
> -Chris

The doorbell controller hardware, I should think. The BSpec describes 
using LOCK_CMPXCHG8B to update doorbells, so I think this code is just 
based on what it says there. If the CPU hardware doesn't implement it 
efficiently, surely the GPU h/w designers wouldn't have mandated it in 
this way?

Maybe Alex or Tom know more about the CPU<->doorbell<->GuC signalling.

.Dave.