On Mon, Aug 09, 2021 at 06:20:51PM +0000, Matthew Brost wrote: > On Mon, Aug 09, 2021 at 04:27:01PM +0200, Daniel Vetter wrote: > > On Tue, Aug 03, 2021 at 03:29:08PM -0700, Matthew Brost wrote: > > > Calling switch_to_kernel_context isn't needed if the engine PM reference > > > is taken while all contexts are pinned. By not calling > > > switch_to_kernel_context we save on issuing a request to the engine. > > > > > > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/gt/intel_engine_pm.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c > > > index 1f07ac4e0672..58099de6bf07 100644 > > > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c > > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c > > > @@ -162,6 +162,10 @@ static bool switch_to_kernel_context(struct intel_engine_cs *engine) > > > unsigned long flags; > > > bool result = true; > > > > > > + /* No need to switch_to_kernel_context if GuC submission */ > > > > Maybe whack a big FIXME on here that we should unravel this properly. > > Sure, can add a FIXME here. > > > Currently the execlist backend assumptions are leaked all over the place, > > leading to stuff like this. Which means extremely fragile code. > > > > Yes, this something required for execlists implemented in what should be > generic code. > > > I currently don't have a great idea on how exactly we should do that, but > > oh well. > > Me either, it will be a process. > > > > > btw just in case we ever want to make guc lrc properly evictable (which as > > the og use-case for this function, way, way back), would we need to fully > > Can you explain what you mean by fully evictable? Not getting what you > mean in this context. > > > unregister them from guc? At least I'm assuming there's no other trick > > If scheduling is disabled on the context (currently done on unpin) you are > free move anything around as the GuC is guaranteed not to touch the > context state. If on re-pin something has moved (e.g. the LRC vaddr is > different), you need to unregister and re-register the context with the > GuC. So at that point GuC also guarantees that it's not left in the hw engine? Execlist has this barrier request to fully unload the ctx from the hw, and that's also why I cam on the topic of OA. > > like the below one. > > > > Another aside: How does the perf/OA patching work on GuC? > > > > Not my area of expertise but perf somewhat a WIP. The plan is for the > GuC to write out some stats to HWSP I think? John Harrison is working to > get this fully implemented. > > OA is working afaik, with Umesh Nerlige Ramappa being the expert here. I think it's OA that I'm thinking of here: We have code in i915_perf.c to patch all the ctx currently in the system, so that they have a consistent OA config. That's also relying on this barrier stuff, and I was wondering how that will work with GuC. -Daniel > > Matt > > > Anyway, patch looks legit: > > > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > > > > > > + if (intel_engine_uses_guc(engine)) > > > + return true; > > > + > > > /* GPU is pointing to the void, as good as in the kernel context. */ > > > if (intel_gt_is_wedged(engine->gt)) > > > return true; > > > -- > > > 2.28.0 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
