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. > >
