Re: [PATCH 20/20] drm/i915: Early creation of relay channel for capturing boot time logs

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx