[Intel-gfx] [PATCH 3/5] drm/i915/guc: init engine directly in GuC submission mode

[Daniele Ceraolo Spurio]

On 1/5/2021 3:33 PM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2021-01-05 23:19:45)
>> Instead of starting the engine in execlists submission mode and then
>> switching to GuC, start directly in GuC submission mode. The initial
>> setup functions have been copied over from the execlists code
>> and simplified by removing the execlists submission-specific parts.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> Cc: John Harrison <john.c.harrison at intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c     |   5 +-
>>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 249 +++++++++++++++++-
>>   .../gpu/drm/i915/gt/uc/intel_guc_submission.h |   1 +
>>   3 files changed, 244 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> index f62303bf80b8..6b4483b72c3f 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> @@ -40,6 +40,7 @@
>>   #include "intel_lrc_reg.h"
>>   #include "intel_reset.h"
>>   #include "intel_ring.h"
>> +#include "uc/intel_guc_submission.h"
>>   
>>   /* Haswell does have the CXT_SIZE register however it does not appear to be
>>    * valid. Now, docs explain in dwords what is in the context object. The full
>> @@ -907,7 +908,9 @@ int intel_engines_init(struct intel_gt *gt)
>>          enum intel_engine_id id;
>>          int err;
>>   
>> -       if (HAS_EXECLISTS(gt->i915))
>> +       if (intel_uc_uses_guc_submission(&gt->uc))
> When do we determine intel_uc_uses_guc_submission?

at firmware fetch time.

>
> I'm a bit nervous since I've lost track of needs/wants/uses. Is there a
> telltale we can place here to assert that we've processed the relevant
> init functions (also acting as an aide memoire)?

There is already a GEM_BUG_ON for this inside the function, it'll 
trigger if we call it too early.
For the submission side of things, there isn't much difference at the 
moment between "wants" and "uses" since we do wedge if GuC submission is 
selected and the FW is not found. I still prefer to use "uses" where 
possible in case we want to change this in the future.

>
>> +               setup = intel_guc_submission_setup;
>> +       else if (HAS_EXECLISTS(gt->i915))
>>                  setup = intel_execlists_submission_setup;
>>          else
>>                  setup = intel_ring_submission_setup;
>> +static bool unexpected_starting_state(struct intel_engine_cs *engine)
>> +{
>> +       bool unexpected = false;
>> +
>> +       if (ENGINE_READ_FW(engine, RING_MI_MODE) & STOP_RING) {
>> +               drm_dbg(&engine->i915->drm,
>> +                       "STOP_RING still set in RING_MI_MODE\n");
>> +               unexpected = true;
>> +       }
> Do we care? Is this something we can assume the guc will handle?
> (It originated in debugging reset failures.)

GuC handles it post engine reset, but not on init and resume. If you 
think this only makes sense for reset debug then I'll get rid of it.

Thanks,
Daniele

>
>> +       return unexpected;
>> +}