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
>
