[Intel-gfx] [PATCH] drm/i915/guc: Add onion teardown to the GuC setup

[Joonas Lahtinen]

On to, 2017-02-16 at 06:18 -0800, Oscar Mateo wrote:
> Starting with intel_guc_loader, down to intel_guc_submission
> and finally to intel_guc_log.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>

<SNIP>

> -static void guc_client_free(struct i915_guc_client *client)
> +static void guc_client_free(struct i915_guc_client **p_client)
>  {
> +	struct i915_guc_client *client;
> +
> +	client = fetch_and_zero(p_client);
> +	if (!client)
> +		return;
> +

Might be useful to wrap the reminder of this function into
__guc_client_free, which can be called directly. But that can be added
later, when code described by Daniele appears.

> @@ -835,14 +841,11 @@ static void guc_addon_create(struct intel_guc *guc)
>  			sizeof(struct guc_mmio_reg_state) +
>  			GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
>  
> -	vma = guc->ads_vma;
> -	if (!vma) {
> -		vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
> -		if (IS_ERR(vma))
> -			return;
> +	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
> +	if (IS_ERR(vma))
> +		return PTR_ERR(vma);
>  
> -		guc->ads_vma = vma;
> -	}
> +	guc->ads_vma = vma;

Only now realized the connection, may I suggest s/ads_vma/addon_vma/ as
a follow-up patch :P

> @@ -935,14 +954,33 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>  					       dev_priv->kernel_context);
>  	if (IS_ERR(guc->execbuf_client)) {
>  		DRM_ERROR("Failed to create GuC client for execbuf!\n");
> -		goto err;
> +		ret = PTR_ERR(guc->execbuf_client);
> +		goto err_ads;
>  	}
>  
>  	return 0;

\n here to make Chris happy.

> +err_ads:
> +	guc_addon_destroy(guc);
> +err_log:
> +	intel_guc_log_destroy(guc);
> +err_vaddr:
> +	i915_gem_object_unpin_map(guc->ctx_pool->obj);
> +err_vma:
> +	i915_vma_unpin_and_release(&guc->ctx_pool);
>  
> -err:
> -	i915_guc_submission_fini(dev_priv);
> -	return -ENOMEM;
> +	return ret;
> +}

<SNIP>

> -static int guc_log_create_relay_channel(struct intel_guc *guc)
> +static int guc_log_relay_channel_create(struct intel_guc *guc)

If guc_log_relay_channel is going to be a thing, then this is the right
thing to do.

> @@ -172,12 +166,21 @@ static int guc_log_create_relay_channel(struct intel_guc *guc)
>  	return 0;
>  }
>  
> -static int guc_log_create_relay_file(struct intel_guc *guc)
> +static void guc_log_relay_channel_destroy(struct intel_guc *guc)
> +{
> +	relay_close(guc->log.relay_chan);
> +	guc->log.relay_chan = NULL;

If the relay channel is a dynamic thing which gets recreated and
destroyed in runtime, then this is OKish, but if it's only created at
driver init and destroyed at shutdown, then don't bother assigning
NULL.

> +static int guc_log_relay_file_create(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>  	struct dentry *log_dir;
>  	int ret;
>  
> +	if (i915.guc_log_level < 0)
> +		return 0; /* nothing to do */

The comment is not necessary, the check is rather self-explaining.
 
> @@ -198,7 +201,10 @@ static int guc_log_create_relay_file(struct intel_guc *guc)
> >  	}
>  
>  	ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
> -	if (ret) {
> +	if (ret == -EEXIST) {
> +		/* Ignore "file already exists" */

Comment again is redundant. Just;

if (ret < 0 && ret != -EEXISTS)

> +	}
> +	else if (ret < 0) {
>  		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
>  		return ret;
>  	}

<SNIP>

> -static int guc_log_create_extras(struct intel_guc *guc)
> +static int guc_log_extras_create(struct intel_guc *guc)

The naming convention we have is rather difficult, here
guc_log_create_extras would be the right name, as "guc_log" is the
structure, and "create extras" is our verb. If we had "guc_log_extras",
then "guc_log_extras" would be the structure and "create" the verb. But
if you intend to break out guc_log_extras, then it's good.

Although, the purpose of this function sounds more like init_extras.

>  {
>  	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);
>  
> -	/* Nothing to do */
>  	if (i915.guc_log_level < 0)
> -		return 0;
> +		return 0; /* nothing to do */

Don't be shy to nuke such comments.

>  
> -	if (!guc->log.buf_addr) {
> -		/* Create a WC (Uncached for read) vmalloc mapping of log
> -		 * buffer pages, so that we can directly get the data
> -		 * (up-to-date) from memory.
> -		 */
> -		vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
> -		if (IS_ERR(vaddr)) {
> -			ret = PTR_ERR(vaddr);
> -			DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> -			return ret;
> -		}
> +	if (guc->log.buf_addr)
> +		return 0; /* already allocated */

This check can be hoisted to calling function and if you feel like so,
add "guc_has_log_extras" helper. (Comment is redundant again).

Generally speaking, the calls should not be idempotent, so instead of
checking, add GEM_BUG_ON(guc->log.buf_addr); The more "if"s we have,
the harder it's to get good testing coverage.

<SNIP>

> +	guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> +						    WQ_HIGHPRI | WQ_FREEZABLE);
> +	if (guc->log.flush_wq == NULL) {

While around, make it if (!guc->log.flush_wq)

> +		DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
> +		ret = -ENOMEM;
> +		goto err_relaychan;
>  	}
>  
>  	return 0;

\n here and other spots.

> +err_relaychan:
> +	guc_log_relay_channel_destroy(guc);
> +err_vaddr:
> > +	i915_gem_object_unpin_map(guc->log.vma->obj);
> +
> > +	return ret;
>  }

<SNIP>

> @@ -609,17 +615,24 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>  		return ret;
>  	}
>  
> -	i915.guc_log_level = log_param.verbosity;
> +	if (log_param.logging_enabled)

Extra newline here to be removed.

A much wanted improvement. I might move the function renames to
follow-up patches when new structs get introduced. I'd also like some
clarity between "extras", "addon" and "ads" in follow-up :)

With above changes, this patch is looking good to me.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation