Re: [Intel-gfx] [Intel-gfx 4/6] drm/i915/guc: Provide debugfs for log relay sub-buf info

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 05 Dec 2022 17:55:20 -0800, Teres Alexis, Alan Previn wrote:
>

Hi Alan,

> 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.

OK, great!

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

So did you test this before sending out the latest series? Or does this
still have to be done? I am holding off on this patch till I hear about
this.

Thanks.
--
Ashutosh



>
> >
> Alan: [snip]