[Intel-gfx] [PATCH] drm/i915: Engine relative MMIO

[John Harrison]

On 5/6/2019 14:36, Rodrigo Vivi wrote:
> On Tue, Apr 23, 2019 at 06:50:13PM -0700, John.C.Harrison at Intel.com wrote:
>> From: John Harrison <John.C.Harrison at Intel.com>
>>
>> With virtual engines, it is no longer possible to know which specific
>> physical engine a given request will be executed on at the time that
>> request is generated. This means that the request itself must be engine
>> agnostic - any direct register writes must be relative to the engine
>> and not absolute addresses.
>>
>> The LRI command has support for engine relative addressing. However,
>> the mechanism is not transparent to the driver. The scheme for Gen11
>> (MI_LRI_ADD_CS_MMIO_START) requires the LRI address to have no
>> absolute engine base component. The hardware then adds on the correct
>> engine offset at execution time.
>>
>> Due to the non-trivial and differing schemes on different hardware, it
>> is not possible to simply update the code that creates the LRI
>> commands to set a remap flag and let the hardware get on with it.
>> Instead, this patch adds function wrappers for generating the LRI
>> command itself and then for constructing the correct address to use
>> with the LRI.
>>
>> v2: Fix build break in GVT. Remove flags parameter [review feedback
>> from Chris W].
>>
>> v3: Fix build break in selftest. Rebase to newer base tree and fix
>> merge conflict.
>>
>> v4: More rebasing. Rmoved relative addressing support from Gen7-9 only
>> code paths [review feedback from Chris W].
> First of all, would you have a rebased version after gt/ ?
I have just done the rebase. Was planning to resend shortly. Although if 
there is more discussion about the best direction to take then I would 
rather hold off posting until a consensus is reached.


> So, from my point of view v3 was better than this because this spread
> the __MI_LOAD_REGISTER_IMM everywhere.
>
> Maybe I just disagree with Chris and I'd prefer a single place
> like v3, but anyway we could probably arrive in an intermediate
> solution like: Couldn't we do in a way that we keep the MI_LRI without
> '__' and use this new function only on the paths needed?
>
> and maybe name this function gen11_get_lri_cmd? to make it clear
> that gen11+ needs to use this path.

The intention was to make it clear that no new code should be directly 
writing MI_LRI. Everything should go through the helper function. Hence 
renaming to add the '__' to make it obvious. Otherwise, someone might 
use the old one by accident and we won't notice until some random and 
hard to track down failure related to virtual engines.

Not sure I would say that the __MI_LRI  is spreading 'everywhere'. There 
are only 8 instances versus double that of the get_lri_cmd version. Note 
also that it is not only Gen11+ specific paths. There are multiple 
places that are gen agnostic. So, unless you want to split those into 
pre/post Gen11 versions as well, you would end up with Gen7 calling a 
Gen11 labelled function.

John.