[PATCH 0/4] Various improvements around the GuC topic

[PATCH 0/4] Various improvements around the GuC topic

[Oscar Mateo]

They have been discussed in various threads, but I am putting them together
as a series for clarity. These go on top of Joonas' "Sanitize GuC client
initialization", so they cannot be merged until that patch goes thru (at
that point, it is probably better if Joonas resends these as well).

Oscar Mateo (4):
  drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access
  drm/i915/guc: Add onion teardown to the GuC setup
  drm/i915/guc: s/ads_vma/addon
  drm/i915/guc: Break out the GuC log "extras"

 drivers/gpu/drm/i915/i915_drv.c            |   7 +-
 drivers/gpu/drm/i915/i915_guc_submission.c | 181 +++++++-------
 drivers/gpu/drm/i915/i915_irq.c            |   4 +-
 drivers/gpu/drm/i915/intel_guc_loader.c    |  38 +--
 drivers/gpu/drm/i915/intel_guc_log.c       | 382 ++++++++++++++---------------
 drivers/gpu/drm/i915/intel_uc.h            |  25 +-
 6 files changed, 331 insertions(+), 306 deletions(-)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4 v3] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access

[Oscar Mateo]

The GuC descriptor is big in size. If we use a local definition of
guc_desc we have a chance to overflow stack, so avoid it.

Also, Chris abhors scatterlists :)

v2: Rebased, helper function to retrieve the context descriptor,
s/ctx_pool_vma/ctx_pool/

v3: Zero out guc_context_desc before initialization

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 95 +++++++++++++++---------------
 drivers/gpu/drm/i915/intel_guc_loader.c    |  2 +-
 drivers/gpu/drm/i915/intel_uc.h            |  3 +-
 3 files changed, 49 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index ed8d265..d5847e8 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -134,6 +134,12 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
+static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
+{
+	return client->guc->ctx_pool_vaddr +
+		sizeof(struct guc_context_desc) * client->ctx_index;
+}
+
 /*
  * Initialise, update, or clear doorbell data shared with the GuC
  *
@@ -143,21 +149,11 @@ static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 ctx_index)
 
 static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
 {
-	struct sg_table *sg = client->guc->ctx_pool_vma->pages;
-	struct guc_context_desc desc;
-	size_t len;
+	struct guc_context_desc *desc;
 
 	/* Update the GuC's idea of the doorbell ID */
-	len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
-				 sizeof(desc) * client->ctx_index);
-	if (len != sizeof(desc))
-		return -EFAULT;
-
-	desc.db_id = new_id;
-	len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
-				   sizeof(desc) * client->ctx_index);
-	if (len != sizeof(desc))
-		return -EFAULT;
+	desc = __get_context_desc(client);
+	desc->db_id = new_id;
 
 	return 0;
 }
@@ -270,29 +266,28 @@ static void guc_proc_desc_init(struct intel_guc *guc,
  * data structures relating to this client (doorbell, process descriptor,
  * write queue, etc).
  */
-
 static void guc_ctx_desc_init(struct intel_guc *guc,
 			      struct i915_guc_client *client)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	struct intel_engine_cs *engine;
 	struct i915_gem_context *ctx = client->owner;
-	struct guc_context_desc desc;
-	struct sg_table *sg;
+	struct guc_context_desc *desc;
 	unsigned int tmp;
 	u32 gfx_addr;
 
-	memset(&desc, 0, sizeof(desc));
+	desc = __get_context_desc(client);
+	memset(desc, 0, sizeof(*desc));
 
-	desc.attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
-	desc.context_id = client->ctx_index;
-	desc.priority = client->priority;
-	desc.db_id = client->doorbell_id;
+	desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
+	desc->context_id = client->ctx_index;
+	desc->priority = client->priority;
+	desc->db_id = client->doorbell_id;
 
 	for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
 		struct intel_context *ce = &ctx->engine[engine->id];
 		uint32_t guc_engine_id = engine->guc_id;
-		struct guc_execlist_context *lrc = &desc.lrc[guc_engine_id];
+		struct guc_execlist_context *lrc = &desc->lrc[guc_engine_id];
 
 		/* TODO: We have a design issue to be solved here. Only when we
 		 * receive the first batch, we know which engine is used by the
@@ -317,50 +312,41 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
 		lrc->ring_next_free_location = lrc->ring_begin;
 		lrc->ring_current_tail_pointer_value = 0;
 
-		desc.engines_used |= (1 << guc_engine_id);
+		desc->engines_used |= (1 << guc_engine_id);
 	}
 
 	DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
-			client->engines, desc.engines_used);
-	WARN_ON(desc.engines_used == 0);
+			client->engines, desc->engines_used);
+	WARN_ON(desc->engines_used == 0);
 
 	/*
 	 * The doorbell, process descriptor, and workqueue are all parts
 	 * of the client object, which the GuC will reference via the GGTT
 	 */
 	gfx_addr = guc_ggtt_offset(client->vma);
-	desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+	desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
 				client->doorbell_offset;
-	desc.db_trigger_cpu =
-		(uintptr_t)client->vaddr + client->doorbell_offset;
-	desc.db_trigger_uk = gfx_addr + client->doorbell_offset;
-	desc.process_desc = gfx_addr + client->proc_desc_offset;
-	desc.wq_addr = gfx_addr + client->wq_offset;
-	desc.wq_size = client->wq_size;
+	desc->db_trigger_cpu = (uintptr_t)client->vaddr +
+				client->doorbell_offset;
+	desc->db_trigger_uk = gfx_addr + client->doorbell_offset;
+	desc->process_desc = gfx_addr + client->proc_desc_offset;
+	desc->wq_addr = gfx_addr + client->wq_offset;
+	desc->wq_size = client->wq_size;
 
 	/*
 	 * XXX: Take LRCs from an existing context if this is not an
 	 * IsKMDCreatedContext client
 	 */
-	desc.desc_private = (uintptr_t)client;
-
-	/* Pool context is pinned already */
-	sg = guc->ctx_pool_vma->pages;
-	sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
-			     sizeof(desc) * client->ctx_index);
+	desc->desc_private = (uintptr_t)client;
 }
 
 static void guc_ctx_desc_fini(struct intel_guc *guc,
 			      struct i915_guc_client *client)
 {
-	struct guc_context_desc desc;
-	struct sg_table *sg;
+	struct guc_context_desc *desc;
 
-	memset(&desc, 0, sizeof(desc));
-
-	sg = guc->ctx_pool_vma->pages;
-	sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
-			     sizeof(desc) * client->ctx_index);
+	desc = __get_context_desc(client);
+	memset(desc, 0, sizeof(*desc));
 }
 
 /**
@@ -915,6 +901,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	const size_t gemsize = round_up(poolsize, PAGE_SIZE);
 	struct intel_guc *guc = &dev_priv->guc;
 	struct i915_vma *vma;
+	void *vaddr;
 
 	if (!HAS_GUC_SCHED(dev_priv))
 		return 0;
@@ -926,14 +913,21 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	if (!i915.enable_guc_submission)
 		return 0; /* not enabled  */
 
-	if (guc->ctx_pool_vma)
+	if (guc->ctx_pool)
 		return 0; /* already allocated */
 
 	vma = intel_guc_allocate_vma(guc, gemsize);
 	if (IS_ERR(vma))
 		return PTR_ERR(vma);
 
-	guc->ctx_pool_vma = vma;
+	guc->ctx_pool = vma;
+
+	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+	if (IS_ERR(vaddr))
+		goto err;
+
+	guc->ctx_pool_vaddr = vaddr;
+
 	ida_init(&guc->ctx_ids);
 	intel_guc_log_create(guc);
 	guc_addon_create(guc);
@@ -1025,9 +1019,12 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
 	i915_vma_unpin_and_release(&guc->ads_vma);
 	i915_vma_unpin_and_release(&guc->log.vma);
 
-	if (guc->ctx_pool_vma)
+	if (guc->ctx_pool_vaddr) {
 		ida_destroy(&guc->ctx_ids);
-	i915_vma_unpin_and_release(&guc->ctx_pool_vma);
+		i915_gem_object_unpin_map(guc->ctx_pool->obj);
+	}
+
+	i915_vma_unpin_and_release(&guc->ctx_pool);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 9885f76..22f882d 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -221,7 +221,7 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
 
 	/* If GuC submission is enabled, set up additional parameters here */
 	if (i915.enable_guc_submission) {
-		u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool_vma);
+		u32 pgs = guc_ggtt_offset(dev_priv->guc.ctx_pool);
 		u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
 
 		pgs >>= PAGE_SHIFT;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index c80f470..511b96b 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -153,7 +153,8 @@ struct intel_guc {
 	bool interrupts_enabled;
 
 	struct i915_vma *ads_vma;
-	struct i915_vma *ctx_pool_vma;
+	struct i915_vma *ctx_pool;
+	void *ctx_pool_vaddr;
 	struct ida ctx_ids;
 
 	struct i915_guc_client *execbuf_client;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4 v3] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access

[Joonas Lahtinen]

On pe, 2017-02-24 at 06:01 -0800, Oscar Mateo wrote:
> The GuC descriptor is big in size. If we use a local definition of
> guc_desc we have a chance to overflow stack, so avoid it.
> 
> Also, Chris abhors scatterlists :)
> 
> v2: Rebased, helper function to retrieve the context descriptor,
> s/ctx_pool_vma/ctx_pool/
> 
> v3: Zero out guc_context_desc before initialization
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

The first three patches would be rather ready for merging, right?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4 v3] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access

[Oscar Mateo]

On 03/02/2017 02:41 AM, Joonas Lahtinen wrote:
> On pe, 2017-02-24 at 06:01 -0800, Oscar Mateo wrote:
>> The GuC descriptor is big in size. If we use a local definition of
>> guc_desc we have a chance to overflow stack, so avoid it.
>>
>> Also, Chris abhors scatterlists :)
>>
>> v2: Rebased, helper function to retrieve the context descriptor,
>> s/ctx_pool_vma/ctx_pool/
>>
>> v3: Zero out guc_context_desc before initialization
>>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> The first three patches would be rather ready for merging, right?
>
> Regards, Joonas
Hmmmm... not really :(
They sit on top of your patch with the reworked doorbells, so there will 
be merge conflicts. Also, your comment regarding the log extras really 
belongs to the third patch (and while playing with GuC I have found 
another problem with this third patch).
I believe I'll meet you IRL/AFK, so we can agree on a merge strategy 
then :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4 v2] drm/i915/guc: Add onion teardown to the GuC setup

[Oscar Mateo]

Starting with intel_guc_loader, down to intel_guc_submission
and finally to intel_guc_log.

v2:
  - Null execbuf client outside guc_client_free (Daniele)
  - Assert if things try to get allocated twice (Daniele/Joonas)
  - Null guc->log.buf_addr when destroyed (Daniele)
  - Newline between returning success and error labels (Joonas)
  - Remove some unnecessary comments (Joonas)
  - Keep guc_log_create_extras naming convention (Joonas)
  - Helper function guc_log_has_extras (Joonas)
  - No need for separate relay_channel create/destroy. It's just another extra.
  - No need to nullify guc->log.flush_wq when destroyed (Joonas)
  - Hoist the check for has_extras out of guc_log_create_extras (Joonas)
  - Try to do i915_guc_log_register/unregister calls (kind of) symmetric (Daniele)
  - Make sure initel_guc_fini is not called before init is ever called (Daniele)

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |   7 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |  96 ++++----
 drivers/gpu/drm/i915/intel_guc_loader.c    |  32 +--
 drivers/gpu/drm/i915/intel_guc_log.c       | 360 ++++++++++++++---------------
 drivers/gpu/drm/i915/intel_uc.h            |   8 +-
 5 files changed, 262 insertions(+), 241 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b76e8f7..0c2ee5f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -551,6 +551,8 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 static void i915_gem_fini(struct drm_i915_private *dev_priv)
 {
 	mutex_lock(&dev_priv->drm.struct_mutex);
+	if (i915.enable_guc_submission)
+		intel_guc_cleanup(dev_priv);
 	i915_gem_cleanup_engines(dev_priv);
 	i915_gem_context_fini(dev_priv);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
@@ -614,7 +616,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	ret = i915_gem_init(dev_priv);
 	if (ret)
-		goto cleanup_irq;
+		goto cleanup_uc;
 
 	intel_modeset_gem_init(dev);
 
@@ -636,9 +638,10 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (i915_gem_suspend(dev_priv))
 		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
 	i915_gem_fini(dev_priv);
-cleanup_irq:
+cleanup_uc:
 	intel_guc_fini(dev_priv);
 	intel_huc_fini(dev_priv);
+cleanup_irq:
 	drm_irq_uninstall(dev);
 	intel_teardown_gmbus(dev_priv);
 cleanup_csr:
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d5847e8..7562343c 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -699,6 +699,7 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 		goto err_update;
 
 	return 0;
+
 err_reserve:
 	__unreserve_doorbell(client);
 err_update:
@@ -789,13 +790,13 @@ static int guc_init_doorbell_hw(struct intel_guc *guc)
 			 client->doorbell_id, client->doorbell_offset);
 
 	return client;
+
 err_vma:
 	i915_vma_unpin_and_release(&client->vma);
 err_id:
 	ida_simple_remove(&guc->ctx_ids, client->ctx_index);
 err_client:
 	kfree(client);
-
 	return ERR_PTR(ret);
 }
 
@@ -821,7 +822,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;
@@ -833,19 +834,18 @@ static void guc_addon_create(struct intel_guc *guc)
 	struct page *page;
 	u32 size;
 
+	GEM_BUG_ON(guc->ads_vma);
+
 	/* The ads obj includes the struct itself and buffers passed to GuC */
 	size = sizeof(struct guc_ads) + sizeof(struct guc_policies) +
 			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);
@@ -888,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);
 }
 
 /*
@@ -902,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;
@@ -911,10 +919,10 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	i915_guc_submission_disable(dev_priv);
 
 	if (!i915.enable_guc_submission)
-		return 0; /* not enabled  */
+		return 0;
 
 	if (guc->ctx_pool)
-		return 0; /* already allocated */
+		return 0;
 
 	vma = intel_guc_allocate_vma(guc, gemsize);
 	if (IS_ERR(vma))
@@ -922,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,
@@ -938,14 +954,34 @@ 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:
-	i915_guc_submission_fini(dev_priv);
-	return -ENOMEM;
+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);
+	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);
+	guc->execbuf_client = NULL;
+	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)
@@ -1007,26 +1043,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..1f9ec54 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 :(
 	 *
@@ -587,6 +588,13 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
+void intel_guc_cleanup(struct drm_i915_private *dev_priv)
+{
+	gen9_disable_guc_interrupts(dev_priv);
+	i915_guc_submission_fini(dev_priv);
+	i915_ggtt_disable_guc(dev_priv);
+}
+
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 			 struct intel_uc_fw *uc_fw)
 {
@@ -792,12 +800,6 @@ void intel_guc_fini(struct drm_i915_private *dev_priv)
 	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
 	struct drm_i915_gem_object *obj;
 
-	mutex_lock(&dev_priv->drm.struct_mutex);
-	guc_interrupts_release(dev_priv);
-	i915_guc_submission_disable(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-
 	obj = fetch_and_zero(&guc_fw->obj);
 	if (obj)
 		i915_gem_object_put(obj);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 5c0f9a4..d0632fc 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,45 +138,15 @@ 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)
-{
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct rchan *guc_log_relay_chan;
-	size_t n_subbufs, subbuf_size;
-
-	/* Keep the size of sub buffers same as shared log buffer */
-	subbuf_size = guc->log.vma->obj->base.size;
-
-	/* Store up to 8 snapshots, which is large enough to buffer sufficient
-	 * boot time logs and provides enough leeway to User, in terms of
-	 * latency, for consuming the logs from relay. Also doesn't take
-	 * up too much memory.
-	 */
-	n_subbufs = 8;
-
-	guc_log_relay_chan = relay_open(NULL, NULL, subbuf_size,
-					n_subbufs, &relay_callbacks, dev_priv);
-	if (!guc_log_relay_chan) {
-		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
-		return -ENOMEM;
-	}
-
-	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
-	guc->log.relay_chan = guc_log_relay_chan;
-	return 0;
-}
-
-static int guc_log_create_relay_file(struct intel_guc *guc)
+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;
+
 	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
 	log_dir = dev_priv->drm.primary->debugfs_root;
 
@@ -198,7 +167,7 @@ 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 < 0 && ret != -EEXIST) {
 		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
 		return ret;
 	}
@@ -371,31 +340,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 +348,105 @@ static void capture_logs_work(struct work_struct *work)
 	guc_log_capture_logs(guc);
 }
 
+static bool guc_log_has_extras(struct intel_guc *guc)
+{
+	return (guc->log.buf_addr != NULL);
+}
+
 static int guc_log_create_extras(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	void *vaddr;
-	int ret;
+	struct rchan *guc_log_relay_chan;
+	size_t n_subbufs, subbuf_size;
+	int ret = 0;
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 
-	/* Nothing to do */
-	if (i915.guc_log_level < 0)
-		return 0;
-
-	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;
-		}
-
-		guc->log.buf_addr = vaddr;
-	}
+	GEM_BUG_ON(guc_log_has_extras(guc));
 
-	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;
+	/* 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.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;
-		}
-	}
+	guc->log.buf_addr = vaddr;
 
-	return 0;
-}
+	 /* Keep the size of sub buffers same as shared log buffer */
+	subbuf_size = guc->log.vma->obj->base.size;
 
-void intel_guc_log_create(struct intel_guc *guc)
-{
-	struct i915_vma *vma;
-	unsigned long offset;
-	uint32_t size, flags;
+	/* Store up to 8 snapshots, which is large enough to buffer sufficient
+	 * boot time logs and provides enough leeway to User, in terms of
+	 * latency, for consuming the logs from relay. Also doesn't take
+	 * up too much memory.
+	 */
+	n_subbufs = 8;
 
-	if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
-		i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
+	/* 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.
+	 */
+	guc_log_relay_chan = relay_open(NULL, NULL, subbuf_size,
+					n_subbufs, &relay_callbacks, dev_priv);
+	if (!guc_log_relay_chan) {
+		DRM_ERROR("Couldn't create relay chan for GuC logging\n");
 
-	/* 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;
+		ret = -ENOMEM;
+		goto err_vaddr;
+	}
 
-	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;
-		}
+	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
+	guc->log.relay_chan = guc_log_relay_chan;
 
-		vma = intel_guc_allocate_vma(guc, size);
-		if (IS_ERR(vma)) {
-			/* logging will be off */
-			i915.guc_log_level = -1;
-			return;
-		}
+	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) {
+		DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
+		ret = -ENOMEM;
+		goto err_relaychan;
+	}
 
-		guc->log.vma = vma;
+	return 0;
 
-		if (guc_log_create_extras(guc)) {
-			guc_log_cleanup(guc);
-			i915_vma_unpin_and_release(&guc->log.vma);
-			i915.guc_log_level = -1;
-			return;
-		}
-	}
+err_relaychan:
+	relay_close(guc->log.relay_chan);
+err_vaddr:
+	i915_gem_object_unpin_map(guc->log.vma->obj);
+	guc->log.buf_addr = NULL;
+	return ret;
+}
 
-	/* 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);
+static void guc_log_destroy_extras(struct intel_guc *guc)
+{
+	/*
+	 * It's possible that extras were never allocated because guc_log_level
+	 * was < 0 at the time
+	 **/
+	if (!guc_log_has_extras(guc))
+		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);
+	relay_close(guc->log.relay_chan);
+	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,24 +456,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);
-	if (ret)
-		goto err;
+	if (!guc_log_has_extras(guc)) {
+		/* 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);
+		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_destroy_extras(guc);
 err:
-	guc_log_cleanup(guc);
 	/* logging will remain off */
 	i915.guc_log_level = -1;
 	return ret;
@@ -586,6 +516,72 @@ 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;
+
+	GEM_BUG_ON(guc->log.vma);
+
+	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;
+
+	if (i915.guc_log_level >= 0) {
+		ret = guc_log_create_extras(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_destroy_extras(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 +605,21 @@ 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 +629,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 +650,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);
+	guc_log_destroy_extras(&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..330d08f 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -198,8 +198,9 @@ static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 l
 
 /* intel_guc_loader.c */
 extern void intel_guc_init(struct drm_i915_private *dev_priv);
-extern int intel_guc_setup(struct drm_i915_private *dev_priv);
 extern void intel_guc_fini(struct drm_i915_private *dev_priv);
+extern int intel_guc_setup(struct drm_i915_private *dev_priv);
+extern void intel_guc_cleanup(struct drm_i915_private *dev_priv);
 extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
 extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
 extern int intel_guc_resume(struct drm_i915_private *dev_priv);
@@ -217,10 +218,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)
 {
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4 v2] drm/i915/guc: Add onion teardown to the GuC setup

[Joonas Lahtinen]

On pe, 2017-02-24 at 06:01 -0800, Oscar Mateo wrote:
> Starting with intel_guc_loader, down to intel_guc_submission
> and finally to intel_guc_log.
> 
> v2:
>   - Null execbuf client outside guc_client_free (Daniele)
>   - Assert if things try to get allocated twice (Daniele/Joonas)
>   - Null guc->log.buf_addr when destroyed (Daniele)
>   - Newline between returning success and error labels (Joonas)
>   - Remove some unnecessary comments (Joonas)
>   - Keep guc_log_create_extras naming convention (Joonas)
>   - Helper function guc_log_has_extras (Joonas)
>   - No need for separate relay_channel create/destroy. It's just another extra.
>   - No need to nullify guc->log.flush_wq when destroyed (Joonas)
>   - Hoist the check for has_extras out of guc_log_create_extras (Joonas)
>   - Try to do i915_guc_log_register/unregister calls (kind of) symmetric (Daniele)
>   - Make sure initel_guc_fini is not called before init is ever called (Daniele)
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

<SNIP>

> @@ -404,120 +348,105 @@ static void capture_logs_work(struct work_struct *work)
>  	guc_log_capture_logs(guc);
>  }
>  
> +static bool guc_log_has_extras(struct intel_guc *guc)
> +{
> +	return (guc->log.buf_addr != NULL);
> +}

brackets not needed.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915/guc: s/ads_vma/addon

[Oscar Mateo]

This vma contains much more than just the additional data struct (ads)
and since we were already using the word "addon" as an object in
guc_addon_create, make it the preffered one. No need for the vma suffix
either, as that dependency is given by the type.

while at it, add an explanation of what things go inside the addon object.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 12 ++++++++----
 drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++--
 drivers/gpu/drm/i915/intel_uc.h            |  2 +-
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 7562343c..e1922fe 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -834,9 +834,13 @@ static int guc_addon_create(struct intel_guc *guc)
 	struct page *page;
 	u32 size;
 
-	GEM_BUG_ON(guc->ads_vma);
+	GEM_BUG_ON(guc->addon);
 
-	/* The ads obj includes the struct itself and buffers passed to GuC */
+	/* The additional data struct (ADS) has pointers for different buffers
+	 * used by the GuC. The addon object contains the ADS itself (guc_ads),
+	 * the scheduling policies (guc_policies), a structure describing
+	 * a collection of register sets (guc_mmio_reg_state) and some extra
+	 * pages for the GuC to save its internal state for sleep */
 	size = sizeof(struct guc_ads) + sizeof(struct guc_policies) +
 			sizeof(struct guc_mmio_reg_state) +
 			GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
@@ -845,7 +849,7 @@ static int guc_addon_create(struct intel_guc *guc)
 	if (IS_ERR(vma))
 		return PTR_ERR(vma);
 
-	guc->ads_vma = vma;
+	guc->addon = vma;
 
 	page = i915_vma_first_page(vma);
 	ads = kmap(page);
@@ -894,7 +898,7 @@ static int guc_addon_create(struct intel_guc *guc)
 
 static void guc_addon_destroy(struct intel_guc *guc)
 {
-	i915_vma_unpin_and_release(&guc->ads_vma);
+	i915_vma_unpin_and_release(&guc->addon);
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 1f9ec54..1eb0c51 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -213,8 +213,8 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
 	} else
 		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
 
-	if (guc->ads_vma) {
-		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
+	if (guc->addon) {
+		u32 ads = guc_ggtt_offset(guc->addon) >> PAGE_SHIFT;
 		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
 		params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
 	}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 330d08f..d8897b5 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -152,7 +152,7 @@ struct intel_guc {
 	/* intel_guc_recv interrupt related state */
 	bool interrupts_enabled;
 
-	struct i915_vma *ads_vma;
+	struct i915_vma *addon;
 	struct i915_vma *ctx_pool;
 	void *ctx_pool_vaddr;
 	struct ida ctx_ids;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/guc: s/ads_vma/addon

[Joonas Lahtinen]

On pe, 2017-02-24 at 06:01 -0800, Oscar Mateo wrote:
> This vma contains much more than just the additional data struct (ads)
> and since we were already using the word "addon" as an object in
> guc_addon_create, make it the preffered one. No need for the vma suffix
> either, as that dependency is given by the type.
> 
> while at it, add an explanation of what things go inside the addon object.
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

<SNIP>

> @@ -834,9 +834,13 @@ static int guc_addon_create(struct intel_guc *guc)
>  	struct page *page;
>  	u32 size;
>  
> -	GEM_BUG_ON(guc->ads_vma);
> +	GEM_BUG_ON(guc->addon);
>  
> -	/* The ads obj includes the struct itself and buffers passed to GuC */
> +	/* The additional data struct (ADS) has pointers for different buffers
> +	 * used by the GuC. The addon object contains the ADS itself (guc_ads),
> +	 * the scheduling policies (guc_policies), a structure describing
> +	 * a collection of register sets (guc_mmio_reg_state) and some extra
> +	 * pages for the GuC to save its internal state for sleep */

Put this comment in the header and this is;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

>  	size = sizeof(struct guc_ads) + sizeof(struct guc_policies) +
>  			sizeof(struct guc_mmio_reg_state) +
>  			GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;

-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/guc: s/ads_vma/addon

[Michal Wajdeczko]

On Fri, Feb 24, 2017 at 06:01:36AM -0800, Oscar Mateo wrote:
> This vma contains much more than just the additional data struct (ads)
> and since we were already using the word "addon" as an object in
> guc_addon_create, make it the preffered one. No need for the vma suffix
> either, as that dependency is given by the type.

Hmm, but almost all over the driver we're using "vma" as a name or suffix.
Why do you want to change this here ?

Also, GUC still is using term "ADS" for this blob so I'm not convinced
that we need new name here.

-Michal


> 
> while at it, add an explanation of what things go inside the addon object.
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 12 ++++++++----
>  drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++--
>  drivers/gpu/drm/i915/intel_uc.h            |  2 +-
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 7562343c..e1922fe 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -834,9 +834,13 @@ static int guc_addon_create(struct intel_guc *guc)
>  	struct page *page;
>  	u32 size;
>  
> -	GEM_BUG_ON(guc->ads_vma);
> +	GEM_BUG_ON(guc->addon);
>  
> -	/* The ads obj includes the struct itself and buffers passed to GuC */
> +	/* The additional data struct (ADS) has pointers for different buffers
> +	 * used by the GuC. The addon object contains the ADS itself (guc_ads),
> +	 * the scheduling policies (guc_policies), a structure describing
> +	 * a collection of register sets (guc_mmio_reg_state) and some extra
> +	 * pages for the GuC to save its internal state for sleep */
>  	size = sizeof(struct guc_ads) + sizeof(struct guc_policies) +
>  			sizeof(struct guc_mmio_reg_state) +
>  			GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
> @@ -845,7 +849,7 @@ static int guc_addon_create(struct intel_guc *guc)
>  	if (IS_ERR(vma))
>  		return PTR_ERR(vma);
>  
> -	guc->ads_vma = vma;
> +	guc->addon = vma;
>  
>  	page = i915_vma_first_page(vma);
>  	ads = kmap(page);
> @@ -894,7 +898,7 @@ static int guc_addon_create(struct intel_guc *guc)
>  
>  static void guc_addon_destroy(struct intel_guc *guc)
>  {
> -	i915_vma_unpin_and_release(&guc->ads_vma);
> +	i915_vma_unpin_and_release(&guc->addon);
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 1f9ec54..1eb0c51 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -213,8 +213,8 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
>  	} else
>  		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
>  
> -	if (guc->ads_vma) {
> -		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
> +	if (guc->addon) {
> +		u32 ads = guc_ggtt_offset(guc->addon) >> PAGE_SHIFT;
>  		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
>  		params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 330d08f..d8897b5 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -152,7 +152,7 @@ struct intel_guc {
>  	/* intel_guc_recv interrupt related state */
>  	bool interrupts_enabled;
>  
> -	struct i915_vma *ads_vma;
> +	struct i915_vma *addon;
>  	struct i915_vma *ctx_pool;
>  	void *ctx_pool_vaddr;
>  	struct ida ctx_ids;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/guc: s/ads_vma/addon

[Oscar Mateo]

On 03/07/2017 07:07 AM, Michal Wajdeczko wrote:
> On Fri, Feb 24, 2017 at 06:01:36AM -0800, Oscar Mateo wrote:
>> This vma contains much more than just the additional data struct (ads)
>> and since we were already using the word "addon" as an object in
>> guc_addon_create, make it the preffered one. No need for the vma suffix
>> either, as that dependency is given by the type.
> Hmm, but almost all over the driver we're using "vma" as a name or suffix.
> Why do you want to change this here ?
Same reason why I changed ctx_pool_vma to ctx_pool: because the type 
(i915_vma) already tells you everything you need to know (suggestion by 
Joonas).
> Also, GUC still is using term "ADS" for this blob so I'm not convinced
> that we need new name here.
>
> -Michal
>
We still keep the name "ads" in this patch, but we use it to refer to 
the struct the GuC expects, while "addon" is the GEM object that 
contains both the ads struct and all the additional stuff the GuC needs. 
But I could equally keep the ads name and 
s/guc_addon_create/guc_ads_create (after all, blue or red, a shed is a 
shed).

>> while at it, add an explanation of what things go inside the addon object.
>>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 12 ++++++++----
>>   drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++--
>>   drivers/gpu/drm/i915/intel_uc.h            |  2 +-
>>   3 files changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 7562343c..e1922fe 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -834,9 +834,13 @@ static int guc_addon_create(struct intel_guc *guc)
>>   	struct page *page;
>>   	u32 size;
>>   
>> -	GEM_BUG_ON(guc->ads_vma);
>> +	GEM_BUG_ON(guc->addon);
>>   
>> -	/* The ads obj includes the struct itself and buffers passed to GuC */
>> +	/* The additional data struct (ADS) has pointers for different buffers
>> +	 * used by the GuC. The addon object contains the ADS itself (guc_ads),
>> +	 * the scheduling policies (guc_policies), a structure describing
>> +	 * a collection of register sets (guc_mmio_reg_state) and some extra
>> +	 * pages for the GuC to save its internal state for sleep */
>>   	size = sizeof(struct guc_ads) + sizeof(struct guc_policies) +
>>   			sizeof(struct guc_mmio_reg_state) +
>>   			GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
>> @@ -845,7 +849,7 @@ static int guc_addon_create(struct intel_guc *guc)
>>   	if (IS_ERR(vma))
>>   		return PTR_ERR(vma);
>>   
>> -	guc->ads_vma = vma;
>> +	guc->addon = vma;
>>   
>>   	page = i915_vma_first_page(vma);
>>   	ads = kmap(page);
>> @@ -894,7 +898,7 @@ static int guc_addon_create(struct intel_guc *guc)
>>   
>>   static void guc_addon_destroy(struct intel_guc *guc)
>>   {
>> -	i915_vma_unpin_and_release(&guc->ads_vma);
>> +	i915_vma_unpin_and_release(&guc->addon);
>>   }
>>   
>>   /*
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index 1f9ec54..1eb0c51 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -213,8 +213,8 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
>>   	} else
>>   		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
>>   
>> -	if (guc->ads_vma) {
>> -		u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
>> +	if (guc->addon) {
>> +		u32 ads = guc_ggtt_offset(guc->addon) >> PAGE_SHIFT;
>>   		params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT;
>>   		params[GUC_CTL_DEBUG] |= GUC_ADS_ENABLED;
>>   	}
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
>> index 330d08f..d8897b5 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -152,7 +152,7 @@ struct intel_guc {
>>   	/* intel_guc_recv interrupt related state */
>>   	bool interrupts_enabled;
>>   
>> -	struct i915_vma *ads_vma;
>> +	struct i915_vma *addon;
>>   	struct i915_vma *ctx_pool;
>>   	void *ctx_pool_vaddr;
>>   	struct ida ctx_ids;
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915/guc: Break out the GuC log "extras"

[Oscar Mateo]

When initializing the GuC log struct, there is an object we need to
allocate always, since the GuC needs its address at fw load time.
The rest are "extras", in the sense that we only need them if we
actually enable GuC logging. Make that distinction explicit by
subdividing further the intel_guc_log struct.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  4 +--
 drivers/gpu/drm/i915/intel_guc_log.c | 54 ++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_uc.h      | 12 ++++----
 3 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc70e2c..c826aa8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1724,8 +1724,8 @@ static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
 			I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
 
 			/* Handle flush interrupt in bottom half */
-			queue_work(dev_priv->guc.log.flush_wq,
-				   &dev_priv->guc.log.flush_work);
+			queue_work(dev_priv->guc.log.extras.flush_wq,
+				   &dev_priv->guc.log.extras.flush_work);
 
 			dev_priv->guc.log.flush_interrupt_count++;
 		} else {
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index d0632fc..bb1c136 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -166,7 +166,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
 		return -ENODEV;
 	}
 
-	ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
+	ret = relay_late_setup_files(guc->log.extras.relay_chan, "guc_log", log_dir);
 	if (ret < 0 && ret != -EEXIST) {
 		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
 		return ret;
@@ -183,15 +183,15 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
 	smp_wmb();
 
 	/* All data has been written, so now move the offset of sub buffer. */
-	relay_reserve(guc->log.relay_chan, guc->log.vma->obj->base.size);
+	relay_reserve(guc->log.extras.relay_chan, guc->log.vma->obj->base.size);
 
 	/* Switch to the next sub buffer */
-	relay_flush(guc->log.relay_chan);
+	relay_flush(guc->log.extras.relay_chan);
 }
 
 static void *guc_get_write_buffer(struct intel_guc *guc)
 {
-	if (!guc->log.relay_chan)
+	if (!guc->log.extras.relay_chan)
 		return NULL;
 
 	/* Just get the base address of a new sub buffer and copy data into it
@@ -202,7 +202,7 @@ static void *guc_get_write_buffer(struct intel_guc *guc)
 	 * 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);
+	return relay_reserve(guc->log.extras.relay_chan, 0);
 }
 
 static bool guc_check_log_buf_overflow(struct intel_guc *guc,
@@ -253,11 +253,11 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 	void *src_data, *dst_data;
 	bool new_overflow;
 
-	if (WARN_ON(!guc->log.buf_addr))
+	if (WARN_ON(!guc->log.extras.buf_addr))
 		return;
 
 	/* Get the pointer to shared GuC log buffer */
-	log_buf_state = src_data = guc->log.buf_addr;
+	log_buf_state = src_data = guc->log.extras.buf_addr;
 
 	/* Get the pointer to local buffer to store the logs */
 	log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
@@ -343,17 +343,17 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
 static void capture_logs_work(struct work_struct *work)
 {
 	struct intel_guc *guc =
-		container_of(work, struct intel_guc, log.flush_work);
+		container_of(work, struct intel_guc, log.extras.flush_work);
 
 	guc_log_capture_logs(guc);
 }
 
 static bool guc_log_has_extras(struct intel_guc *guc)
 {
-	return (guc->log.buf_addr != NULL);
+	return (guc->log.extras.buf_addr != NULL);
 }
 
-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;
@@ -375,7 +375,7 @@ static int guc_log_create_extras(struct intel_guc *guc)
 		return PTR_ERR(vaddr);
 	}
 
-	guc->log.buf_addr = vaddr;
+	guc->log.extras.buf_addr = vaddr;
 
 	 /* Keep the size of sub buffers same as shared log buffer */
 	subbuf_size = guc->log.vma->obj->base.size;
@@ -401,9 +401,9 @@ static int guc_log_create_extras(struct intel_guc *guc)
 	}
 
 	GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
-	guc->log.relay_chan = guc_log_relay_chan;
+	guc->log.extras.relay_chan = guc_log_relay_chan;
 
-	INIT_WORK(&guc->log.flush_work, capture_logs_work);
+	INIT_WORK(&guc->log.extras.flush_work, capture_logs_work);
 
 	/*
 	 * GuC log buffer flush work item has to do register access to
@@ -416,9 +416,9 @@ static int guc_log_create_extras(struct intel_guc *guc)
 	 * 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",
+	guc->log.extras.flush_wq = alloc_ordered_workqueue("i915-guc_log",
 						    WQ_HIGHPRI | WQ_FREEZABLE);
-	if (!guc->log.flush_wq) {
+	if (!guc->log.extras.flush_wq) {
 		DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
 		ret = -ENOMEM;
 		goto err_relaychan;
@@ -427,14 +427,14 @@ static int guc_log_create_extras(struct intel_guc *guc)
 	return 0;
 
 err_relaychan:
-	relay_close(guc->log.relay_chan);
+	relay_close(guc->log.extras.relay_chan);
 err_vaddr:
 	i915_gem_object_unpin_map(guc->log.vma->obj);
-	guc->log.buf_addr = NULL;
+	guc->log.extras.buf_addr = NULL;
 	return ret;
 }
 
-static void guc_log_destroy_extras(struct intel_guc *guc)
+static void guc_log_extras_destroy(struct intel_guc *guc)
 {
 	/*
 	 * It's possible that extras were never allocated because guc_log_level
@@ -443,10 +443,10 @@ static void guc_log_destroy_extras(struct intel_guc *guc)
 	if (!guc_log_has_extras(guc))
 		return;
 
-	destroy_workqueue(guc->log.flush_wq);
-	relay_close(guc->log.relay_chan);
+	destroy_workqueue(guc->log.extras.flush_wq);
+	relay_close(guc->log.extras.relay_chan);
 	i915_gem_object_unpin_map(guc->log.vma->obj);
-	guc->log.buf_addr = NULL;
+	guc->log.extras.buf_addr = NULL;
 }
 
 static int guc_log_late_setup(struct intel_guc *guc)
@@ -461,7 +461,7 @@ static int guc_log_late_setup(struct intel_guc *guc)
 		 * 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;
 	}
@@ -473,7 +473,7 @@ static int guc_log_late_setup(struct intel_guc *guc)
 	return 0;
 
 err_extras:
-	guc_log_destroy_extras(guc);
+	guc_log_extras_destroy(guc);
 err:
 	/* logging will remain off */
 	i915.guc_log_level = -1;
@@ -507,7 +507,7 @@ static void guc_flush_logs(struct intel_guc *guc)
 	/* Before initiating the forceful flush, wait for any pending/ongoing
 	 * flush to complete otherwise forceful flush may not actually happen.
 	 */
-	flush_work(&guc->log.flush_work);
+	flush_work(&guc->log.extras.flush_work);
 
 	/* Ask GuC to update the log buffer state */
 	guc_log_flush(guc);
@@ -552,7 +552,7 @@ int intel_guc_log_create(struct intel_guc *guc)
 	guc->log.vma = vma;
 
 	if (i915.guc_log_level >= 0) {
-		ret = guc_log_create_extras(guc);
+		ret = guc_log_extras_create(guc);
 		if (ret < 0)
 			goto err_vma;
 	}
@@ -578,7 +578,7 @@ int intel_guc_log_create(struct intel_guc *guc)
 
 void intel_guc_log_destroy(struct intel_guc *guc)
 {
-	guc_log_destroy_extras(guc);
+	guc_log_extras_destroy(guc);
 	i915_vma_unpin_and_release(&guc->log.vma);
 }
 
@@ -651,6 +651,6 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	gen9_disable_guc_interrupts(dev_priv);
-	guc_log_destroy_extras(&dev_priv->guc);
+	guc_log_extras_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 d8897b5..c198ca7 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -132,11 +132,13 @@ struct intel_uc_fw {
 struct intel_guc_log {
 	uint32_t flags;
 	struct i915_vma *vma;
-	void *buf_addr;
-	struct workqueue_struct *flush_wq;
-	struct work_struct flush_work;
-	struct rchan *relay_chan;
-
+	/* These extras get created only when GuC logging gets enabled */
+	struct {
+		void *buf_addr;
+		struct workqueue_struct *flush_wq;
+		struct work_struct flush_work;
+		struct rchan *relay_chan;
+	} extras;
 	/* logging related stats */
 	u32 capture_miss_count;
 	u32 flush_interrupt_count;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/guc: Break out the GuC log "extras"

[Joonas Lahtinen]

On pe, 2017-02-24 at 06:01 -0800, Oscar Mateo wrote:
> When initializing the GuC log struct, there is an object we need to
> allocate always, since the GuC needs its address at fw load time.
> The rest are "extras", in the sense that we only need them if we
> actually enable GuC logging. Make that distinction explicit by
> subdividing further the intel_guc_log struct.
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

I'm not sure about calling them extras. How about runtime?

Also, I'm not quite sure if the abstractions are currently correct
(despite the names), logging seems to control overall guc interrups,
for example (i915_guc_log_unregister func).

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/guc: Break out the GuC log "extras"

[Oscar Mateo]

On 03/01/2017 04:37 AM, Joonas Lahtinen wrote:
> On pe, 2017-02-24 at 06:01 -0800, Oscar Mateo wrote:
>> When initializing the GuC log struct, there is an object we need to
>> allocate always, since the GuC needs its address at fw load time.
>> The rest are "extras", in the sense that we only need them if we
>> actually enable GuC logging. Make that distinction explicit by
>> subdividing further the intel_guc_log struct.
>>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> I'm not sure about calling them extras. How about runtime?
Agree, runtime sounds much better. I'll refresh the patch.
> Also, I'm not quite sure if the abstractions are currently correct
> (despite the names), logging seems to control overall guc interrups,
> for example (i915_guc_log_unregister func).
>
Hmmm... GuC interrupts are a mess, I agree (for the moment the only 
Guc2Host interrupts we handle are log-related ones, so I imagine that's 
the reason, but this could very easily change in the future). I'll try 
to fix this either as part of patch 3 or in a separate patch.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/guc: Break out the GuC log "extras"

[Michal Wajdeczko]

On Fri, Feb 24, 2017 at 06:01:37AM -0800, Oscar Mateo wrote:
> When initializing the GuC log struct, there is an object we need to
> allocate always, since the GuC needs its address at fw load time.
> The rest are "extras", in the sense that we only need them if we
> actually enable GuC logging. Make that distinction explicit by
> subdividing further the intel_guc_log struct.
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

Regards,
Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx