[07/17] drm/i915: Add a relay backed debugfs interface for capturing GuC logs

Commit Message

[e-mail] July 10, 2016, 1:41 p.m. UTC

From: Akash Goel <[e-mail]>

Added a new debugfs interface '/sys/kernel/debug/dri/guc_log' for the
User to capture GuC firmware logs. Availed relay framework to implement
the interface, where Driver will have to just use a relay API to store
snapshots of the GuC log buffer in the buffer managed by relay.
The snapshot will be taken when GuC firmware sends a log buffer flush
interrupt and up to four snaphots could be stored in the relay buffer.
The relay buffer will be operated in a mode where it will overwrite the
data not yet collected by User.
Besides mmap method, through which User can directly access the relay
buffer contents, relay also supports the 'poll' method. Through the 'poll'
call on log file, User can come to know whenever a new snapshot of the
log buffer is taken by Driver, so can run in tandem with the Driver and
capture the logs in a sustained/streaming manner, without any loss of data.

v2: Defer the creation of relay channel & associated debugfs file, as
    debugfs setup is now done at the end of i915 Driver load. (Chris)

v3:
- Switch to no-overwrite mode for relay.
- Fix the relay sub buffer switching sequence.

Suggested-by: Chris Wilson <[e-mail]>
Signed-off-by: Sourab Gupta <[e-mail]>
Signed-off-by: Akash Goel <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.c            |   2 +
 drivers/gpu/drm/i915/i915_guc_submission.c | 197 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_guc.h           |   3 +
 3 files changed, 199 insertions(+), 3 deletions(-)

Comments

kernel test robot July 10, 2016, 5:07 p.m. UTC | #1

Hi,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160708]
[cannot apply to v4.7-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/akash-goel-intel-com/Support-for-sustained-capturing-of-GuC-firmware-logs/20160710-213134
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-s5-07102345 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `relay_reserve':
>> i915_guc_submission.c:(.text+0x1db57e): undefined reference to `relay_switch_subbuf'
   drivers/built-in.o: In function `create_buf_file_callback':
   i915_guc_submission.c:(.text+0x1db5dc): undefined reference to `relay_file_operations'
   drivers/built-in.o: In function `subbuf_start_callback':
>> i915_guc_submission.c:(.text+0x1db5f8): undefined reference to `relay_buf_full'
   drivers/built-in.o: In function `guc_log_cleanup':
>> i915_guc_submission.c:(.text+0x1db6ec): undefined reference to `relay_close'
   drivers/built-in.o: In function `i915_guc_capture_logs':
>> (.text+0x1dcce4): undefined reference to `relay_flush'
   drivers/built-in.o: In function `i915_guc_register':
   (.text+0x1dcebd): undefined reference to `relay_open'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Tvrtko Ursulin July 19, 2016, 11:31 a.m. UTC | #2

On 10/07/16 14:41, [e-mail] wrote:
> From: Akash Goel <[e-mail]>
>
> Added a new debugfs interface '/sys/kernel/debug/dri/guc_log' for the
> User to capture GuC firmware logs. Availed relay framework to implement
> the interface, where Driver will have to just use a relay API to store
> snapshots of the GuC log buffer in the buffer managed by relay.
> The snapshot will be taken when GuC firmware sends a log buffer flush
> interrupt and up to four snaphots could be stored in the relay buffer.
> The relay buffer will be operated in a mode where it will overwrite the
> data not yet collected by User.
> Besides mmap method, through which User can directly access the relay
> buffer contents, relay also supports the 'poll' method. Through the 'poll'
> call on log file, User can come to know whenever a new snapshot of the
> log buffer is taken by Driver, so can run in tandem with the Driver and
> capture the logs in a sustained/streaming manner, without any loss of data.
>
> v2: Defer the creation of relay channel & associated debugfs file, as
>      debugfs setup is now done at the end of i915 Driver load. (Chris)
>
> v3:
> - Switch to no-overwrite mode for relay.
> - Fix the relay sub buffer switching sequence.
>
> Suggested-by: Chris Wilson <[e-mail]>
> Signed-off-by: Sourab Gupta <[e-mail]>
> Signed-off-by: Akash Goel <[e-mail]>
> ---
>   drivers/gpu/drm/i915/i915_drv.c            |   2 +
>   drivers/gpu/drm/i915/i915_guc_submission.c | 197 ++++++++++++++++++++++++++++-
>   drivers/gpu/drm/i915/intel_guc.h           |   3 +
>   3 files changed, 199 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 25c6b9b..43c9900 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1177,6 +1177,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>   	/* Reveal our presence to userspace */
>   	if (drm_dev_register(dev, 0) == 0) {
>   		i915_debugfs_register(dev_priv);
> +		i915_guc_register(dev);
>   		i915_setup_sysfs(dev);
>   	} else
>   		DRM_ERROR("Failed to register driver for userspace access!\n");
> @@ -1215,6 +1216,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>   	intel_opregion_unregister(dev_priv);
>
>   	i915_teardown_sysfs(&dev_priv->drm);
> +	i915_guc_unregister(&dev_priv->drm);
>   	i915_debugfs_unregister(dev_priv);
>   	drm_dev_unregister(&dev_priv->drm);
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index d3dbb8e..9b436fa 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -23,6 +23,8 @@
>    */
>   #include <linux/firmware.h>
>   #include <linux/circ_buf.h>
> +#include <linux/debugfs.h>
> +#include <linux/relay.h>
>   #include "i915_drv.h"
>   #include "intel_guc.h"
>
> @@ -836,12 +838,33 @@ err:
>
>   static void guc_move_to_next_buf(struct intel_guc *guc)
>   {
> -	return;
> +	/* Make sure our updates are in the sub buffer are visible when
> +	 * Consumer sees a newly produced sub buffer.
> +	 */
> +	smp_wmb();
> +
> +	/* All data has been written, so now move the offset of sub buffer. */
> +	relay_reserve(guc->log.relay_chan, guc->log.obj->base.size);
> +
> +	/* Switch to the next sub buffer */
> +	relay_flush(guc->log.relay_chan);
>   }
>
>   static void* guc_get_write_buffer(struct intel_guc *guc)
>   {
> -	return NULL;
> +	/* FIXME: Cover the check under a lock ? */
> +	if (!guc->log.relay_chan)
> +		return NULL;
> +
> +	/* Just get the base address of a new sub buffer and copy data into it
> +	 * ourselves. NULL will be returned in no-overwrite mode, if all sub
> +	 * buffers are full. Could have used the relay_write() to indirectly
> +	 * copy the data, but that would have been bit convoluted, as we need to
> +	 * write to only certain locations inside a sub buffer which cannot be
> +	 * done without using relay_reserve() along with relay_write(). So its
> +	 * better to use relay_reserve() alone.
> +	 */
> +	return relay_reserve(guc->log.relay_chan, 0);
>   }
>
>   static void guc_read_update_log_buffer(struct drm_device *dev)
> @@ -906,6 +929,119 @@ static void guc_read_update_log_buffer(struct drm_device *dev)
>   		guc_move_to_next_buf(guc);
>   }
>
> +/*
> + * Sub buffer switch callback. Called whenever relay has to switch to a new
> + * sub buffer, relay stays on the same sub buffer if 0 is returned.
> + */
> +static int subbuf_start_callback(struct rchan_buf *buf,
> +				 void *subbuf,
> +				 void *prev_subbuf,
> +				 size_t prev_padding)
> +{
> +	/* Use no-overwrite mode by default, where relay will stop accepting
> +	 * new data if there are no empty sub buffers left.
> +	 * There is no strict synchronization enforced by relay between Consumer
> +	 * and Producer. In overwrite mode, there is a possibility of getting
> +	 * inconsistent/garbled data, the producer could be writing on to the
> +	 * same sub buffer from which Consumer is reading. This can't be avoided
> +	 * unless Consumer is fast enough and can always run in tandem with
> +	 * Producer.
> +	 */
> +	if (relay_buf_full(buf))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +/*
> + * file_create() callback. Creates relay file in debugfs.
> + */
> +static struct dentry *create_buf_file_callback(const char *filename,
> +					       struct dentry *parent,
> +					       umode_t mode,
> +					       struct rchan_buf *buf,
> +					       int *is_global)
> +{
> +	/*
> +	 * Not using the channel filename passed as an argument, since for each
> +	 * channel relay appends the corresponding CPU number to the filename
> +	 * passed in relay_open(). This should be fine as relay just needs a
> +	 * dentry of the file associated with the channel buffer and that file's
> +	 * name need not be same as the filename passed as an argument.
> +	 */
> +	struct dentry *buf_file = debugfs_create_file("guc_log", mode,
> +			parent, buf, &relay_file_operations);
> +
> +	/* This to enable the use of a single buffer for the relay channel and
> +	 * correspondingly have a single file exposed to User, through which
> +	 * it can collect the logs inorder without any post-processing.
> +	 */
> +	*is_global = 1;
> +
> +	return buf_file;
> +}
> +
> +/*
> + * file_remove() default callback. Removes relay file in debugfs.
> + */
> +static int remove_buf_file_callback(struct dentry *dentry)
> +{
> +	debugfs_remove(dentry);
> +	return 0;
> +}
> +
> +/* relay channel callbacks */
> +static struct rchan_callbacks relay_callbacks = {
> +	.subbuf_start = subbuf_start_callback,
> +	.create_buf_file = create_buf_file_callback,
> +	.remove_buf_file = remove_buf_file_callback,
> +};
> +
> +static void guc_remove_log_relay_file(struct intel_guc *guc)
> +{
> +	if (guc->log.relay_chan)
> +		relay_close(guc->log.relay_chan);
> +}
> +
> +static int guc_create_log_relay_file(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct drm_device *dev = &dev_priv->drm;
> +	struct dentry *log_dir;
> +	struct rchan *guc_log_relay_chan;
> +	size_t n_subbufs, subbuf_size;
> +
> +	if (guc->log.relay_chan)
> +		return 0;
> +
> +	/* 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 (!dev->primary->debugfs_root)
> +		return -ENODEV;
> +
> +	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
> +	log_dir = dev->primary->debugfs_root;
> +
> +	/* Keep the size of sub buffers same as shared log buffer */
> +	subbuf_size = guc->log.obj->base.size;
> +	/* TODO: Decide based on the User's input */
> +	n_subbufs = 4;
> +
> +	guc_log_relay_chan = relay_open("guc_log", log_dir,
> +			subbuf_size, n_subbufs, &relay_callbacks, dev);
> +
> +	if (!guc_log_relay_chan) {
> +		DRM_DEBUG_DRIVER("Couldn't create relay chan for guc logs\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* FIXME: Cover the update under a lock ? */
> +	guc->log.relay_chan = guc_log_relay_chan;
> +	return 0;
> +}
> +
>   static void guc_log_cleanup(struct drm_i915_private *dev_priv)
>   {
>   	struct intel_guc *guc = &dev_priv->guc;
> @@ -918,6 +1054,9 @@ static void guc_log_cleanup(struct drm_i915_private *dev_priv)
>   	/* First disable the flush interrupt */
>   	gen9_disable_guc_interrupts(dev_priv);
>
> +	guc_remove_log_relay_file(guc);
> +	guc->log.relay_chan = NULL;
> +
>   	if (guc->log.buf_addr)
>   		i915_gem_object_unpin_map(guc->log.obj);
>
> @@ -996,6 +1135,39 @@ static void guc_create_log(struct intel_guc *guc)
>   	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
>   }
>
> +static int guc_log_late_setup(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_guc *guc = &dev_priv->guc;
> +	int ret;
> +
> +	lockdep_assert_held(&dev->struct_mutex);
> +
> +	if (i915.guc_log_level < 0)
> +		return -EINVAL;
> +
> +	if (WARN_ON(guc->log.relay_chan))
> +		return -EINVAL;
> +
> +	/* If log_level was set as -1 at boot time, then vmalloc mapping would
> +	 * not have been created for the log buffer, so create one now.
> +	 */
> +	ret = guc_create_log_extras(guc);
> +	if (ret)
> +		goto err;
> +
> +	ret = guc_create_log_relay_file(guc);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +err:
> +	guc_log_cleanup(dev_priv);
> +	/* logging will remain off */
> +	i915.guc_log_level = -1;
> +	return ret;
> +}
> +
>   static void init_guc_policies(struct guc_policies *policies)
>   {
>   	struct guc_policy *policy;
> @@ -1154,7 +1326,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>   	gem_release_guc_obj(dev_priv->guc.ads_obj);
>   	guc->ads_obj = NULL;
>
> -	guc_log_cleanup(dev_priv);
>   	gem_release_guc_obj(dev_priv->guc.log.obj);
>   	guc->log.obj = NULL;
>
> @@ -1232,3 +1403,23 @@ void i915_guc_capture_logs(struct drm_device *dev)
>   	host2guc_logbuffer_flush_complete(&dev_priv->guc);
>   	intel_runtime_pm_put(dev_priv);
>   }
> +
> +void i915_guc_unregister(struct drm_device *dev)
> +{
> +	if (!i915.enable_guc_submission)
> +		return;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	guc_log_cleanup(dev->dev_private);
> +	mutex_unlock(&dev->struct_mutex);
> +}
> +
> +void i915_guc_register(struct drm_device *dev)
> +{
> +	if (!i915.enable_guc_submission)
> +		return;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	guc_log_late_setup(dev);
> +	mutex_unlock(&dev->struct_mutex);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index d4f0fae..a784cf8 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -127,6 +127,7 @@ struct intel_guc_log {
>   	struct drm_i915_gem_object *obj;
>   	struct workqueue_struct *wq;
>   	void *buf_addr;
> +	struct rchan *relay_chan;
>   };
>
>   struct intel_guc {
> @@ -174,5 +175,7 @@ int i915_guc_submit(struct drm_i915_gem_request *rq);
>   void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
>   void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
>   void i915_guc_capture_logs(struct drm_device *dev);
> +void i915_guc_register(struct drm_device *dev);
> +void i915_guc_unregister(struct drm_device *dev);
>
>   #endif
>

I am clueless about relayfs - do we have someone in house who could 
provide a better review of that?

In the mean time, you got TODO and FIXME's in the code so that needs to 
be resolved before a full review anyway. :)

Regards,

Tvrtko

[e-mail] July 20, 2016, 3:41 a.m. UTC | #3

On 7/19/2016 5:01 PM, Tvrtko Ursulin wrote:
>
> On 10/07/16 14:41, [e-mail] wrote:
>> From: Akash Goel <[e-mail]>
>>
>> Added a new debugfs interface '/sys/kernel/debug/dri/guc_log' for the
>> User to capture GuC firmware logs. Availed relay framework to implement
>> the interface, where Driver will have to just use a relay API to store
>> snapshots of the GuC log buffer in the buffer managed by relay.
>> The snapshot will be taken when GuC firmware sends a log buffer flush
>> interrupt and up to four snaphots could be stored in the relay buffer.
>> The relay buffer will be operated in a mode where it will overwrite the
>> data not yet collected by User.
>> Besides mmap method, through which User can directly access the relay
>> buffer contents, relay also supports the 'poll' method. Through the
>> 'poll'
>> call on log file, User can come to know whenever a new snapshot of the
>> log buffer is taken by Driver, so can run in tandem with the Driver and
>> capture the logs in a sustained/streaming manner, without any loss of
>> data.
>>
>> v2: Defer the creation of relay channel & associated debugfs file, as
>>      debugfs setup is now done at the end of i915 Driver load. (Chris)
>>
>> v3:
>> - Switch to no-overwrite mode for relay.
>> - Fix the relay sub buffer switching sequence.
>>
>> Suggested-by: Chris Wilson <[e-mail]>
>> Signed-off-by: Sourab Gupta <[e-mail]>
>> Signed-off-by: Akash Goel <[e-mail]>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c            |   2 +
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 197
>> ++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/i915/intel_guc.h           |   3 +
>>   3 files changed, 199 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 25c6b9b..43c9900 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1177,6 +1177,7 @@ static void i915_driver_register(struct
>> drm_i915_private *dev_priv)
>>       /* Reveal our presence to userspace */
>>       if (drm_dev_register(dev, 0) == 0) {
>>           i915_debugfs_register(dev_priv);
>> +        i915_guc_register(dev);
>>           i915_setup_sysfs(dev);
>>       } else
>>           DRM_ERROR("Failed to register driver for userspace access!\n");
>> @@ -1215,6 +1216,7 @@ static void i915_driver_unregister(struct
>> drm_i915_private *dev_priv)
>>       intel_opregion_unregister(dev_priv);
>>
>>       i915_teardown_sysfs(&dev_priv->drm);
>> +    i915_guc_unregister(&dev_priv->drm);
>>       i915_debugfs_unregister(dev_priv);
>>       drm_dev_unregister(&dev_priv->drm);
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index d3dbb8e..9b436fa 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -23,6 +23,8 @@
>>    */
>>   #include <linux/firmware.h>
>>   #include <linux/circ_buf.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/relay.h>
>>   #include "i915_drv.h"
>>   #include "intel_guc.h"
>>
>> @@ -836,12 +838,33 @@ err:
>>
>>   static void guc_move_to_next_buf(struct intel_guc *guc)
>>   {
>> -    return;
>> +    /* Make sure our updates are in the sub buffer are visible when
>> +     * Consumer sees a newly produced sub buffer.
>> +     */
>> +    smp_wmb();
>> +
>> +    /* All data has been written, so now move the offset of sub
>> buffer. */
>> +    relay_reserve(guc->log.relay_chan, guc->log.obj->base.size);
>> +
>> +    /* Switch to the next sub buffer */
>> +    relay_flush(guc->log.relay_chan);
>>   }
>>
>>   static void* guc_get_write_buffer(struct intel_guc *guc)
>>   {
>> -    return NULL;
>> +    /* FIXME: Cover the check under a lock ? */
>> +    if (!guc->log.relay_chan)
>> +        return NULL;
>> +
>> +    /* Just get the base address of a new sub buffer and copy data
>> into it
>> +     * ourselves. NULL will be returned in no-overwrite mode, if all sub
>> +     * buffers are full. Could have used the relay_write() to indirectly
>> +     * copy the data, but that would have been bit convoluted, as we
>> need to
>> +     * write to only certain locations inside a sub buffer which
>> cannot be
>> +     * done without using relay_reserve() along with relay_write().
>> So its
>> +     * better to use relay_reserve() alone.
>> +     */
>> +    return relay_reserve(guc->log.relay_chan, 0);
>>   }
>>
>>   static void guc_read_update_log_buffer(struct drm_device *dev)
>> @@ -906,6 +929,119 @@ static void guc_read_update_log_buffer(struct
>> drm_device *dev)
>>           guc_move_to_next_buf(guc);
>>   }
>>
>> +/*
>> + * Sub buffer switch callback. Called whenever relay has to switch to
>> a new
>> + * sub buffer, relay stays on the same sub buffer if 0 is returned.
>> + */
>> +static int subbuf_start_callback(struct rchan_buf *buf,
>> +                 void *subbuf,
>> +                 void *prev_subbuf,
>> +                 size_t prev_padding)
>> +{
>> +    /* Use no-overwrite mode by default, where relay will stop accepting
>> +     * new data if there are no empty sub buffers left.
>> +     * There is no strict synchronization enforced by relay between
>> Consumer
>> +     * and Producer. In overwrite mode, there is a possibility of
>> getting
>> +     * inconsistent/garbled data, the producer could be writing on to
>> the
>> +     * same sub buffer from which Consumer is reading. This can't be
>> avoided
>> +     * unless Consumer is fast enough and can always run in tandem with
>> +     * Producer.
>> +     */
>> +    if (relay_buf_full(buf))
>> +        return 0;
>> +
>> +    return 1;
>> +}
>> +
>> +/*
>> + * file_create() callback. Creates relay file in debugfs.
>> + */
>> +static struct dentry *create_buf_file_callback(const char *filename,
>> +                           struct dentry *parent,
>> +                           umode_t mode,
>> +                           struct rchan_buf *buf,
>> +                           int *is_global)
>> +{
>> +    /*
>> +     * Not using the channel filename passed as an argument, since
>> for each
>> +     * channel relay appends the corresponding CPU number to the
>> filename
>> +     * passed in relay_open(). This should be fine as relay just needs a
>> +     * dentry of the file associated with the channel buffer and that
>> file's
>> +     * name need not be same as the filename passed as an argument.
>> +     */
>> +    struct dentry *buf_file = debugfs_create_file("guc_log", mode,
>> +            parent, buf, &relay_file_operations);
>> +
>> +    /* This to enable the use of a single buffer for the relay
>> channel and
>> +     * correspondingly have a single file exposed to User, through which
>> +     * it can collect the logs inorder without any post-processing.
>> +     */
>> +    *is_global = 1;
>> +
>> +    return buf_file;
>> +}
>> +
>> +/*
>> + * file_remove() default callback. Removes relay file in debugfs.
>> + */
>> +static int remove_buf_file_callback(struct dentry *dentry)
>> +{
>> +    debugfs_remove(dentry);
>> +    return 0;
>> +}
>> +
>> +/* relay channel callbacks */
>> +static struct rchan_callbacks relay_callbacks = {
>> +    .subbuf_start = subbuf_start_callback,
>> +    .create_buf_file = create_buf_file_callback,
>> +    .remove_buf_file = remove_buf_file_callback,
>> +};
>> +
>> +static void guc_remove_log_relay_file(struct intel_guc *guc)
>> +{
>> +    if (guc->log.relay_chan)
>> +        relay_close(guc->log.relay_chan);
>> +}
>> +
>> +static int guc_create_log_relay_file(struct intel_guc *guc)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +    struct drm_device *dev = &dev_priv->drm;
>> +    struct dentry *log_dir;
>> +    struct rchan *guc_log_relay_chan;
>> +    size_t n_subbufs, subbuf_size;
>> +
>> +    if (guc->log.relay_chan)
>> +        return 0;
>> +
>> +    /* 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 (!dev->primary->debugfs_root)
>> +        return -ENODEV;
>> +
>> +    /* For now create the log file in /sys/kernel/debug/dri/0 dir */
>> +    log_dir = dev->primary->debugfs_root;
>> +
>> +    /* Keep the size of sub buffers same as shared log buffer */
>> +    subbuf_size = guc->log.obj->base.size;
>> +    /* TODO: Decide based on the User's input */
>> +    n_subbufs = 4;
>> +
>> +    guc_log_relay_chan = relay_open("guc_log", log_dir,
>> +            subbuf_size, n_subbufs, &relay_callbacks, dev);
>> +
>> +    if (!guc_log_relay_chan) {
>> +        DRM_DEBUG_DRIVER("Couldn't create relay chan for guc logs\n");
>> +        return -ENOMEM;
>> +    }
>> +
>> +    /* FIXME: Cover the update under a lock ? */
>> +    guc->log.relay_chan = guc_log_relay_chan;
>> +    return 0;
>> +}
>> +
>>   static void guc_log_cleanup(struct drm_i915_private *dev_priv)
>>   {
>>       struct intel_guc *guc = &dev_priv->guc;
>> @@ -918,6 +1054,9 @@ static void guc_log_cleanup(struct
>> drm_i915_private *dev_priv)
>>       /* First disable the flush interrupt */
>>       gen9_disable_guc_interrupts(dev_priv);
>>
>> +    guc_remove_log_relay_file(guc);
>> +    guc->log.relay_chan = NULL;
>> +
>>       if (guc->log.buf_addr)
>>           i915_gem_object_unpin_map(guc->log.obj);
>>
>> @@ -996,6 +1135,39 @@ static void guc_create_log(struct intel_guc *guc)
>>       guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
>>   }
>>
>> +static int guc_log_late_setup(struct drm_device *dev)
>> +{
>> +    struct drm_i915_private *dev_priv = dev->dev_private;
>> +    struct intel_guc *guc = &dev_priv->guc;
>> +    int ret;
>> +
>> +    lockdep_assert_held(&dev->struct_mutex);
>> +
>> +    if (i915.guc_log_level < 0)
>> +        return -EINVAL;
>> +
>> +    if (WARN_ON(guc->log.relay_chan))
>> +        return -EINVAL;
>> +
>> +    /* If log_level was set as -1 at boot time, then vmalloc mapping
>> would
>> +     * not have been created for the log buffer, so create one now.
>> +     */
>> +    ret = guc_create_log_extras(guc);
>> +    if (ret)
>> +        goto err;
>> +
>> +    ret = guc_create_log_relay_file(guc);
>> +    if (ret)
>> +        goto err;
>> +
>> +    return 0;
>> +err:
>> +    guc_log_cleanup(dev_priv);
>> +    /* logging will remain off */
>> +    i915.guc_log_level = -1;
>> +    return ret;
>> +}
>> +
>>   static void init_guc_policies(struct guc_policies *policies)
>>   {
>>       struct guc_policy *policy;
>> @@ -1154,7 +1326,6 @@ void i915_guc_submission_fini(struct
>> drm_i915_private *dev_priv)
>>       gem_release_guc_obj(dev_priv->guc.ads_obj);
>>       guc->ads_obj = NULL;
>>
>> -    guc_log_cleanup(dev_priv);
>>       gem_release_guc_obj(dev_priv->guc.log.obj);
>>       guc->log.obj = NULL;
>>
>> @@ -1232,3 +1403,23 @@ void i915_guc_capture_logs(struct drm_device *dev)
>>       host2guc_logbuffer_flush_complete(&dev_priv->guc);
>>       intel_runtime_pm_put(dev_priv);
>>   }
>> +
>> +void i915_guc_unregister(struct drm_device *dev)
>> +{
>> +    if (!i915.enable_guc_submission)
>> +        return;
>> +
>> +    mutex_lock(&dev->struct_mutex);
>> +    guc_log_cleanup(dev->dev_private);
>> +    mutex_unlock(&dev->struct_mutex);
>> +}
>> +
>> +void i915_guc_register(struct drm_device *dev)
>> +{
>> +    if (!i915.enable_guc_submission)
>> +        return;
>> +
>> +    mutex_lock(&dev->struct_mutex);
>> +    guc_log_late_setup(dev);
>> +    mutex_unlock(&dev->struct_mutex);
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index d4f0fae..a784cf8 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -127,6 +127,7 @@ struct intel_guc_log {
>>       struct drm_i915_gem_object *obj;
>>       struct workqueue_struct *wq;
>>       void *buf_addr;
>> +    struct rchan *relay_chan;
>>   };
>>
>>   struct intel_guc {
>> @@ -174,5 +175,7 @@ int i915_guc_submit(struct drm_i915_gem_request *rq);
>>   void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
>>   void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
>>   void i915_guc_capture_logs(struct drm_device *dev);
>> +void i915_guc_register(struct drm_device *dev);
>> +void i915_guc_unregister(struct drm_device *dev);
>>
>>   #endif
>>
>
> I am clueless about relayfs - do we have someone in house who could
> provide a better review of that?
>
Probably a skim through 
https://www.kernel.org/doc/Documentation/filesystems/relay.txt & a bit 
of code walk-through would help, that's what I also had to do.

> In the mean time, you got TODO and FIXME's in the code so that needs to
> be resolved before a full review anyway. :)
>
TODO is already taken care in a subsequent patch.
About FIXME, I am not really sure and need suggestion/inputs on it.

Best regards
Akash

> Regards,
>
> Tvrtko
>

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 25c6b9b..43c9900 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1177,6 +1177,7 @@  static void i915_driver_register(struct drm_i915_private *dev_priv)
 	/* Reveal our presence to userspace */
 	if (drm_dev_register(dev, 0) == 0) {
 		i915_debugfs_register(dev_priv);
+		i915_guc_register(dev);
 		i915_setup_sysfs(dev);
 	} else
 		DRM_ERROR("Failed to register driver for userspace access!\n");
@@ -1215,6 +1216,7 @@  static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 	intel_opregion_unregister(dev_priv);
 
 	i915_teardown_sysfs(&dev_priv->drm);
+	i915_guc_unregister(&dev_priv->drm);
 	i915_debugfs_unregister(dev_priv);
 	drm_dev_unregister(&dev_priv->drm);
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d3dbb8e..9b436fa 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -23,6 +23,8 @@ 
  */
 #include <linux/firmware.h>
 #include <linux/circ_buf.h>
+#include <linux/debugfs.h>
+#include <linux/relay.h>
 #include "i915_drv.h"
 #include "intel_guc.h"
 
@@ -836,12 +838,33 @@  err:
 
 static void guc_move_to_next_buf(struct intel_guc *guc)
 {
-	return;
+	/* Make sure our updates are in the sub buffer are visible when
+	 * Consumer sees a newly produced sub buffer.
+	 */
+	smp_wmb();
+
+	/* All data has been written, so now move the offset of sub buffer. */
+	relay_reserve(guc->log.relay_chan, guc->log.obj->base.size);
+
+	/* Switch to the next sub buffer */
+	relay_flush(guc->log.relay_chan);
 }
 
 static void* guc_get_write_buffer(struct intel_guc *guc)
 {
-	return NULL;
+	/* FIXME: Cover the check under a lock ? */
+	if (!guc->log.relay_chan)
+		return NULL;
+
+	/* Just get the base address of a new sub buffer and copy data into it
+	 * ourselves. NULL will be returned in no-overwrite mode, if all sub
+	 * buffers are full. Could have used the relay_write() to indirectly
+	 * copy the data, but that would have been bit convoluted, as we need to
+	 * write to only certain locations inside a sub buffer which cannot be
+	 * done without using relay_reserve() along with relay_write(). So its
+	 * better to use relay_reserve() alone.
+	 */
+	return relay_reserve(guc->log.relay_chan, 0);
 }
 
 static void guc_read_update_log_buffer(struct drm_device *dev)
@@ -906,6 +929,119 @@  static void guc_read_update_log_buffer(struct drm_device *dev)
 		guc_move_to_next_buf(guc);
 }
 
+/*
+ * Sub buffer switch callback. Called whenever relay has to switch to a new
+ * sub buffer, relay stays on the same sub buffer if 0 is returned.
+ */
+static int subbuf_start_callback(struct rchan_buf *buf,
+				 void *subbuf,
+				 void *prev_subbuf,
+				 size_t prev_padding)
+{
+	/* Use no-overwrite mode by default, where relay will stop accepting
+	 * new data if there are no empty sub buffers left.
+	 * There is no strict synchronization enforced by relay between Consumer
+	 * and Producer. In overwrite mode, there is a possibility of getting
+	 * inconsistent/garbled data, the producer could be writing on to the
+	 * same sub buffer from which Consumer is reading. This can't be avoided
+	 * unless Consumer is fast enough and can always run in tandem with
+	 * Producer.
+	 */
+	if (relay_buf_full(buf))
+		return 0;
+
+	return 1;
+}
+
+/*
+ * file_create() callback. Creates relay file in debugfs.
+ */
+static struct dentry *create_buf_file_callback(const char *filename,
+					       struct dentry *parent,
+					       umode_t mode,
+					       struct rchan_buf *buf,
+					       int *is_global)
+{
+	/*
+	 * Not using the channel filename passed as an argument, since for each
+	 * channel relay appends the corresponding CPU number to the filename
+	 * passed in relay_open(). This should be fine as relay just needs a
+	 * dentry of the file associated with the channel buffer and that file's
+	 * name need not be same as the filename passed as an argument.
+	 */
+	struct dentry *buf_file = debugfs_create_file("guc_log", mode,
+			parent, buf, &relay_file_operations);
+
+	/* This to enable the use of a single buffer for the relay channel and
+	 * correspondingly have a single file exposed to User, through which
+	 * it can collect the logs inorder without any post-processing.
+	 */
+	*is_global = 1;
+
+	return buf_file;
+}
+
+/*
+ * file_remove() default callback. Removes relay file in debugfs.
+ */
+static int remove_buf_file_callback(struct dentry *dentry)
+{
+	debugfs_remove(dentry);
+	return 0;
+}
+
+/* relay channel callbacks */
+static struct rchan_callbacks relay_callbacks = {
+	.subbuf_start = subbuf_start_callback,
+	.create_buf_file = create_buf_file_callback,
+	.remove_buf_file = remove_buf_file_callback,
+};
+
+static void guc_remove_log_relay_file(struct intel_guc *guc)
+{
+	if (guc->log.relay_chan)
+		relay_close(guc->log.relay_chan);
+}
+
+static int guc_create_log_relay_file(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct drm_device *dev = &dev_priv->drm;
+	struct dentry *log_dir;
+	struct rchan *guc_log_relay_chan;
+	size_t n_subbufs, subbuf_size;
+
+	if (guc->log.relay_chan)
+		return 0;
+
+	/* 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 (!dev->primary->debugfs_root)
+		return -ENODEV;
+
+	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
+	log_dir = dev->primary->debugfs_root;
+
+	/* Keep the size of sub buffers same as shared log buffer */
+	subbuf_size = guc->log.obj->base.size;
+	/* TODO: Decide based on the User's input */
+	n_subbufs = 4;
+
+	guc_log_relay_chan = relay_open("guc_log", log_dir,
+			subbuf_size, n_subbufs, &relay_callbacks, dev);
+
+	if (!guc_log_relay_chan) {
+		DRM_DEBUG_DRIVER("Couldn't create relay chan for guc logs\n");
+		return -ENOMEM;
+	}
+
+	/* FIXME: Cover the update under a lock ? */
+	guc->log.relay_chan = guc_log_relay_chan;
+	return 0;
+}
+
 static void guc_log_cleanup(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -918,6 +1054,9 @@  static void guc_log_cleanup(struct drm_i915_private *dev_priv)
 	/* First disable the flush interrupt */
 	gen9_disable_guc_interrupts(dev_priv);
 
+	guc_remove_log_relay_file(guc);
+	guc->log.relay_chan = NULL;
+
 	if (guc->log.buf_addr)
 		i915_gem_object_unpin_map(guc->log.obj);
 
@@ -996,6 +1135,39 @@  static void guc_create_log(struct intel_guc *guc)
 	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
 }
 
+static int guc_log_late_setup(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_guc *guc = &dev_priv->guc;
+	int ret;
+
+	lockdep_assert_held(&dev->struct_mutex);
+
+	if (i915.guc_log_level < 0)
+		return -EINVAL;
+
+	if (WARN_ON(guc->log.relay_chan))
+		return -EINVAL;
+
+	/* If log_level was set as -1 at boot time, then vmalloc mapping would
+	 * not have been created for the log buffer, so create one now.
+	 */
+	ret = guc_create_log_extras(guc);
+	if (ret)
+		goto err;
+
+	ret = guc_create_log_relay_file(guc);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	guc_log_cleanup(dev_priv);
+	/* logging will remain off */
+	i915.guc_log_level = -1;
+	return ret;
+}
+
 static void init_guc_policies(struct guc_policies *policies)
 {
 	struct guc_policy *policy;
@@ -1154,7 +1326,6 @@  void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	gem_release_guc_obj(dev_priv->guc.ads_obj);
 	guc->ads_obj = NULL;
 
-	guc_log_cleanup(dev_priv);
 	gem_release_guc_obj(dev_priv->guc.log.obj);
 	guc->log.obj = NULL;
 
@@ -1232,3 +1403,23 @@  void i915_guc_capture_logs(struct drm_device *dev)
 	host2guc_logbuffer_flush_complete(&dev_priv->guc);
 	intel_runtime_pm_put(dev_priv);
 }
+
+void i915_guc_unregister(struct drm_device *dev)
+{
+	if (!i915.enable_guc_submission)
+		return;
+
+	mutex_lock(&dev->struct_mutex);
+	guc_log_cleanup(dev->dev_private);
+	mutex_unlock(&dev->struct_mutex);
+}
+
+void i915_guc_register(struct drm_device *dev)
+{
+	if (!i915.enable_guc_submission)
+		return;
+
+	mutex_lock(&dev->struct_mutex);
+	guc_log_late_setup(dev);
+	mutex_unlock(&dev->struct_mutex);
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index d4f0fae..a784cf8 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -127,6 +127,7 @@  struct intel_guc_log {
 	struct drm_i915_gem_object *obj;
 	struct workqueue_struct *wq;
 	void *buf_addr;
+	struct rchan *relay_chan;
 };
 
 struct intel_guc {
@@ -174,5 +175,7 @@  int i915_guc_submit(struct drm_i915_gem_request *rq);
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
 void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
 void i915_guc_capture_logs(struct drm_device *dev);
+void i915_guc_register(struct drm_device *dev);
+void i915_guc_unregister(struct drm_device *dev);
 
 #endif