[Intel-gfx] [PATCH 5/5] drm/i915/gt: Split logical ring context manipulation into intel_lrc.c

[Daniele Ceraolo Spurio]

On 1/28/20 2:08 PM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2020-01-28 21:55:02)
>>
>>
>> On 1/28/20 12:43 PM, Chris Wilson wrote:
>>> Extract the core functions for setting up the machine dependent logical
>>> ring context to intel_lrc.c. This will be shared by all submission
>>> backends that utilise logical ring contexts.
>>>
>>
>> I still don't understand why you don't want to put the lrc and ring
>> object creation and initial setup (populate_lr_context) somewhere
> 
> The initial setup is common in intel_lrc.c, the rest is really just
> mapping the local context layout, which will vary depending on the
> different abstractions for the backends.
> 
>> common. I see no way we could do that differently for GuC submission,
>> since the HW obviously requires the same objects and IMO it makes sense
>> to keep the same initialization flow, so we'd have to copy that logic
>> and maintain 2 identical copies of it. The redzone is the only thing
>> that is not hw-enforced, but I believe it makes sense to keep that on
>> the GuC side as well to catch possible issues. Or am I missing something?
> 
> They don't have the same execution flow, and I expect the differences in
> context bookkeeping will continue to grow. Certainly there's an
> abstraction around virtual engines to come, because the current struct
> is heavily tied into the backend.

Yes, virtual engines are definitely going to look a lot different with 
GuC submission since the GuC is going to do all the balancing work, but 
the objects involved are still going to be the same. To give you a rough 
idea, from our POV there is not going to be any extra work needed for a 
virtual context compared to a normal one, with the exception of setting 
a mask within a GuC structure to specify which engines the context is 
allowed to run on.

Anyway, I understand that without all the GuC code in place it isn't 
easy to see exactly how this is going to pan out, so I'm ok with 
duplicating that part as well for now and we can re-unify later if we 
think it makes sense. I'll review the patch in a bit more detail as I 
only skimmed through it earlier and spot-checked the functions I was 
more interested in.

Daniele

> 
> populate_lr_context is one that might be shared, but I just didn't see
> much point in sharing a couple of very minor routines, when the
> common mechanism is available. I'm still waiting for the other shoe to
> fall and wide-contexts to become a necessity again.
> -Chris
>