Re: [PATCH 06/11] drm/i915: Add a relay backed debugfs interface for capturing GuC logs

Chris Wilson
2016-06-28T09:47:42Z

On Mon, Jun 27, 2016 at 05:46:53PM +0530, akash.goel@intel.com wrote:
> +static void guc_remove_log_relay_file(struct intel_guc *guc)
> +{
> + relay_close(guc->log_relay_chan);
> +}
> +
> +static void 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->dev;
> + struct dentry *log_dir;
> + struct rchan *guc_log_relay_chan;
> + size_t n_subbufs, subbuf_size;
> +
> + if (guc->log_relay_chan)
> + return;
> +
> + /* 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.
> + */

Ah. dev->primary->debugfs_root does not exist until the end of driver
loading.

You need to add an intel_guc_register() to the i915_driver_register()
after we call drm_dev_rigster() (that then calls this function).

Similarly, this needs to be torn down in unregister.

> + if (!dev->primary->debugfs_root) {
> + /* logging will remain off */
> + i915.guc_log_level = -1;
> + return;
> + }
> +
> + /* For now create the log file in /sys/kernel/debug/dri dir. */
> + log_dir = dev->primary->debugfs_root->d_parent;

In future, this will be something like /sys/kernel/gpu/i915/guc_log, so
I don't see a good argument for not being more canonical in the debugfs
placement and using dev->primary->debugfs_root (i.e. /.../dri/0)

At the very least, you need to explain why we don't use dri/0/
-Chris

--
Chris Wilson, Intel Open Source Technology Centre