[Intel-gfx] [RFC 0/6] Start separating GuC and execlists submission

[Daniele Ceraolo Spurio]

On 1/28/20 4:49 PM, Matthew Brost wrote:
> On Fri, Jan 24, 2020 at 04:55:31PM -0800, Daniele Ceraolo Spurio wrote:
>> Note: this applies on top of my series to commit early to GuC [1].
>>
>> Picking up from the feedback from my RFC series about splitting
>> up intel_lrc.c [2], this series introduces GuC submission versions
>> of most of the engine and context functions. As a starting point,
>> the functions are still very similar to the execlists ones, but
>> they will progressively diverge while we add the new submission
>> logic. Some of the functions have been simplified by removing
>> support for pre-gen11 cases as we only aim to enable GuC submission
>> for gen11+; I've left comments and BUG_ONs to mark the reduces
>> support spots to easily find them in case we ever want to enable
>> GuC submission for older gens.
>>
>> Going slightly against the feedback from the previous series, I have
>> kept the basic context and ring object management shared between
>> execlists and GuC submission functions. The rationale is that those
>> objects are HW-related and therefore their creation and their main
>> attributes (e.g. context size) are not dependent on the submission
>> method in any way. Handling of more SW constructs, like the timeline,
>> has been duplicated.
>>
>> Still in theme of sharing, the flush and bb_start functions have also
>> been re-used on the GuC side, but I'm not sure if keeping them in
>> intel_lrc.c is the best solution. My medium-term plan is still the same
>> as [2], i.e. split execlists_submission.c out of intel_lrc.c, with the
>> latter holding the common code, but it might be worth moving the
>> command emission to a dedicated file or to a header as inlines, like
>> we already do in some cases.
>>
> 
> I can't say I'm thrilled about the amount of code duplicated in this 
> series.
> i.e. 95% of the breadcrumb code is exactly the same, request_alloc is a 
> copy and
> paste, same with resume, and same with the context operations. Surely 
> there is a
> way to share the code common between the GuC execlists? I like the idea of

I do understand the concerns as I started with the same idea you have 
(see my RFC for splitting intel_lrc.c), but after the received feedback 
and looking a bit more at the flows I've partially changed my mind, 
because not all that make sense to have in a common place today will 
stay the same going forward. e.g. we'll have to add a semaphore_signal 
to the breadcrumb_fini to signal completion to the GuC if we want the 
GuC to handle the dependencies for us, which will impact both the 
breadcrumb code itself and the size used in request_alloc.
This said, I do agree that there is certainly the opportunity to move 
more code to be common than what is done in this series, but IMO we need 
to wait for all the bits to be in place to have a clearer picture of 
what the full flow is going to look like. We did go through something 
similar, but on a bigger scale, when we duplicated everything from 
ringbuffer to execlists submission and then gradually merged it back 
together where possible.
Also, consider that some functions are back-end level, so IMO we need to 
compare them with their ringbuffer version as well.

Daniele

> putting all of this common code in a shared file and then
> intel_execlist_submission.c & intel_guc_submission.c call into the common
> functions while retaining there own set of unique function pointers.
> 
> Matt
> 
>> The code is still a bit rough and the series has been compile-tested
>> only. I wanted to get some early comments about the direction before
>> rebasing the rest of the GuC code on top of it and start testing.
>>
>> [1] https://patchwork.freedesktop.org/series/72031/
>> [2] https://patchwork.freedesktop.org/series/70787/
>>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: John Harrison <John.C.Harrison at Intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>>
>> Daniele Ceraolo Spurio (6):
>>  drm/i915/guc: Add guc-specific breadcrumb functions
>>  drm/i915/guc: Add request_alloc for guc_submission
>>  drm/i915/guc: Add engine->resume for GuC submission
>>  drm/i915/guc: Re-use lrc flush functions
>>  drm/i915/guc: Add initial context ops for GuC submission
>>  drm/i915/guc: Stop inheriting from execlists_set_default_submission
>>
>> drivers/gpu/drm/i915/gt/intel_lrc.c           | 217 +++++-----
>> drivers/gpu/drm/i915/gt/intel_lrc.h           |  17 +-
>> drivers/gpu/drm/i915/gt/selftest_lrc.c        |  12 +-
>> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 378 +++++++++++++++++-
>> 4 files changed, 496 insertions(+), 128 deletions(-)
>>
>> -- 
>> 2.24.1
>>