[Intel-gfx] [PATCH v5 06/10] drm/i915/guc: Update GuC's log-buffer-state access for error capture.

[Teres Alexis, Alan Previn]

Looks like trying to move the vma up into guc-upper is impacting many other functions
in intel_guc_log and intel_guc_log_debugfs. I'll have to take it back (the level of
redesign i shall attempt with this series).

I'll just move the log_stats back into intel_guc_log and have intel_guc_capture
have its own stats structure - but keep the VMA allocation of this shared buffer in
intel_guc_log (like it was in the prior revs) and have intel_guc_capture "reach across"
into intel_guc_log to get the vma ptr (like it was in the prior revs).

...alan


On Mon, 2022-02-14 at 11:22 -0800, Alan Previn Teres Alexis wrote:
> Matt, just a final confirmation on below
> 
> > > > > > On Fri, 2022-02-04 at 10:19 -0800, Matthew Brost wrote:
> > > > > > > On Wed, Jan 26, 2022 at 02:48:18AM -0800, Alan Previn wrote:
> > > If the object lives at the GuC level, operate on it at the GuC level.
> > > 
> > > e.g.
> > > intel_guc_log_init_early calls mutex init on guc->log_state - that is
> > > wrong and breaks the layering. intel_guc_log_init_early should only
> > > operate on guc_log or below objects, not above it.
> > > 
> > > The key here is consisteny, if the GuC level owns the object it should
> > > be initialized there + passed into the lower levels if possible. The
> > > lower levels should avoid reaching back to GuC level for objects
> > > whenever possible.
> > > 
> > > You could have 2 intel_guc_log_stats objects below the guc_log object
> > > and 1 intel_guc_log_stats object for capture at the GuC level. That's
> > > likely the right approach here.
> > 
> > Thanks Matt - I'm in agreement... I was concerned about too much of
> > change - but you're right, I should be focusing on the design consistency.
> > Above sounds like the correct design (these stats and locks should belong
> > to their sole user).
> > 
> > ...alan
> > 
> 
> So this means:
> 1. guc[upper] allocates the shared-logging-buffer
>    - but would ask the lower level components for the sizes before
>      buffering-up or capping-down to match interface spec.
> 2. guc-log and guc-error-capture requests guc for a vmap at their init.
> 3. guc-log and guc-error-capture owns independent log-stats and
>    (and separate locks if needed).
> 4. when lower level components are done, they relinquish access to
>    their region by requesting guc[upper] to unmap and free
> 
> A super clean separation like above could mean ripping apart enums
> and other #defines to split them across guc_log and guc_error_capture
> headers (such as region sizes).
> 
> I believe that separation complicates the understanding of the fw interface
> for logging as we break that picture into independant files / components.
> For now I want to keep guc[upper] aware of the individual sub-region
> allocation requirements (no ripping apart of enums but moving them around)
> but only keep the requesting of vmap and independant log-region-stats
> within the lower level?
> 
> Are you okay with this?
> 
> side note: error-capture no longer need locks after the recent G2H triggered
> linked-list extraction redesign.
> 
>