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

[Goel, Akash]

On 8/12/2016 9:52 PM, Tvrtko Ursulin wrote:
>
> On 12/08/16 07:25, akash.goel at intel.com wrote:
>> From: Akash Goel <akash.goel at 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 at chris-wilson.co.uk>
>> Signed-off-by: Akash Goel <akash.goel at 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 at 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