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

Oscar Mateo oscar.mateo at intel.com
Thu Feb 16 14:21:23 UTC 2017


This goes on top of "Keep the ctx_pool_vaddr mapped, for easy access", 
which is in turn goes on top of Joonas' "Sanitize GuC client 
initialization". If the reviews go well and once Joonas finishes his 
patch, I can resend everything as a series so that merging is easier.

-- Oscar

On 02/16/2017 06:18 AM, 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>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c |  94 +++++----
>   drivers/gpu/drm/i915/intel_guc_loader.c    |  19 +-
>   drivers/gpu/drm/i915/intel_guc_log.c       | 309 +++++++++++++++--------------
>   drivers/gpu/drm/i915/intel_uc.h            |   5 +-
>   4 files changed, 229 insertions(+), 198 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index f7d9897..4147674 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -609,8 +609,14 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>   	return vma;
>   }
>   
> -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;
> +
>   	/*
>   	 * XXX: wait for any outstanding submissions before freeing memory.
>   	 * Be sure to drop any locks
> @@ -818,7 +824,7 @@ static void guc_policies_init(struct guc_policies *policies)
>   	policies->is_valid = 1;
>   }
>   
> -static void guc_addon_create(struct intel_guc *guc)
> +static int guc_addon_create(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	struct i915_vma *vma;
> @@ -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;
>   
>   	page = i915_vma_first_page(vma);
>   	ads = kmap(page);
> @@ -885,6 +888,13 @@ static void guc_addon_create(struct intel_guc *guc)
>   			sizeof(struct guc_mmio_reg_state);
>   
>   	kunmap(page);
> +
> +	return 0;
> +}
> +
> +static void guc_addon_destroy(struct intel_guc *guc)
> +{
> +	i915_vma_unpin_and_release(&guc->ads_vma);
>   }
>   
>   /*
> @@ -899,6 +909,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>   	struct intel_guc *guc = &dev_priv->guc;
>   	struct i915_vma *vma;
>   	void *vaddr;
> +	int ret;
>   
>   	if (!HAS_GUC_SCHED(dev_priv))
>   		return 0;
> @@ -919,15 +930,23 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>   
>   	guc->ctx_pool = vma;
>   
> -	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> -	if (IS_ERR(vaddr))
> -		goto err;
> +	vaddr = i915_gem_object_pin_map(guc->ctx_pool->obj, I915_MAP_WB);
> +	if (IS_ERR(vaddr)) {
> +		ret = PTR_ERR(vaddr);
> +		goto err_vma;
> +	}
>   
>   	guc->ctx_pool_vaddr = vaddr;
>   
> +	ret = intel_guc_log_create(guc);
> +	if (ret < 0)
> +		goto err_vaddr;
> +
> +	ret = guc_addon_create(guc);
> +	if (ret < 0)
> +		goto err_log;
> +
>   	ida_init(&guc->ctx_ids);
> -	intel_guc_log_create(guc);
> -	guc_addon_create(guc);
>   
>   	guc->execbuf_client = guc_client_alloc(dev_priv,
>   					       INTEL_INFO(dev_priv)->ring_mask,
> @@ -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;
> +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;
> +}
> +
> +void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_guc *guc = &dev_priv->guc;
> +
> +	guc_client_free(&guc->execbuf_client);
> +	ida_destroy(&guc->ctx_ids);
> +	guc_addon_destroy(guc);
> +	intel_guc_log_destroy(guc);
> +	i915_gem_object_unpin_map(guc->ctx_pool->obj);
> +	i915_vma_unpin_and_release(&guc->ctx_pool);
>   }
>   
>   static void guc_reset_wq(struct i915_guc_client *client)
> @@ -1004,26 +1042,6 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
>   	intel_execlists_enable_submission(dev_priv);
>   }
>   
> -void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_guc *guc = &dev_priv->guc;
> -	struct i915_guc_client *client;
> -
> -	client = fetch_and_zero(&guc->execbuf_client);
> -	if (client && !IS_ERR(client))
> -		guc_client_free(client);
> -
> -	i915_vma_unpin_and_release(&guc->ads_vma);
> -	i915_vma_unpin_and_release(&guc->log.vma);
> -
> -	if (guc->ctx_pool_vaddr) {
> -		ida_destroy(&guc->ctx_ids);
> -		i915_gem_object_unpin_map(guc->ctx_pool->obj);
> -	}
> -
> -	i915_vma_unpin_and_release(&guc->ctx_pool);
> -}
> -
>   /**
>    * intel_guc_suspend() - notify GuC entering suspend state
>    * @dev_priv:	i915 device private
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 22f882d..0b48ad8 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -489,7 +489,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>   
>   	err = i915_guc_submission_init(dev_priv);
>   	if (err)
> -		goto fail;
> +		goto err_guc;
>   
>   	/*
>   	 * WaEnableuKernelHeaderValidFix:skl,bxt
> @@ -504,7 +504,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>   		 */
>   		err = guc_hw_reset(dev_priv);
>   		if (err)
> -			goto fail;
> +			goto err_submission;
>   
>   		intel_huc_load(dev_priv);
>   		err = guc_ucode_xfer(dev_priv);
> @@ -512,7 +512,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>   			break;
>   
>   		if (--retries == 0)
> -			goto fail;
> +			goto err_submission;
>   
>   		DRM_INFO("GuC fw load failed: %d; will reset and "
>   			 "retry %d more time(s)\n", err, retries);
> @@ -528,7 +528,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>   
>   		err = i915_guc_submission_enable(dev_priv);
>   		if (err)
> -			goto fail;
> +			goto err_interrupts;
>   		guc_interrupts_capture(dev_priv);
>   	}
>   
> @@ -539,15 +539,16 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>   
>   	return 0;
>   
> +err_interrupts:
> +	gen9_disable_guc_interrupts(dev_priv);
> +err_submission:
> +	i915_guc_submission_fini(dev_priv);
> +err_guc:
> +	i915_ggtt_disable_guc(dev_priv);
>   fail:
>   	if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
>   		guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
>   
> -	guc_interrupts_release(dev_priv);
> -	i915_guc_submission_disable(dev_priv);
> -	i915_guc_submission_fini(dev_priv);
> -	i915_ggtt_disable_guc(dev_priv);
> -
>   	/*
>   	 * We've failed to load the firmware :(
>   	 *
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 5c0f9a4..c1365e6 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -66,7 +66,6 @@ static int guc_log_control(struct intel_guc *guc, u32 control_val)
>   	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>   }
>   
> -
>   /*
>    * 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.
> @@ -139,12 +138,7 @@ static int remove_buf_file_callback(struct dentry *dentry)
>   	.remove_buf_file = remove_buf_file_callback,
>   };
>   
> -static void guc_log_remove_relay_file(struct intel_guc *guc)
> -{
> -	relay_close(guc->log.relay_chan);
> -}
> -
> -static int guc_log_create_relay_channel(struct intel_guc *guc)
> +static int guc_log_relay_channel_create(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	struct rchan *guc_log_relay_chan;
> @@ -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;
> +}
> +
> +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 */
> +
>   	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
>   	log_dir = dev_priv->drm.primary->debugfs_root;
>   
> @@ -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" */
> +	}
> +	else if (ret < 0) {
>   		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
>   		return ret;
>   	}
> @@ -371,31 +377,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   	}
>   }
>   
> -static void guc_log_cleanup(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> -	lockdep_assert_held(&dev_priv->drm.struct_mutex);
> -
> -	/* First disable the flush interrupt */
> -	gen9_disable_guc_interrupts(dev_priv);
> -
> -	if (guc->log.flush_wq)
> -		destroy_workqueue(guc->log.flush_wq);
> -
> -	guc->log.flush_wq = NULL;
> -
> -	if (guc->log.relay_chan)
> -		guc_log_remove_relay_file(guc);
> -
> -	guc->log.relay_chan = NULL;
> -
> -	if (guc->log.buf_addr)
> -		i915_gem_object_unpin_map(guc->log.vma->obj);
> -
> -	guc->log.buf_addr = NULL;
> -}
> -
>   static void capture_logs_work(struct work_struct *work)
>   {
>   	struct intel_guc *guc =
> @@ -404,120 +385,84 @@ static void capture_logs_work(struct work_struct *work)
>   	guc_log_capture_logs(guc);
>   }
>   
> -static int guc_log_create_extras(struct intel_guc *guc)
> +static int guc_log_extras_create(struct intel_guc *guc)
>   {
>   	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 */
>   
> -	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 */
>   
> -		guc->log.buf_addr = vaddr;
> +	/* 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)) {
> +		DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> +		return PTR_ERR(vaddr);
>   	}
>   
> -	if (!guc->log.relay_chan) {
> -		/* Create a relay channel, so that we have buffers for storing
> -		 * the GuC firmware logs, the channel will be linked with a file
> -		 * later on when debugfs is registered.
> -		 */
> -		ret = guc_log_create_relay_channel(guc);
> -		if (ret)
> -			return ret;
> -	}
> +	guc->log.buf_addr = vaddr;
>   
> -	if (!guc->log.flush_wq) {
> -		INIT_WORK(&guc->log.flush_work, capture_logs_work);
> -
> -		 /*
> -		 * GuC log buffer flush work item has to do register access to
> -		 * send the ack to GuC and this work item, if not synced before
> -		 * suspend, can potentially get executed after the GFX device is
> -		 * suspended.
> -		 * By marking the WQ as freezable, we don't have to bother about
> -		 * flushing of this work item from the suspend hooks, the pending
> -		 * work item if any will be either executed before the suspend
> -		 * or scheduled later on resume. This way the handling of work
> -		 * item can be kept same between system suspend & rpm suspend.
> -		 */
> -		guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> -							    WQ_HIGHPRI | WQ_FREEZABLE);
> -		if (guc->log.flush_wq == NULL) {
> -			DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
> -			return -ENOMEM;
> -		}
> +	/* Create a relay channel, so that we have buffers for storing
> +	 * the GuC firmware logs, the channel will be linked with a file
> +	 * later on when debugfs is registered.
> +	 */
> +	ret = guc_log_relay_channel_create(guc);
> +	if (ret < 0)
> +		goto err_vaddr;
> +
> +	INIT_WORK(&guc->log.flush_work, capture_logs_work);
> +
> +	 /*
> +	 * GuC log buffer flush work item has to do register access to
> +	 * send the ack to GuC and this work item, if not synced before
> +	 * suspend, can potentially get executed after the GFX device is
> +	 * suspended.
> +	 * By marking the WQ as freezable, we don't have to bother about
> +	 * flushing of this work item from the suspend hooks, the pending
> +	 * work item if any will be either executed before the suspend
> +	 * or scheduled later on resume. This way the handling of work
> +	 * item can be kept same between system suspend & rpm suspend.
> +	 */
> +	guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> +						    WQ_HIGHPRI | WQ_FREEZABLE);
> +	if (guc->log.flush_wq == NULL) {
> +		DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
> +		ret = -ENOMEM;
> +		goto err_relaychan;
>   	}
>   
>   	return 0;
> +err_relaychan:
> +	guc_log_relay_channel_destroy(guc);
> +err_vaddr:
> +	i915_gem_object_unpin_map(guc->log.vma->obj);
> +
> +	return ret;
>   }
>   
> -void intel_guc_log_create(struct intel_guc *guc)
> +static void guc_log_extras_destroy(struct intel_guc *guc)
>   {
> -	struct i915_vma *vma;
> -	unsigned long offset;
> -	uint32_t size, flags;
> -
> -	if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
> -		i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
> -
> -	/* The first page is to save log buffer state. Allocate one
> -	 * extra page for others in case for overlap */
> -	size = (1 + GUC_LOG_DPC_PAGES + 1 +
> -		GUC_LOG_ISR_PAGES + 1 +
> -		GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
> -
> -	vma = guc->log.vma;
> -	if (!vma) {
> -		/* We require SSE 4.1 for fast reads from the GuC log buffer and
> -		 * it should be present on the chipsets supporting GuC based
> -		 * submisssions.
> -		 */
> -		if (WARN_ON(!i915_has_memcpy_from_wc())) {
> -			/* logging will not be enabled */
> -			i915.guc_log_level = -1;
> -			return;
> -		}
> -
> -		vma = intel_guc_allocate_vma(guc, size);
> -		if (IS_ERR(vma)) {
> -			/* logging will be off */
> -			i915.guc_log_level = -1;
> -			return;
> -		}
> -
> -		guc->log.vma = vma;
> -
> -		if (guc_log_create_extras(guc)) {
> -			guc_log_cleanup(guc);
> -			i915_vma_unpin_and_release(&guc->log.vma);
> -			i915.guc_log_level = -1;
> -			return;
> -		}
> -	}
> -
> -	/* each allocated unit is a page */
> -	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
> -		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
> -		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
> -		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
> +	/*
> +	 * It's possible that extras were never allocated because guc_log_level
> +	 * was < 0 at the time
> +	 **/
> +	if (!guc->log.buf_addr)
> +		return;
>   
> -	offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
> -	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
> +	destroy_workqueue(guc->log.flush_wq);
> +	guc->log.flush_wq = NULL;
> +	guc_log_relay_channel_destroy(guc);
> +	i915_gem_object_unpin_map(guc->log.vma->obj);
> +	guc->log.buf_addr = NULL;
>   }
>   
>   static int guc_log_late_setup(struct intel_guc *guc)
> @@ -527,26 +472,25 @@ static int guc_log_late_setup(struct intel_guc *guc)
>   
>   	lockdep_assert_held(&dev_priv->drm.struct_mutex);
>   
> -	if (i915.guc_log_level < 0)
> -		return -EINVAL;
> -
>   	/* If log_level was set as -1 at boot time, then setup needed to
>   	 * handle log buffer flush interrupts would not have been done yet,
>   	 * so do that now.
>   	 */
> -	ret = guc_log_create_extras(guc);
> +	ret = guc_log_extras_create(guc);
>   	if (ret)
>   		goto err;
>   
> -	ret = guc_log_create_relay_file(guc);
> +	ret = guc_log_relay_file_create(guc);
>   	if (ret)
> -		goto err;
> +		goto err_extras;
>   
>   	return 0;
> +err_extras:
> +	guc_log_extras_destroy(guc);
>   err:
> -	guc_log_cleanup(guc);
>   	/* logging will remain off */
>   	i915.guc_log_level = -1;
> +
>   	return ret;
>   }
>   
> @@ -586,6 +530,68 @@ static void guc_flush_logs(struct intel_guc *guc)
>   	guc_log_capture_logs(guc);
>   }
>   
> +int intel_guc_log_create(struct intel_guc *guc)
> +{
> +	struct i915_vma *vma;
> +	unsigned long offset;
> +	uint32_t size, flags;
> +	int ret;
> +
> +	if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
> +		i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
> +
> +	/* The first page is to save log buffer state. Allocate one
> +	 * extra page for others in case for overlap */
> +	size = (1 + GUC_LOG_DPC_PAGES + 1 +
> +		GUC_LOG_ISR_PAGES + 1 +
> +		GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
> +
> +	/* We require SSE 4.1 for fast reads from the GuC log buffer and
> +	 * it should be present on the chipsets supporting GuC based
> +	 * submisssions.
> +	 */
> +	if (WARN_ON(!i915_has_memcpy_from_wc())) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	vma = intel_guc_allocate_vma(guc, size);
> +	if (IS_ERR(vma)) {
> +		ret = PTR_ERR(vma);
> +		goto err;
> +	}
> +
> +	guc->log.vma = vma;
> +
> +	ret = guc_log_extras_create(guc);
> +	if (ret < 0)
> +		goto err_vma;
> +
> +	/* each allocated unit is a page */
> +	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
> +		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
> +		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
> +		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
> +
> +	offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
> +	guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
> +
> +	return 0;
> +err_vma:
> +	i915_vma_unpin_and_release(&guc->log.vma);
> +err:
> +	/* logging will be off */
> +	i915.guc_log_level = -1;
> +
> +	return ret;
> +}
> +
> +void intel_guc_log_destroy(struct intel_guc *guc)
> +{
> +	guc_log_extras_destroy(guc);
> +	i915_vma_unpin_and_release(&guc->log.vma);
> +}
> +
>   int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>   {
>   	struct intel_guc *guc = &dev_priv->guc;
> @@ -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)
> +	{
> +		i915.guc_log_level = log_param.verbosity;
>   
> -	/* If log_level was set as -1 at boot time, then the relay channel file
> -	 * wouldn't have been created by now and interrupts also would not have
> -	 * been enabled.
> -	 */
> -	if (!dev_priv->guc.log.relay_chan) {
> +		/* If log_level was set as -1 at boot time, then the relay channel file
> +		 * wouldn't have been created by now and interrupts also would not have
> +		 * been enabled. Try again now, just in case.
> +		 */
>   		ret = guc_log_late_setup(guc);
> -		if (!ret)
> -			gen9_enable_guc_interrupts(dev_priv);
> -	} else if (!log_param.logging_enabled) {
> +		if (ret < 0) {
> +			DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
> +			return ret;
> +		}
> +
> +		gen9_enable_guc_interrupts(dev_priv);
> +	}
> +	else
> +	{
>   		/* Once logging is disabled, GuC won't generate logs & send an
>   		 * interrupt. But there could be some data in the log buffer
>   		 * which is yet to be captured. So request GuC to update the log
> @@ -629,9 +642,6 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>   
>   		/* As logging is disabled, update log level to reflect that */
>   		i915.guc_log_level = -1;
> -	} else {
> -		/* In case interrupts were disabled, enable them now */
> -		gen9_enable_guc_interrupts(dev_priv);
>   	}
>   
>   	return ret;
> @@ -653,6 +663,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>   		return;
>   
>   	mutex_lock(&dev_priv->drm.struct_mutex);
> -	guc_log_cleanup(&dev_priv->guc);
> +	gen9_disable_guc_interrupts(dev_priv);
> +	intel_guc_log_destroy(&dev_priv->guc);
>   	mutex_unlock(&dev_priv->drm.struct_mutex);
>   }
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 511b96b..f34448b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -217,10 +217,11 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>   struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
>   
>   /* intel_guc_log.c */
> -void intel_guc_log_create(struct intel_guc *guc);
> +int intel_guc_log_create(struct intel_guc *guc);
> +void intel_guc_log_destroy(struct intel_guc *guc);
> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
>   void i915_guc_log_register(struct drm_i915_private *dev_priv);
>   void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
> -int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
>   
>   static inline u32 guc_ggtt_offset(struct i915_vma *vma)
>   {



More information about the Intel-gfx mailing list