Re: [PATCH] drm/i915: Engine relative MMIO

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, May 07, 2019 at 11:55:11AM -0700, John Harrison wrote:
> On 5/6/2019 14:36, Rodrigo Vivi wrote:
> > On Tue, Apr 23, 2019 at 06:50:13PM -0700, John.C.Harrison@Intel.com wrote:
> > > From: John Harrison <John.C.Harrison@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.

makes sense. Although I prefer the use of v3 with __MI_LRI only used inside
the function, it seems I'm the only one... let's move with v4 then...


> 
> John.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx