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).
>
> > +
> > + 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?
> > 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?
