[Intel-gfx] [RFC] drm/i915/guc: Enable guc logging on guc log relay write

[Daniele Ceraolo Spurio]

On 9/10/19 3:46 PM, Robert M. Fosha wrote:
> Creating and opening the GuC log relay file enables and starts
> the relay potentially before the caller is ready to consume logs.
> Change the behavior so that relay starts only on an explicit call
> to the write function (with a value of '1'). Other values flush
> the log relay as before.
> 
> Cc: Matthew Brost <matthew.brost at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Signed-off-by: Robert M. Fosha <robert.m.fosha at intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 38 +++++++++++++++++-----
>   drivers/gpu/drm/i915/gt/uc/intel_guc_log.h |  2 ++
>   drivers/gpu/drm/i915/i915_debugfs.c        | 27 +++++++++++++--
>   3 files changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index 36332064de9c..9a98270d05b6 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -361,6 +361,7 @@ void intel_guc_log_init_early(struct intel_guc_log *log)
>   {
>   	mutex_init(&log->relay.lock);
>   	INIT_WORK(&log->relay.flush_work, capture_logs_work);
> +	log->relay_started = false;
>   }
>   
>   static int guc_log_relay_create(struct intel_guc_log *log)
> @@ -585,15 +586,6 @@ int intel_guc_log_relay_open(struct intel_guc_log *log)
>   
>   	mutex_unlock(&log->relay.lock);
>   
> -	guc_log_enable_flush_events(log);
> -
> -	/*
> -	 * When GuC is logging without us relaying to userspace, we're ignoring
> -	 * the flush notification. This means that we need to unconditionally
> -	 * flush on relay enabling, since GuC only notifies us once.
> -	 */
> -	queue_work(system_highpri_wq, &log->relay.flush_work);
> -
>   	return 0;
>   
>   out_relay:
> @@ -604,12 +596,38 @@ int intel_guc_log_relay_open(struct intel_guc_log *log)
>   	return ret;
>   }
>   
> +int intel_guc_log_relay_start(struct intel_guc_log *log)
> +{
> +	int ret = 0;
> +
> +	if (log->relay_started) {
> +		ret =  -EEXIST;
> +	} else {

style: for this kind of checks, we usually just return early instead of 
using an if-else, i.e.:

	if (log->relay_started)
		return -EEXIST;

	[...] /* code */

	return 0;

> +		guc_log_enable_flush_events(log);
> +
> +		/*
> +		 * When GuC is logging without us relaying to userspace, we're
> +		 * ignoring the flush notification. This means that we need to
> +		 * unconditionally * flush on relay enabling, since GuC only

stray "*"

> +		 * notifies us once.
> +		 */
> +		queue_work(system_highpri_wq, &log->relay.flush_work);
> +
> +		log->relay_started = false;

s/false/true/

> +	}
> +
> +	return ret;
> +}
> +
>   void intel_guc_log_relay_flush(struct intel_guc_log *log)
>   {
>   	struct intel_guc *guc = log_to_guc(log);
>   	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>   	intel_wakeref_t wakeref;
>   
> +	if (!log->relay_started)
> +		return;
> +
>   	/*
>   	 * Before initiating the forceful flush, wait for any pending/ongoing
>   	 * flush to complete otherwise forceful flush may not actually happen.
> @@ -638,6 +656,8 @@ void intel_guc_log_relay_close(struct intel_guc_log *log)
>   	guc_log_unmap(log);
>   	guc_log_relay_destroy(log);
>   	mutex_unlock(&log->relay.lock);
> +
> +	log->relay_started = false;


For symmetry, it might be worth adding a guc_log_relay_stop:

static void guc_log_relay_stop(...)
{
	if (!log->relay_started)
		return;

	guc_log_disable_flush_events(log);
	intel_synchronize_irq(i915);

	flush_work(&log->relay.flush_work);

	log->relay_started = false;
}

and call it from intel_guc_log_relay_close().

Also, it should be impossible to race the start/flush with the stop 
because the relay_write can't race the relay_close, but it might be 
worth a comment in case we decide to have guc_log_relay_stop() callable 
from the debugfs in the future.

>   }
>   
>   void intel_guc_log_handle_flush_event(struct intel_guc_log *log)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
> index 6f764879acb1..ecf7a49416b4 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
> @@ -44,6 +44,7 @@ struct intel_guc;
>   
>   struct intel_guc_log {
>   	u32 level;
> +	bool relay_started;

this should move inside the relay structure below. Just "started" will 
be enough as a name at that point because the structure is already 
called relay.

>   	struct i915_vma *vma;
>   	struct {
>   		void *buf_addr;
> @@ -67,6 +68,7 @@ void intel_guc_log_destroy(struct intel_guc_log *log);
>   int intel_guc_log_set_level(struct intel_guc_log *log, u32 level);
>   bool intel_guc_log_relay_enabled(const struct intel_guc_log *log);

intel_guc_log_relay_enabled() implied both created and started before, 
but now that you're splitting that in 2 separate states you'll need to 
account for that. From a quick look, all the callers seem to just care 
about the buffers being allocated, so a rename should be enough.

>   int intel_guc_log_relay_open(struct intel_guc_log *log);
> +int intel_guc_log_relay_start(struct intel_guc_log *log);
>   void intel_guc_log_relay_flush(struct intel_guc_log *log);
>   void intel_guc_log_relay_close(struct intel_guc_log *log);
>   
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 708855e051b5..c3683fdd0deb 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2049,9 +2049,32 @@ i915_guc_log_relay_write(struct file *filp,
>   			 loff_t *ppos)
>   {
>   	struct intel_guc_log *log = filp->private_data;
> +	char *input_buffer;
> +	int val;
> +	int ret;
>   
> -	intel_guc_log_relay_flush(log);
> -	return cnt;
> +	input_buffer = memdup_user_nul(ubuf, cnt);
> +	if (IS_ERR(input_buffer))
> +		return PTR_ERR(input_buffer);
> +
> +	ret = kstrtoint(input_buffer, 10, &val);
> +	if (ret < 0)
> +		goto out;
> +

you can use kstrtoint_from_user to convert user input to int directly, 
without having to copy to a temp buf.

> +	/*
> +	 * Enable and start the guc log relay on value of 1.
> +	 * Flush log relay for any other value.
> +	 */
> +	if (val == 1) {
> +		ret = intel_guc_log_relay_start(log);
> +	} else {
> +		intel_guc_log_relay_flush(log);
> +		ret = cnt;
> +	}
> +
> +out:
> +	kfree(input_buffer);
> +	return ret;

Probably better to always return cnt in case of success, i.e:

	return ret ?: cnt;

And remove the ret = cnt above.

Daniele

>   }
>   
>   static int i915_guc_log_relay_release(struct inode *inode, struct file *file)
>