It's been a while - trying to resurrect this now.
On Tue, 2022-07-19 at 20:40 -0700, Dixit, Ashutosh wrote:
> On Mon, 09 May 2022 14:01:49 -0700, Alan Previn wrote:
> >
> >
Alan: [snip]
> > +#define GUC_LOG_RELAY_SUBBUF_COUNT 8
> > +u32 intel_guc_log_relay_subbuf_count(struct intel_guc_log *log)
> > +{
> > + return GUC_LOG_RELAY_SUBBUF_COUNT;
> > +}
>
> uapi wise, instead of exposing guc_log_size and subbuf_count, why not
> expose subbuf_size and subbuf_count?
To combine addressing your request + consistency with existing knobs (all of which are dedicated for guc-relay-logging),
I'll go ahead and change it to guc_log_relay_subbuf_size_get and guc_log_relay_subbuf_count_get.
>
> > +static int guc_log_relay_buf_size_get(void *data, u64 *val)
> > +{
> > + struct intel_guc_log *log = data;
> > +
> > + if (!log)
> > + return -ENODEV;
> > + if (!log->vma)
> > + return -ENODEV;
>
> Why are these checks needed? Hasn't log been initialized and attached at
> intel_gt_debugfs_register_files()/debugfs_create_file() time itself?
>
> Also if needed let's do:
>
> if (!log || !log->vma)
> return -ENODEV;
>
Alan: You are right, we don't to check log but might need to check log->vma: its been a long time - i can vaguely
remember but i recall some weird behavior if the user space was holding on to relay logging handles and still calling in
while driver is being unloaded. I'll have to retest this and see if its something to care about or consider as a user
error. But even there, we can shorten it to if(!log->vma) as the minimum. (of course even if there was a bug, the
debugfs path should eventually get released as part of the i915 unload but just a tiny bit later after the guc resources
are freed). Same comment here applies to two more comments you provided. Needs to be tested.
>
Alan: [snip]
