| Message ID | 1470983123-22127-21-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/08/16 07:25, akash.goel@intel.com wrote: > From: Akash Goel <akash.goel@intel.com> > > As per the current i915 Driver load sequence, debugfs registration is done > at the end and so the relay channel debugfs file is also created after that > but the GuC firmware is loaded much earlier in the sequence. > As a result Driver could miss capturing the boot-time logs of GuC firmware > if there are flush interrupts from the GuC side. > Relay has a provision to support early logging where initially only relay > channel can be created, to have buffers for storing logs, and later on > channel can be associated with a debugfs file at appropriate time. > Have availed that, which allows Driver to capture boot time logs also, > which can be collected once Userspace comes up. > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Akash Goel <akash.goel@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 61 +++++++++++++++++++++--------- > 1 file changed, 44 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index af48f62..1c287d7 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -1099,25 +1099,12 @@ static void guc_remove_log_relay_file(struct intel_guc *guc) > relay_close(guc->log.relay_chan); > } > > -static int guc_create_log_relay_file(struct intel_guc *guc) > +static int guc_create_relay_channel(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > struct rchan *guc_log_relay_chan; > - struct dentry *log_dir; > size_t n_subbufs, subbuf_size; > > - /* For now create the log file in /sys/kernel/debug/dri/0 dir */ > - log_dir = dev_priv->drm.primary->debugfs_root; > - > - /* If /sys/kernel/debug/dri/0 location do not exist, then debugfs is > - * not mounted and so can't create the relay file. > - * The relay API seems to fit well with debugfs only. It only needs a dentry, I don't see that it has to be a debugfs one. > - */ > - if (!log_dir) { > - DRM_DEBUG_DRIVER("Parent debugfs directory not available yet\n"); > - return -ENODEV; > - } > - > /* Keep the size of sub buffers same as shared log buffer */ > subbuf_size = guc->log.obj->base.size; > /* Store up to 8 snaphosts, which is large enough to buffer sufficient > @@ -1127,7 +1114,7 @@ static int guc_create_log_relay_file(struct intel_guc *guc) > */ > n_subbufs = 8; > > - guc_log_relay_chan = relay_open("guc_log", log_dir, > + guc_log_relay_chan = relay_open(NULL, NULL, > subbuf_size, n_subbufs, &relay_callbacks, dev_priv); > > if (!guc_log_relay_chan) { > @@ -1140,6 +1127,33 @@ static int guc_create_log_relay_file(struct intel_guc *guc) > return 0; > } > > +static int guc_create_log_relay_file(struct intel_guc *guc) > +{ > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > + struct dentry *log_dir; > + int ret; > + > + /* For now create the log file in /sys/kernel/debug/dri/0 dir */ > + log_dir = dev_priv->drm.primary->debugfs_root; > + > + /* If /sys/kernel/debug/dri/0 location do not exist, then debugfs is > + * not mounted and so can't create the relay file. > + * The relay API seems to fit well with debugfs only. > + */ > + if (!log_dir) { > + DRM_DEBUG_DRIVER("Parent debugfs directory not available yet\n"); > + return -ENODEV; > + } > + > + ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir); > + if (ret) { > + DRM_DEBUG_DRIVER("Couldn't associate the channel with file %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > static void guc_log_cleanup(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > @@ -1167,7 +1181,7 @@ static int guc_create_log_extras(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > void *vaddr; > - int ret; > + int ret = 0; > > lockdep_assert_held(&dev_priv->drm.struct_mutex); > > @@ -1190,7 +1204,15 @@ static int guc_create_log_extras(struct intel_guc *guc) > guc->log.buf_addr = vaddr; > } > > - return 0; > + if (!guc->log.relay_chan) { > + /* Create a relay channel, so that we have buffers for storing > + * the GuC firmware logs, the channel will be linked with a file > + * later on when debugfs is registered. > + */ > + ret = guc_create_relay_channel(guc); > + } > + > + return ret; > } > > static void guc_create_log(struct intel_guc *guc) > @@ -1231,6 +1253,7 @@ static void guc_create_log(struct intel_guc *guc) > guc->log.obj = obj; > > if (guc_create_log_extras(guc)) { > + guc_log_cleanup(guc); > gem_release_guc_obj(guc->log.obj); > guc->log.obj = NULL; > i915.guc_log_level = -1; > @@ -1265,6 +1288,10 @@ static int guc_log_late_setup(struct intel_guc *guc) > if (ret) > goto err; > > + /* Parent debugfs dir should be available by now, associate the already > + * opened relay channel with a debugfs file, which will then allow User > + * to pull the logs. > + */ > ret = guc_create_log_relay_file(guc); > if (ret) > goto err; > Can't spot any problems. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> I suppose you will also have to add some IGTs to exercise the whole thing. Stopping/starting the logging, reading back, capturing some commands etc. Or contract someone from validation to do it. :) Are there any such plans? Regards, Tvrtko
On 8/12/2016 9:52 PM, Tvrtko Ursulin wrote: > > On 12/08/16 07:25, akash.goel@intel.com wrote: >> From: Akash Goel <akash.goel@intel.com> >> >> As per the current i915 Driver load sequence, debugfs registration is >> done >> at the end and so the relay channel debugfs file is also created after >> that >> but the GuC firmware is loaded much earlier in the sequence. >> As a result Driver could miss capturing the boot-time logs of GuC >> firmware >> if there are flush interrupts from the GuC side. >> Relay has a provision to support early logging where initially only relay >> channel can be created, to have buffers for storing logs, and later on >> channel can be associated with a debugfs file at appropriate time. >> Have availed that, which allows Driver to capture boot time logs also, >> which can be collected once Userspace comes up. >> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Akash Goel <akash.goel@intel.com> >> --- >> drivers/gpu/drm/i915/i915_guc_submission.c | 61 >> +++++++++++++++++++++--------- >> 1 file changed, 44 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >> b/drivers/gpu/drm/i915/i915_guc_submission.c >> index af48f62..1c287d7 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -1099,25 +1099,12 @@ static void guc_remove_log_relay_file(struct >> intel_guc *guc) >> relay_close(guc->log.relay_chan); >> } >> >> -static int guc_create_log_relay_file(struct intel_guc *guc) >> +static int guc_create_relay_channel(struct intel_guc *guc) >> { >> struct drm_i915_private *dev_priv = guc_to_i915(guc); >> struct rchan *guc_log_relay_chan; >> - struct dentry *log_dir; >> size_t n_subbufs, subbuf_size; >> >> - /* For now create the log file in /sys/kernel/debug/dri/0 dir */ >> - log_dir = dev_priv->drm.primary->debugfs_root; >> - >> - /* If /sys/kernel/debug/dri/0 location do not exist, then debugfs is >> - * not mounted and so can't create the relay file. >> - * The relay API seems to fit well with debugfs only. > > It only needs a dentry, I don't see that it has to be a debugfs one. > Besides dentry, there are other requirements for using relay, which can be met only for a debugfs file. debugfs wasn't the preferred choice to place the log file, but had no other option, as relay API is compatible with debugfs only. Also retrieving dentry of a file is not so straight forward, as it might seem (spent considerable time on this initially). >> - */ >> - if (!log_dir) { >> - DRM_DEBUG_DRIVER("Parent debugfs directory not available >> yet\n"); >> - return -ENODEV; >> - } >> - >> /* Keep the size of sub buffers same as shared log buffer */ >> subbuf_size = guc->log.obj->base.size; >> /* Store up to 8 snaphosts, which is large enough to buffer >> sufficient >> @@ -1127,7 +1114,7 @@ static int guc_create_log_relay_file(struct >> intel_guc *guc) >> */ >> n_subbufs = 8; >> >> - guc_log_relay_chan = relay_open("guc_log", log_dir, >> + guc_log_relay_chan = relay_open(NULL, NULL, >> subbuf_size, n_subbufs, &relay_callbacks, dev_priv); >> >> if (!guc_log_relay_chan) { >> @@ -1140,6 +1127,33 @@ static int guc_create_log_relay_file(struct >> intel_guc *guc) >> return 0; >> } >> >> +static int guc_create_log_relay_file(struct intel_guc *guc) >> +{ >> + struct drm_i915_private *dev_priv = guc_to_i915(guc); >> + struct dentry *log_dir; >> + int ret; >> + >> + /* For now create the log file in /sys/kernel/debug/dri/0 dir */ >> + log_dir = dev_priv->drm.primary->debugfs_root; >> + >> + /* If /sys/kernel/debug/dri/0 location do not exist, then debugfs is >> + * not mounted and so can't create the relay file. >> + * The relay API seems to fit well with debugfs only. >> + */ >> + if (!log_dir) { >> + DRM_DEBUG_DRIVER("Parent debugfs directory not available >> yet\n"); >> + return -ENODEV; >> + } >> + >> + ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", >> log_dir); >> + if (ret) { >> + DRM_DEBUG_DRIVER("Couldn't associate the channel with file >> %d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> static void guc_log_cleanup(struct intel_guc *guc) >> { >> struct drm_i915_private *dev_priv = guc_to_i915(guc); >> @@ -1167,7 +1181,7 @@ static int guc_create_log_extras(struct >> intel_guc *guc) >> { >> struct drm_i915_private *dev_priv = guc_to_i915(guc); >> void *vaddr; >> - int ret; >> + int ret = 0; >> >> lockdep_assert_held(&dev_priv->drm.struct_mutex); >> >> @@ -1190,7 +1204,15 @@ static int guc_create_log_extras(struct >> intel_guc *guc) >> guc->log.buf_addr = vaddr; >> } >> >> - return 0; >> + if (!guc->log.relay_chan) { >> + /* Create a relay channel, so that we have buffers for storing >> + * the GuC firmware logs, the channel will be linked with a file >> + * later on when debugfs is registered. >> + */ >> + ret = guc_create_relay_channel(guc); >> + } >> + >> + return ret; >> } >> >> static void guc_create_log(struct intel_guc *guc) >> @@ -1231,6 +1253,7 @@ static void guc_create_log(struct intel_guc *guc) >> guc->log.obj = obj; >> >> if (guc_create_log_extras(guc)) { >> + guc_log_cleanup(guc); >> gem_release_guc_obj(guc->log.obj); >> guc->log.obj = NULL; >> i915.guc_log_level = -1; >> @@ -1265,6 +1288,10 @@ static int guc_log_late_setup(struct intel_guc >> *guc) >> if (ret) >> goto err; >> >> + /* Parent debugfs dir should be available by now, associate the >> already >> + * opened relay channel with a debugfs file, which will then >> allow User >> + * to pull the logs. >> + */ >> ret = guc_create_log_relay_file(guc); >> if (ret) >> goto err; >> > > Can't spot any problems. > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > I suppose you will also have to add some IGTs to exercise the whole > thing. Stopping/starting the logging, reading back, capturing some > commands etc. Or contract someone from validation to do it. :) Are there > any such plans? Sure will seek help from Validation folks on this. Best regards Akash > > Regards, > > Tvrtko
On 12/08/16 17:31, Goel, Akash wrote: > On 8/12/2016 9:52 PM, Tvrtko Ursulin wrote: >> On 12/08/16 07:25, akash.goel@intel.com wrote: >>> From: Akash Goel <akash.goel@intel.com> >>> >>> As per the current i915 Driver load sequence, debugfs registration is >>> done >>> at the end and so the relay channel debugfs file is also created after >>> that >>> but the GuC firmware is loaded much earlier in the sequence. >>> As a result Driver could miss capturing the boot-time logs of GuC >>> firmware >>> if there are flush interrupts from the GuC side. >>> Relay has a provision to support early logging where initially only >>> relay >>> channel can be created, to have buffers for storing logs, and later on >>> channel can be associated with a debugfs file at appropriate time. >>> Have availed that, which allows Driver to capture boot time logs also, >>> which can be collected once Userspace comes up. >>> >>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Signed-off-by: Akash Goel <akash.goel@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_guc_submission.c | 61 >>> +++++++++++++++++++++--------- >>> 1 file changed, 44 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >>> b/drivers/gpu/drm/i915/i915_guc_submission.c >>> index af48f62..1c287d7 100644 >>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >>> @@ -1099,25 +1099,12 @@ static void guc_remove_log_relay_file(struct >>> intel_guc *guc) >>> relay_close(guc->log.relay_chan); >>> } >>> >>> -static int guc_create_log_relay_file(struct intel_guc *guc) >>> +static int guc_create_relay_channel(struct intel_guc *guc) >>> { >>> struct drm_i915_private *dev_priv = guc_to_i915(guc); >>> struct rchan *guc_log_relay_chan; >>> - struct dentry *log_dir; >>> size_t n_subbufs, subbuf_size; >>> >>> - /* For now create the log file in /sys/kernel/debug/dri/0 dir */ >>> - log_dir = dev_priv->drm.primary->debugfs_root; >>> - >>> - /* If /sys/kernel/debug/dri/0 location do not exist, then >>> debugfs is >>> - * not mounted and so can't create the relay file. >>> - * The relay API seems to fit well with debugfs only. >> >> It only needs a dentry, I don't see that it has to be a debugfs one. >> > Besides dentry, there are other requirements for using relay, which can > be met only for a debugfs file. > debugfs wasn't the preferred choice to place the log file, but had no > other option, as relay API is compatible with debugfs only. What are those and should they be mentioned in the comment above? Regards, Tvrtko
On 8/15/2016 2:50 PM, Tvrtko Ursulin wrote: > > On 12/08/16 17:31, Goel, Akash wrote: >> On 8/12/2016 9:52 PM, Tvrtko Ursulin wrote: >>> On 12/08/16 07:25, akash.goel@intel.com wrote: >>>> From: Akash Goel <akash.goel@intel.com> >>>> >>>> As per the current i915 Driver load sequence, debugfs registration is >>>> done >>>> at the end and so the relay channel debugfs file is also created after >>>> that >>>> but the GuC firmware is loaded much earlier in the sequence. >>>> As a result Driver could miss capturing the boot-time logs of GuC >>>> firmware >>>> if there are flush interrupts from the GuC side. >>>> Relay has a provision to support early logging where initially only >>>> relay >>>> channel can be created, to have buffers for storing logs, and later on >>>> channel can be associated with a debugfs file at appropriate time. >>>> Have availed that, which allows Driver to capture boot time logs also, >>>> which can be collected once Userspace comes up. >>>> >>>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> >>>> Signed-off-by: Akash Goel <akash.goel@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_guc_submission.c | 61 >>>> +++++++++++++++++++++--------- >>>> 1 file changed, 44 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >>>> b/drivers/gpu/drm/i915/i915_guc_submission.c >>>> index af48f62..1c287d7 100644 >>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >>>> @@ -1099,25 +1099,12 @@ static void guc_remove_log_relay_file(struct >>>> intel_guc *guc) >>>> relay_close(guc->log.relay_chan); >>>> } >>>> >>>> -static int guc_create_log_relay_file(struct intel_guc *guc) >>>> +static int guc_create_relay_channel(struct intel_guc *guc) >>>> { >>>> struct drm_i915_private *dev_priv = guc_to_i915(guc); >>>> struct rchan *guc_log_relay_chan; >>>> - struct dentry *log_dir; >>>> size_t n_subbufs, subbuf_size; >>>> >>>> - /* For now create the log file in /sys/kernel/debug/dri/0 dir */ >>>> - log_dir = dev_priv->drm.primary->debugfs_root; >>>> - >>>> - /* If /sys/kernel/debug/dri/0 location do not exist, then >>>> debugfs is >>>> - * not mounted and so can't create the relay file. >>>> - * The relay API seems to fit well with debugfs only. >>> >>> It only needs a dentry, I don't see that it has to be a debugfs one. >>> >> Besides dentry, there are other requirements for using relay, which can >> be met only for a debugfs file. >> debugfs wasn't the preferred choice to place the log file, but had no >> other option, as relay API is compatible with debugfs only. > > What are those and For availing relay there are 3 requirements :- a) Need the associated ‘dentry’ pointer of the file, while opening the relay channel. b) Should be able to use 'relay_file_operations' fops for the file. c) Set the 'i_private' field of file’s inode to the pointer of relay channel buffer. All the above 3 requirements can be met for a debugfs file in a straightforward manner. But not all of them can be met for a file created inside sysfs or if the file is created inside /dev as a character device file. > should they be mentioned in the comment above? Or should I mention them in the cover letter or commit message. Best regards Akash > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index af48f62..1c287d7 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -1099,25 +1099,12 @@ static void guc_remove_log_relay_file(struct intel_guc *guc) relay_close(guc->log.relay_chan); } -static int guc_create_log_relay_file(struct intel_guc *guc) +static int guc_create_relay_channel(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); struct rchan *guc_log_relay_chan; - struct dentry *log_dir; size_t n_subbufs, subbuf_size; - /* For now create the log file in /sys/kernel/debug/dri/0 dir */ - log_dir = dev_priv->drm.primary->debugfs_root; - - /* If /sys/kernel/debug/dri/0 location do not exist, then debugfs is - * not mounted and so can't create the relay file. - * The relay API seems to fit well with debugfs only. - */ - if (!log_dir) { - DRM_DEBUG_DRIVER("Parent debugfs directory not available yet\n"); - return -ENODEV; - } - /* Keep the size of sub buffers same as shared log buffer */ subbuf_size = guc->log.obj->base.size; /* Store up to 8 snaphosts, which is large enough to buffer sufficient @@ -1127,7 +1114,7 @@ static int guc_create_log_relay_file(struct intel_guc *guc) */ n_subbufs = 8; - guc_log_relay_chan = relay_open("guc_log", log_dir, + guc_log_relay_chan = relay_open(NULL, NULL, subbuf_size, n_subbufs, &relay_callbacks, dev_priv); if (!guc_log_relay_chan) { @@ -1140,6 +1127,33 @@ static int guc_create_log_relay_file(struct intel_guc *guc) return 0; } +static int guc_create_log_relay_file(struct intel_guc *guc) +{ + struct drm_i915_private *dev_priv = guc_to_i915(guc); + struct dentry *log_dir; + int ret; + + /* For now create the log file in /sys/kernel/debug/dri/0 dir */ + log_dir = dev_priv->drm.primary->debugfs_root; + + /* If /sys/kernel/debug/dri/0 location do not exist, then debugfs is + * not mounted and so can't create the relay file. + * The relay API seems to fit well with debugfs only. + */ + if (!log_dir) { + DRM_DEBUG_DRIVER("Parent debugfs directory not available yet\n"); + return -ENODEV; + } + + ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir); + if (ret) { + DRM_DEBUG_DRIVER("Couldn't associate the channel with file %d\n", ret); + return ret; + } + + return 0; +} + static void guc_log_cleanup(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); @@ -1167,7 +1181,7 @@ static int guc_create_log_extras(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); void *vaddr; - int ret; + int ret = 0; lockdep_assert_held(&dev_priv->drm.struct_mutex); @@ -1190,7 +1204,15 @@ static int guc_create_log_extras(struct intel_guc *guc) guc->log.buf_addr = vaddr; } - return 0; + if (!guc->log.relay_chan) { + /* Create a relay channel, so that we have buffers for storing + * the GuC firmware logs, the channel will be linked with a file + * later on when debugfs is registered. + */ + ret = guc_create_relay_channel(guc); + } + + return ret; } static void guc_create_log(struct intel_guc *guc) @@ -1231,6 +1253,7 @@ static void guc_create_log(struct intel_guc *guc) guc->log.obj = obj; if (guc_create_log_extras(guc)) { + guc_log_cleanup(guc); gem_release_guc_obj(guc->log.obj); guc->log.obj = NULL; i915.guc_log_level = -1; @@ -1265,6 +1288,10 @@ static int guc_log_late_setup(struct intel_guc *guc) if (ret) goto err; + /* Parent debugfs dir should be available by now, associate the already + * opened relay channel with a debugfs file, which will then allow User + * to pull the logs. + */ ret = guc_create_log_relay_file(guc); if (ret) goto err;