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

Tvrtko Ursulin
2016-08-15T15:59:26Z

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