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

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

On 15/08/16 15:49, 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.
>
> v2:
> - Remove the couple of FIXMEs, as now the relay channel will be created
>    early before enabling the flush interrupts, so no possibility of relay
>    channel pointer being modified & read at the same time from 2 different
>    execution contexts.
> - Rebase.
>
> 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 | 66 +++++++++++++++++++++---------
>   1 file changed, 46 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 8733c19..34e4335 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -937,13 +937,39 @@ 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;
>
> +	/* Keep the size of sub buffers same as shared log buffer */
> +	subbuf_size = guc->log.vma->obj->base.size;
> +
> +	/* Store up to 8 snapshots, which is large enough to buffer sufficient
> +	 * boot time logs and provides enough leeway to User, in terms of
> +	 * latency, for consuming the logs from relay. Also doesn't take
> +	 * up too much memory.
> +	 */
> +	n_subbufs = 8;
> +
> +	guc_log_relay_chan = relay_open(NULL, NULL, subbuf_size,
> +					n_subbufs, &relay_callbacks, dev_priv);
> +	if (!guc_log_relay_chan) {
> +		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
> +		return -ENOMEM;
> +	}
> +
> +	guc->log.relay_chan = guc_log_relay_chan;
> +	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;
>
> @@ -963,25 +989,12 @@ static int guc_create_log_relay_file(struct intel_guc *guc)
>   		return -ENODEV;
>   	}
>
> -	/* Keep the size of sub buffers same as shared log buffer */
> -	subbuf_size = guc->log.vma->obj->base.size;
> -
> -	/* Store up to 8 snapshots, which is large enough to buffer sufficient
> -	 * boot time logs and provides enough leeway to User, in terms of
> -	 * latency, for consuming the logs from relay. Also doesn't take
> -	 * up too much memory.
> -	 */
> -	n_subbufs = 8;
> -
> -	guc_log_relay_chan = relay_open("guc_log", log_dir, subbuf_size,
> -					n_subbufs, &relay_callbacks, dev_priv);
> -	if (!guc_log_relay_chan) {
> -		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
> -		return -ENOMEM;
> +	ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
> +	if (ret) {
> +		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
> +		return ret;
>   	}
>
> -	/* FIXME: Cover the update under a lock ? */
> -	guc->log.relay_chan = guc_log_relay_chan;
>   	return 0;
>   }
>
> @@ -1001,7 +1014,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
>
>   static void *guc_get_write_buffer(struct intel_guc *guc)
>   {
> -	/* FIXME: Cover the check under a lock ? */
>   	if (!guc->log.relay_chan)
>   		return NULL;
>
> @@ -1227,6 +1239,16 @@ static int guc_create_log_extras(struct intel_guc *guc)
>   		guc->log.buf_addr = vaddr;
>   	}
>
> +	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);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	if (!guc->log.flush_wq) {
>   		INIT_WORK(&guc->log.flush_work, guc_capture_logs_work);
>
> @@ -1314,6 +1336,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;
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx