Re: [Intel-gfx] [PATCH 1/1] drm/i915/guc: Delay disabling guc_id scheduling for better hysteresis

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Fri, 2022-09-16 at 08:36 -0700, Ceraolo Spurio, Daniele wrote:
> 
> On 9/16/2022 1:58 AM, Tvrtko Ursulin wrote:
> > On 16/09/2022 08:53, Teres Alexis, Alan Previn wrote:
> > > On Thu, 2022-09-15 at 09:48 +0100, Tvrtko Ursulin wrote:
> > > > On 15/09/2022 03:12, Alan Previn wrote:
> > > > > 
> > > > 
alan: [snip]

> > > > IMO it creates less readable code for the benefit of not repeating
> > > > with_intel_runtime_pm -> __guc_context_sched_disable two times. Dunno..
> > > > it's ugly but I have no suggestions. Hm does it have to send using the
> > > > busy loop? It couldn't just queue the request and then wait for 
> > > > reply if
> > > > disable message was emitted?
> > > > 
> > > I agree that the above code lacks readability - will see if i can 
> > > break it down to smaller
> > > functions with cleaner in-function lock/unlock pairs. I agree that a 
> > > little code duplication
> > > is better than less readable code. It was inherited code i didn't 
> > > want to modify but I'll
> > > go ahead and refactor this.
> > > 
> > > On the busy loop - im assuming you are refering to the actual ct 
> > > sending. I'll consult my
> > > team if i am missing anything more but based on comments, I believe 
> > > callers must use that
> > > function to guarantee reservation of space in the G2H CTB to always 
> > > have space to capture
> > > responses for actions that MUST be acknowledged from GuC 
> > > (acknowledged by either replying
> > > with a success or failure). This is necessary for coherent guc-id 
> > > state machine (because the
> > > GuC firmware will drop requests for guc-id's that are not registered 
> > > or not in a
> > > 'sched-enabled' state).
> 
> > Maybe to explain what I meant a bit better. I thought that the reason 
> > for strange unlock pattern is the with_runtime_pm which needs to 
> > happen for the CT send/receive loop. What I meant was would it be 
> > possible to do it like this:
> > 
> > state lock
> > ...
> > sent = queue_msg_2_guc (this would be i915 only, no runtime pm needed)
> > ...
> > state unlock
> > 
> > if (sent)
> >   with_runtime_pm:
> >      send/receive queued guc messages (only this would talk to guc)
> > 
> > But I have truly no idea if the CT messaging infrastructure supports 
> > something like this.
> > 
> > Anyway, see what it is possible and if it is not or too hard for now 
> > leave it be.
> > 
alan: I consulted with my team mates on above and they said that discussion has happened
in the past and the CT infrastructure currently does not support that but the benefit
comes into question because such an undertaking would be an expensive redesign that
has wider impact (changes across all callers). I guess for now i will leave above code
as it is as this would be a whole separate feature change on its own.