Sounds good - thanks. (ignore the "doing the opposite" comment). Reviewed-by: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx> On Mon, 2022-08-08 at 11:43 -0700, Harrison, John C wrote: > On 8/4/2022 17:40, Teres Alexis, Alan Previn wrote: > > I have a question on below code. Everything else looked good. > > Will r-b as soon as we can close on below question > > ...alan > > > > > > On Wed, 2022-07-27 at 19:20 -0700, John.C.Harrison@xxxxxxxxx wrote: > > > From: John Harrison <John.C.Harrison@xxxxxxxxx> > > > > > > It is useful to be able to match GuC events to kernel events when > > > looking at the GuC log. That requires being able to convert GuC > > > timestamps to kernel time. So, when dumping error captures and/or GuC > > > logs, include a stamp in both time zones plus the clock frequency. > > > > > > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx> > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > > @@ -1675,6 +1678,13 @@ gt_record_uc(struct intel_gt_coredump *gt, > > > */ > > > error_uc->guc_fw.path = kstrdup(uc->guc.fw.path, ALLOW_FAIL); > > > error_uc->huc_fw.path = kstrdup(uc->huc.fw.path, ALLOW_FAIL); > > > + > > > + /* > > > + * Save the GuC log and include a timestamp reference for converting the > > > + * log times to system times (in conjunction with the error->boottime and > > > + * gt->clock_frequency fields saved elsewhere). > > > + */ > > > + error_uc->timestamp = intel_uncore_read(gt->_gt->uncore, GUCPMTIMESTAMP); > > Alan:this register is in the GUC-SHIM domain and so unless i am mistaken u might need to ensure we hold a wakeref so > > that are getting a live value of the real timestamp register that this register is mirror-ing and not a stale snapshot. > > Or was this already done farther up the stack? Or are we doing the opposite - in which case we should ensure we drop al > > wakeref prior to this point. (which i am not sure is a reliable method since we wouldnt know what GuC ref was at). > The intel_uncore_read() does a forcewake acquire implicitly. > > Not sure what you mean about dropping all wakerefs prior to this point? > > John. > > > > error_uc->guc_log = create_vma_coredump(gt->_gt, uc->guc.log.vma, > > > "GuC log buffer", compress); > > >
