Re: [Intel-gfx] [RFC 5/7] drm/i915/guc: Update GuC's log-buffer-state access for error capture.

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Dec 07, 2021 at 03:33:00PM -0800, Teres Alexis, Alan Previn wrote:
> Thank you for the detailed review Matt. Responses and follow up questions on some of them below (wanna
> make sure i dont misunderstand).
> 
> Will fix all the rest - glad we dont have any design problems .. so far :)
> 
> ...alan
> 
> On Tue, 2021-12-07 at 14:31 -0800, Matthew Brost wrote:
> > On Mon, Nov 22, 2021 at 03:04:00PM -0800, Alan Previn wrote:
> > > -static bool guc_check_log_buf_overflow(struct intel_guc_log *log,
> > > -                                  enum guc_log_buffer_type type,
> > > -                                  unsigned int full_cnt)
> > > +bool guc_check_log_buf_overflow(struct intel_guc *guc,
> > > +                           struct intel_guc_log_stats *log_state,
> > > +                           unsigned int full_cnt)
> >
> > I don't think you meant to drop the 'static' here.
> actually i do need to call it from guc_capture - but that was on the next patch.
> my action would be to move this change to the next patch and i guess change the name to "intel_guc..."?
> (im assuming we dont wanna duplicate this).
> 

Ok. Yes, if you export a function add the intel_ prefix.

> >
> > > +
> > > +   guc_log_size = PAGE_SIZE + DEBUG_BUFFER_SIZE + CRASH_BUFFER_SIZE + CAPTURE_BUFFER_SIZE;
> >
> > I'd personally keep the original formatting here.
> >
> Question: - You are refering to just that last line of the "guc_log_size = .." above right?
> 

Yes.

> > >   struct intel_guc_log {
> > >     u32 level;
> > >     struct i915_vma *vma;
> > > +   void *buf_addr;
> >
> > I don't think you need both 'buf_addr' and 'relay.buf_addr' as they are
> > the same value, right?
> >
> Matt
> >
> Clarification: In the baseline code, i believe we use the "relay.buf_addr" like an enable flag
> way to verify that the guc relay debugfs was invoked by user space and is currently active.
> (which can be disabled. That said I can definitely remove that relay.buf_addr by introducing
> a more descriptive flag such as "relay.active". Assume this is ok right?
>

Sounds good to me.

Matt
 
>