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

[Goel, Akash]

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 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.
>
> 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