[Intel-gfx] [PATCH 03/23] drm/i915: Remove lrc default desc from GEM context

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-08-01 16:29:53)
> 
> On 01/08/2019 12:13, Chris Wilson wrote:
> > Quoting Chris Wilson (2019-08-01 11:57:06)
> >> Quoting Tvrtko Ursulin (2019-08-01 09:53:15)
> >>> We could store it in ce then. We already have well defined control
> >>> points for when vm changes when all are updated.
> >>
> >> We are storing it in ce; it's not like we recompute it all that often,
> >> and when we do it's because we have rebound the vma.
> >>
> >>> If done like this then it looks like assigning ctx->hw_id could also do
> >>> the default_desc update, so that we can avoid even more work done at pin
> >>> time.
> >>
> >> What ctx->hw_id? You are imagining things again :-p
> >>
> >> Remember that we only do this on first pin from idle, not every pin.
> > 
> > Fwiw, I quickly looked at only doing it if the vma is rebound, but
> > that's move branches just to save a couple. The low frequency at which
> > we have to actually compute this (walk a few more branches inside an
> > already branchy contxt_pin) doesn't seem to justify the extra storage for
> > me. It's not like we are recomputing lrc_desc on every submit as it once
> > was.
> 
> On every submit if last request got retired in the meantime, no, for 
> instance bursty loads? Yeah it is very inconsequential but at some point 
> we made an effort to cache as much as possible what is invariant so it 
> saddens me a bit to remove that.

Once we have hw_id out of the way, we only need to set the bottom 32b
here.
 
> For instance Icelake engine dependent stuff sneaked into 
> intel_lrc.c/lrc_desriptors at some point, which is also against the 
> spirit of caching. If we were to move the cached value in ce then we 
> would be able to remove that and have it once again minimal in there.

Well we can set all bits but hw_id/lrca at init time. How about if I run
that past you?

> Not only just minimal, but not separated in two separate places. I guess 
> this patch improves things in that respect - consolidates the lrc_desc 
> computation once again.
> 
> I did not get the part about VMA re-binding. I did not suggest to move 
> the lrca offset into cache as well. I was just thinking about the gen, 
> engine and vm dependent bits could naturally go into 
> i915_gem_context.c/default_desc_template. Just need to take (engine, 
> hw_id, vm).

I'm just thinking about the bit that changes inside ce->lrc_desc.
 
> And virtual engine would have to re-compute it when moving engines. Hm.. 
> we don't seem to do that? Only when pinning we set it up based on 
> sibling[0] so how it all works? We don't re-pin when moving engine I 
> thought.

No. We don't. Whoops. Good job clearly nothing uses that then.
-Chris