[PATCH 0/2] GuC suspend paths cleanup

[PATCH 0/2] GuC suspend paths cleanup

[Sujaritha Sundaresan]

The work was started to fix bugs that were seen on the
suspend and hibernate devices path.The initial issue to be seen 
was a warning with the CTB. In parallel there were issues seen on the 
suspend paths. This series works to resolve the errors in the GuC
cleanup paths and be compatible with lockless reset.

Sujaritha Sundaresan (2):
  drm/i915/guc: Splitting CT channel open/close functions
  drm/i915/guc: Calling guc_disable_communication in all suspend paths

 drivers/gpu/drm/i915/i915_reset.c   |  2 +-
 drivers/gpu/drm/i915/intel_guc.c    | 12 ++++
 drivers/gpu/drm/i915/intel_guc_ct.c | 85 +++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_guc_ct.h |  3 +
 drivers/gpu/drm/i915/intel_uc.c     | 23 ++++++--
 drivers/gpu/drm/i915/intel_uc.h     |  1 +
 6 files changed, 98 insertions(+), 28 deletions(-)

-- 
2.20.1

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

[PATCH 1/2] drm/i915/guc: Splitting CT channel open/close functions

[Sujaritha Sundaresan]

The aim of this patch is to allow enabling and disabling
of CTB without requiring the mutex lock.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c    | 12 ++++
 drivers/gpu/drm/i915/intel_guc_ct.c | 85 +++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_guc_ct.h |  3 +
 3 files changed, 77 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 8660af3fd755..8ecb47087457 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -203,11 +203,19 @@ int intel_guc_init(struct intel_guc *guc)
 		goto err_log;
 	GEM_BUG_ON(!guc->ads_vma);
 
+	if (HAS_GUC_CT(dev_priv)) {
+		ret = intel_guc_ct_init(&guc->ct);
+		if (ret)
+			goto err_ads;
+	}
+
 	/* We need to notify the guc whenever we change the GGTT */
 	i915_ggtt_enable_guc(dev_priv);
 
 	return 0;
 
+err_ads:
+	intel_guc_ads_destroy(guc);
 err_log:
 	intel_guc_log_destroy(&guc->log);
 err_shared:
@@ -222,6 +230,10 @@ void intel_guc_fini(struct intel_guc *guc)
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
 	i915_ggtt_disable_guc(dev_priv);
+
+	if (HAS_GUC_CT(dev_priv))
+		intel_guc_ct_fini(&guc->ct);
+
 	intel_guc_ads_destroy(guc);
 	intel_guc_log_destroy(&guc->log);
 	guc_shared_data_destroy(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index a52883e9146f..fbf9da247975 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -140,9 +140,9 @@ static int guc_action_deregister_ct_buffer(struct intel_guc *guc,
 	return err;
 }
 
-static bool ctch_is_open(struct intel_guc_ct_channel *ctch)
+static bool ctch_is_enabled(struct intel_guc_ct_channel *ctch)
 {
-	return ctch->vma != NULL;
+	return ctch->is_enabled;
 }
 
 static int ctch_init(struct intel_guc *guc,
@@ -217,22 +217,14 @@ static void ctch_fini(struct intel_guc *guc,
 	i915_vma_unpin_and_release(&ctch->vma, I915_VMA_RELEASE_MAP);
 }
 
-static int ctch_open(struct intel_guc *guc,
+static int ctch_enable(struct intel_guc *guc,
 		     struct intel_guc_ct_channel *ctch)
 {
 	u32 base;
 	int err;
 	int i;
 
-	CT_DEBUG_DRIVER("CT: channel %d reopen=%s\n",
-			ctch->owner, yesno(ctch_is_open(ctch)));
-
-	if (!ctch->vma) {
-		err = ctch_init(guc, ctch);
-		if (unlikely(err))
-			goto err_out;
-		GEM_BUG_ON(!ctch->vma);
-	}
+	GEM_BUG_ON(!ctch->vma);
 
 	/* vma should be already allocated and map'ed */
 	base = intel_guc_ggtt_offset(guc, ctch->vma);
@@ -255,7 +247,7 @@ static int ctch_open(struct intel_guc *guc,
 					    base + PAGE_SIZE/4 * CTB_RECV,
 					    INTEL_GUC_CT_BUFFER_TYPE_RECV);
 	if (unlikely(err))
-		goto err_fini;
+		goto err_out;
 
 	err = guc_action_register_ct_buffer(guc,
 					    base + PAGE_SIZE/4 * CTB_SEND,
@@ -263,23 +255,25 @@ static int ctch_open(struct intel_guc *guc,
 	if (unlikely(err))
 		goto err_deregister;
 
+	ctch->is_enabled = true;
+
 	return 0;
 
 err_deregister:
 	guc_action_deregister_ct_buffer(guc,
 					ctch->owner,
 					INTEL_GUC_CT_BUFFER_TYPE_RECV);
-err_fini:
-	ctch_fini(guc, ctch);
 err_out:
 	DRM_ERROR("CT: can't open channel %d; err=%d\n", ctch->owner, err);
 	return err;
 }
 
-static void ctch_close(struct intel_guc *guc,
+static void ctch_disable(struct intel_guc *guc,
 		       struct intel_guc_ct_channel *ctch)
 {
-	GEM_BUG_ON(!ctch_is_open(ctch));
+	GEM_BUG_ON(!ctch_is_enabled(ctch));
+
+	ctch->is_enabled = false;
 
 	guc_action_deregister_ct_buffer(guc,
 					ctch->owner,
@@ -287,7 +281,6 @@ static void ctch_close(struct intel_guc *guc,
 	guc_action_deregister_ct_buffer(guc,
 					ctch->owner,
 					INTEL_GUC_CT_BUFFER_TYPE_RECV);
-	ctch_fini(guc, ctch);
 }
 
 static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
@@ -481,7 +474,7 @@ static int ctch_send(struct intel_guc_ct *ct,
 	u32 fence;
 	int err;
 
-	GEM_BUG_ON(!ctch_is_open(ctch));
+	GEM_BUG_ON(!ctch_is_enabled(ctch));
 	GEM_BUG_ON(!len);
 	GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
 	GEM_BUG_ON(!response_buf && response_buf_size);
@@ -817,7 +810,7 @@ static void ct_process_host_channel(struct intel_guc_ct *ct)
 	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
 	int err = 0;
 
-	if (!ctch_is_open(ctch))
+	if (!ctch_is_enabled(ctch))
 		return;
 
 	do {
@@ -848,6 +841,49 @@ static void intel_guc_to_host_event_handler_ct(struct intel_guc *guc)
 	ct_process_host_channel(ct);
 }
 
+/**
+ * intel_guc_ct_init - Allocate CT
+ * @ct: pointer to CT struct
+ * 
+ * Shall only be called for platforms with HAS_GUC_CT.
+ * 
+ * Return: 0 on success, a negative errno code on failure.
+ */
+int intel_guc_ct_init(struct intel_guc_ct *ct)
+{
+	struct intel_guc *guc = ct_to_guc(ct);
+	struct intel_guc_ct_channel *ctch = &ct->host_channel;
+	int err;
+
+	GEM_BUG_ON(ctch->vma);
+
+	err = ctch_init(guc, ctch);
+	if (unlikely(err)) {
+		DRM_ERROR("CT: can't open channel %d; err=%d\n", 
+			ctch->owner, err);
+		return err;
+	}
+
+	GEM_BUG_ON(!ctch->vma);
+	return 0; 
+}
+
+/**
+ * intel_guc_ct_fini - Deallocate CT
+ * @ct: pointer to CT struct
+ * 
+ * Shall only be called for platforms with HAS_GUC_CT.
+ */
+void intel_guc_ct_fini(struct intel_guc_ct *ct)
+{
+	struct intel_guc *guc = ct_to_guc(ct);
+	struct intel_guc_ct_channel *ctch = &ct->host_channel;
+
+	GEM_BUG_ON(ctch_is_enabled(ctch));
+
+	ctch_fini(guc, ctch);
+}
+
 /**
  * intel_guc_ct_enable - Enable buffer based command transport.
  * @ct: pointer to CT struct
@@ -865,7 +901,10 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 
 	GEM_BUG_ON(!HAS_GUC_CT(i915));
 
-	err = ctch_open(guc, ctch);
+	if (ctch_is_enabled(ctch))
+		return 0;
+
+	err = ctch_enable(guc, ctch);
 	if (unlikely(err))
 		return err;
 
@@ -890,10 +929,10 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct)
 
 	GEM_BUG_ON(!HAS_GUC_CT(i915));
 
-	if (!ctch_is_open(ctch))
+	if (!ctch_is_enabled(ctch))
 		return;
 
-	ctch_close(guc, ctch);
+	ctch_disable(guc, ctch);
 
 	/* Disable send */
 	guc->send = intel_guc_send_nop;
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
index d774895ab143..e05fdc3cf0b0 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/intel_guc_ct.h
@@ -64,6 +64,7 @@ struct intel_guc_ct_buffer {
 struct intel_guc_ct_channel {
 	struct i915_vma *vma;
 	struct intel_guc_ct_buffer ctbs[2];
+	bool is_enabled;
 	u32 owner;
 	u32 next_fence;
 };
@@ -90,6 +91,8 @@ struct intel_guc_ct {
 };
 
 void intel_guc_ct_init_early(struct intel_guc_ct *ct);
+int intel_guc_ct_init(struct intel_guc_ct *ct);
+void intel_guc_ct_fini(struct intel_guc_ct *ct);
 int intel_guc_ct_enable(struct intel_guc_ct *ct);
 void intel_guc_ct_disable(struct intel_guc_ct *ct);
 
-- 
2.20.1

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

[PATCH 2/2] drm/i915/guc: Calling guc_disable_communication in all suspend paths

[Sujaritha Sundaresan]

This aim of this patch is to call guc_disable_communication in all
suspend paths. The reason to introduce this is to resolve a bug that
occured due to suspend late not being called in the hibernate devices
path.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
---
 drivers/gpu/drm/i915/i915_reset.c |  2 +-
 drivers/gpu/drm/i915/intel_uc.c   | 23 +++++++++++++++++++----
 drivers/gpu/drm/i915/intel_uc.h   |  1 +
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
index 12e74decd7a2..36e5c9c64285 100644
--- a/drivers/gpu/drm/i915/i915_reset.c
+++ b/drivers/gpu/drm/i915/i915_reset.c
@@ -673,7 +673,7 @@ static void reset_prepare(struct drm_i915_private *i915)
 	for_each_engine(engine, i915, id)
 		reset_prepare_engine(engine);
 
-	intel_uc_sanitize(i915);
+	intel_uc_reset_prepare(i915);
 	revoke_mmaps(i915);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index e711eb3268bc..2d360d53757f 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -332,8 +332,6 @@ void intel_uc_sanitize(struct drm_i915_private *i915)
 
 	GEM_BUG_ON(!HAS_GUC(i915));
 
-	guc_disable_communication(guc);
-
 	intel_huc_sanitize(huc);
 	intel_guc_sanitize(guc);
 
@@ -451,6 +449,23 @@ void intel_uc_fini_hw(struct drm_i915_private *i915)
 	guc_disable_communication(guc);
 }
 
+/**
+ * intel_uc_reset_prepare - Prepare for reset
+ * @i915: device private
+ *
+ * Preparing for full gpu reset.
+ */
+void intel_uc_reset_prepare(struct drm_i915_private *i915)
+{
+	struct intel_guc *guc = &i915->guc;
+
+	if (!USES_GUC(i915))
+		return;
+
+	guc_disable_communication(guc);
+	intel_uc_sanitize(i915);
+}
+
 int intel_uc_suspend(struct drm_i915_private *i915)
 {
 	struct intel_guc *guc = &i915->guc;
@@ -468,7 +483,7 @@ int intel_uc_suspend(struct drm_i915_private *i915)
 		return err;
 	}
 
-	gen9_disable_guc_interrupts(i915);
+	guc_disable_communication(guc);
 
 	return 0;
 }
@@ -484,7 +499,7 @@ int intel_uc_resume(struct drm_i915_private *i915)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	gen9_enable_guc_interrupts(i915);
+	guc_enable_communication(guc);
 
 	err = intel_guc_resume(guc);
 	if (err) {
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 870faf9011b9..c14729786652 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -38,6 +38,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv);
 void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
 int intel_uc_init(struct drm_i915_private *dev_priv);
 void intel_uc_fini(struct drm_i915_private *dev_priv);
+void intel_uc_reset_prepare(struct drm_i915_private *i915);
 int intel_uc_suspend(struct drm_i915_private *dev_priv);
 int intel_uc_resume(struct drm_i915_private *dev_priv);
 
-- 
2.20.1

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

Re: [PATCH 1/2] drm/i915/guc: Splitting CT channel open/close functions

[Daniele Ceraolo Spurio]

On 2/14/19 1:45 PM, Sujaritha Sundaresan wrote:
> The aim of this patch is to allow enabling and disabling
> of CTB without requiring the mutex lock.
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc.c    | 12 ++++
>   drivers/gpu/drm/i915/intel_guc_ct.c | 85 +++++++++++++++++++++--------
>   drivers/gpu/drm/i915/intel_guc_ct.h |  3 +
>   3 files changed, 77 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 8660af3fd755..8ecb47087457 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -203,11 +203,19 @@ int intel_guc_init(struct intel_guc *guc)
>   		goto err_log;
>   	GEM_BUG_ON(!guc->ads_vma);
>   
> +	if (HAS_GUC_CT(dev_priv)) {
> +		ret = intel_guc_ct_init(&guc->ct);
> +		if (ret)
> +			goto err_ads;
> +	}
> +
>   	/* We need to notify the guc whenever we change the GGTT */
>   	i915_ggtt_enable_guc(dev_priv);
>   
>   	return 0;
>   
> +err_ads:
> +	intel_guc_ads_destroy(guc);
>   err_log:
>   	intel_guc_log_destroy(&guc->log);
>   err_shared:
> @@ -222,6 +230,10 @@ void intel_guc_fini(struct intel_guc *guc)
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   
>   	i915_ggtt_disable_guc(dev_priv);
> +
> +	if (HAS_GUC_CT(dev_priv))
> +		intel_guc_ct_fini(&guc->ct);
> +
>   	intel_guc_ads_destroy(guc);
>   	intel_guc_log_destroy(&guc->log);
>   	guc_shared_data_destroy(guc);
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index a52883e9146f..fbf9da247975 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -140,9 +140,9 @@ static int guc_action_deregister_ct_buffer(struct intel_guc *guc,
>   	return err;
>   }
>   
> -static bool ctch_is_open(struct intel_guc_ct_channel *ctch)
> +static bool ctch_is_enabled(struct intel_guc_ct_channel *ctch)
>   {
> -	return ctch->vma != NULL;
> +	return ctch->is_enabled;

Bikeshed: now that the check is simpler, doing ctch->enabled is actually 
simpler then ctch_is_enabled(ctch), so I'd prefer to just switch.

>   }
>   
>   static int ctch_init(struct intel_guc *guc,
> @@ -217,22 +217,14 @@ static void ctch_fini(struct intel_guc *guc,
>   	i915_vma_unpin_and_release(&ctch->vma, I915_VMA_RELEASE_MAP);
>   }
>   
> -static int ctch_open(struct intel_guc *guc,
> +static int ctch_enable(struct intel_guc *guc,
>   		     struct intel_guc_ct_channel *ctch)
>   {
>   	u32 base;
>   	int err;
>   	int i;
>   
> -	CT_DEBUG_DRIVER("CT: channel %d reopen=%s\n",
> -			ctch->owner, yesno(ctch_is_open(ctch)));
> -
> -	if (!ctch->vma) {
> -		err = ctch_init(guc, ctch);
> -		if (unlikely(err))
> -			goto err_out;
> -		GEM_BUG_ON(!ctch->vma);
> -	}
> +	GEM_BUG_ON(!ctch->vma);

GEM_BUG_ON(ctch->enabled) as well?

>   
>   	/* vma should be already allocated and map'ed */
>   	base = intel_guc_ggtt_offset(guc, ctch->vma);
> @@ -255,7 +247,7 @@ static int ctch_open(struct intel_guc *guc,
>   					    base + PAGE_SIZE/4 * CTB_RECV,
>   					    INTEL_GUC_CT_BUFFER_TYPE_RECV);
>   	if (unlikely(err))
> -		goto err_fini;
> +		goto err_out;
>   
>   	err = guc_action_register_ct_buffer(guc,
>   					    base + PAGE_SIZE/4 * CTB_SEND,
> @@ -263,23 +255,25 @@ static int ctch_open(struct intel_guc *guc,
>   	if (unlikely(err))
>   		goto err_deregister;
>   
> +	ctch->is_enabled = true;
> +
>   	return 0;
>   
>   err_deregister:
>   	guc_action_deregister_ct_buffer(guc,
>   					ctch->owner,
>   					INTEL_GUC_CT_BUFFER_TYPE_RECV);
> -err_fini:
> -	ctch_fini(guc, ctch);
>   err_out:
>   	DRM_ERROR("CT: can't open channel %d; err=%d\n", ctch->owner, err);
>   	return err;
>   }
>   
> -static void ctch_close(struct intel_guc *guc,
> +static void ctch_disable(struct intel_guc *guc,
>   		       struct intel_guc_ct_channel *ctch)
>   {
> -	GEM_BUG_ON(!ctch_is_open(ctch));
> +	GEM_BUG_ON(!ctch_is_enabled(ctch));
> +
> +	ctch->is_enabled = false;
>   
>   	guc_action_deregister_ct_buffer(guc,
>   					ctch->owner,
> @@ -287,7 +281,6 @@ static void ctch_close(struct intel_guc *guc,
>   	guc_action_deregister_ct_buffer(guc,
>   					ctch->owner,
>   					INTEL_GUC_CT_BUFFER_TYPE_RECV);
> -	ctch_fini(guc, ctch);
>   }
>   
>   static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
> @@ -481,7 +474,7 @@ static int ctch_send(struct intel_guc_ct *ct,
>   	u32 fence;
>   	int err;
>   
> -	GEM_BUG_ON(!ctch_is_open(ctch));
> +	GEM_BUG_ON(!ctch_is_enabled(ctch));
>   	GEM_BUG_ON(!len);
>   	GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>   	GEM_BUG_ON(!response_buf && response_buf_size);
> @@ -817,7 +810,7 @@ static void ct_process_host_channel(struct intel_guc_ct *ct)
>   	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
>   	int err = 0;
>   
> -	if (!ctch_is_open(ctch))
> +	if (!ctch_is_enabled(ctch))
>   		return;
>   
>   	do {
> @@ -848,6 +841,49 @@ static void intel_guc_to_host_event_handler_ct(struct intel_guc *guc)
>   	ct_process_host_channel(ct);
>   }
>   
> +/**
> + * intel_guc_ct_init - Allocate CT

You're not really allocating the CT struct, but the internal channel. 
Maybe just say "init CT communication" here and then say in the 
description below "allocate memory required for communication via the CT 
channel".

Similar thing for the fini path.

> + * @ct: pointer to CT struct
> + *
> + * Shall only be called for platforms with HAS_GUC_CT.
> + *
> + * Return: 0 on success, a negative errno code on failure.
> + */
> +int intel_guc_ct_init(struct intel_guc_ct *ct)
> +{
> +	struct intel_guc *guc = ct_to_guc(ct);
> +	struct intel_guc_ct_channel *ctch = &ct->host_channel;
> +	int err;
> +
> +	GEM_BUG_ON(ctch->vma);

the BUG_ON is already in ctch_init(), no need to duplicate.

> +
> +	err = ctch_init(guc, ctch);
> +	if (unlikely(err)) {
> +		DRM_ERROR("CT: can't open channel %d; err=%d\n",
> +			ctch->owner, err);
> +		return err;
> +	}
> +
> +	GEM_BUG_ON(!ctch->vma);
> +	return 0;
> +}
> +
> +/**
> + * intel_guc_ct_fini - Deallocate CT
> + * @ct: pointer to CT struct
> + *
> + * Shall only be called for platforms with HAS_GUC_CT.
> + */
> +void intel_guc_ct_fini(struct intel_guc_ct *ct)
> +{
> +	struct intel_guc *guc = ct_to_guc(ct);
> +	struct intel_guc_ct_channel *ctch = &ct->host_channel;
> +
> +	GEM_BUG_ON(ctch_is_enabled(ctch));

We could move this inside ctch_fini() to keep all the ctch BUG_ONs 
inside ctch_* functions.

> +
> +	ctch_fini(guc, ctch);
> +}
> +
>   /**
>    * intel_guc_ct_enable - Enable buffer based command transport.
>    * @ct: pointer to CT struct
> @@ -865,7 +901,10 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
>   
>   	GEM_BUG_ON(!HAS_GUC_CT(i915));
>   
> -	err = ctch_open(guc, ctch);
> +	if (ctch_is_enabled(ctch))
> +		return 0;
> +
> +	err = ctch_enable(guc, ctch);
>   	if (unlikely(err))
>   		return err;
>   
> @@ -890,10 +929,10 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct)
>   
>   	GEM_BUG_ON(!HAS_GUC_CT(i915));
>   
> -	if (!ctch_is_open(ctch))
> +	if (!ctch_is_enabled(ctch))
>   		return;
>   
> -	ctch_close(guc, ctch);
> +	ctch_disable(guc, ctch);
>   
>   	/* Disable send */
>   	guc->send = intel_guc_send_nop;
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
> index d774895ab143..e05fdc3cf0b0 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
> @@ -64,6 +64,7 @@ struct intel_guc_ct_buffer {
>   struct intel_guc_ct_channel {
>   	struct i915_vma *vma;
>   	struct intel_guc_ct_buffer ctbs[2];
> +	bool is_enabled;

just "enabled" is fine.

Daniele

>   	u32 owner;
>   	u32 next_fence;
>   };
> @@ -90,6 +91,8 @@ struct intel_guc_ct {
>   };
>   
>   void intel_guc_ct_init_early(struct intel_guc_ct *ct);
> +int intel_guc_ct_init(struct intel_guc_ct *ct);
> +void intel_guc_ct_fini(struct intel_guc_ct *ct);
>   int intel_guc_ct_enable(struct intel_guc_ct *ct);
>   void intel_guc_ct_disable(struct intel_guc_ct *ct);
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for GuC suspend paths cleanup

[Patchwork]

== Series Details ==

Series: GuC suspend paths cleanup
URL   : https://patchwork.freedesktop.org/series/56697/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
22e49b914e2d drm/i915/guc: Splitting CT channel open/close functions
-:70: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#70: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:221:
+static int ctch_enable(struct intel_guc *guc,
 		     struct intel_guc_ct_channel *ctch)

-:119: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#119: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:272:
+static void ctch_disable(struct intel_guc *guc,
 		       struct intel_guc_ct_channel *ctch)

-:161: ERROR:TRAILING_WHITESPACE: trailing whitespace
#161: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:847:
+ * $

-:163: ERROR:TRAILING_WHITESPACE: trailing whitespace
#163: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:849:
+ * $

-:176: ERROR:TRAILING_WHITESPACE: trailing whitespace
#176: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:862:
+^I^IDRM_ERROR("CT: can't open channel %d; err=%d\n", $

-:177: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#177: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:863:
+		DRM_ERROR("CT: can't open channel %d; err=%d\n", 
+			ctch->owner, err);

-:182: ERROR:TRAILING_WHITESPACE: trailing whitespace
#182: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:868:
+^Ireturn 0; $

-:188: ERROR:TRAILING_WHITESPACE: trailing whitespace
#188: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:874:
+ * $

total: 5 errors, 0 warnings, 3 checks, 211 lines checked
fbd14aaf53bc drm/i915/guc: Calling guc_disable_communication in all suspend paths
-:9: WARNING:TYPO_SPELLING: 'occured' may be misspelled - perhaps 'occurred'?
#9: 
occured due to suspend late not being called in the hibernate devices

total: 0 errors, 1 warnings, 0 checks, 62 lines checked

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

✓ Fi.CI.BAT: success for GuC suspend paths cleanup

[Patchwork]

== Series Details ==

Series: GuC suspend paths cleanup
URL   : https://patchwork.freedesktop.org/series/56697/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5604 -> Patchwork_12225
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56697/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12225 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       PASS -> DMESG-WARN [fdo#108965]

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       PASS -> FAIL [fdo#109485]

  
#### Possible fixes ####

  * igt@gem_render_tiled_blits@basic:
    - fi-skl-6770hq:      DMESG-WARN [fdo#105541] -> PASS

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-skl-6700hq:      DMESG-WARN [fdo#105998] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     FAIL [fdo#103167] -> PASS

  * igt@pm_rpm@basic-pci-d3-state:
    - fi-bsw-kefka:       {SKIP} [fdo#109271] -> PASS

  * igt@pm_rpm@basic-rte:
    - fi-bsw-kefka:       FAIL [fdo#108800] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
  [fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
  [fdo#109527]: https://bugs.freedesktop.org/show_bug.cgi?id=109527
  [fdo#109528]: https://bugs.freedesktop.org/show_bug.cgi?id=109528
  [fdo#109530]: https://bugs.freedesktop.org/show_bug.cgi?id=109530


Participating hosts (47 -> 41)
------------------------------

  Additional (1): fi-icl-y 
  Missing    (7): fi-ilk-m540 fi-byt-j1900 fi-byt-squawks fi-bsw-cyan fi-kbl-x1275 fi-icl-u3 fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5604 -> Patchwork_12225

  CI_DRM_5604: bf979a24473a1b23df86ff3fd0ceb33dc1d62e82 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4827: 395eaffd7e1390c9d6043c2980dc14ce3e08b154 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12225: fbd14aaf53bce8c461f3bbff795d10ccc29721dc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

fbd14aaf53bc drm/i915/guc: Calling guc_disable_communication in all suspend paths
22e49b914e2d drm/i915/guc: Splitting CT channel open/close functions

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12225/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for GuC suspend paths cleanup

[Patchwork]

== Series Details ==

Series: GuC suspend paths cleanup
URL   : https://patchwork.freedesktop.org/series/56697/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5604_full -> Patchwork_12225_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12225_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12225_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12225_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
    - shard-apl:          PASS -> FAIL

  
Known issues
------------

  Here are the changes found in Patchwork_12225_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_execlists:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#109567]

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
    - shard-snb:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_ccs@pipe-b-crc-primary-basic:
    - shard-iclb:         NOTRUN -> FAIL [fdo#107725]

  * igt@kms_cursor_crc@cursor-128x128-random:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103232]

  * igt@kms_cursor_crc@cursor-256x85-onscreen:
    - shard-apl:          PASS -> FAIL [fdo#103232] +4

  * igt@kms_cursor_crc@cursor-alpha-opaque:
    - shard-apl:          PASS -> FAIL [fdo#109350]

  * igt@kms_flip@plain-flip-ts-check-interruptible:
    - shard-kbl:          PASS -> FAIL [fdo#100368]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
    - shard-apl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-glk:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +1

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
    - shard-apl:          PASS -> FAIL [fdo#103166] +5

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
    - shard-iclb:         PASS -> FAIL [fdo#103166] +1

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          PASS -> FAIL [fdo#109016]

  * igt@kms_setmode@basic:
    - shard-hsw:          PASS -> FAIL [fdo#99912]

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-apl:          PASS -> FAIL [fdo#104894]

  * igt@pm_backlight@basic-brightness:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107724] +2

  * igt@pm_rpm@gem-mmap-gtt:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] +1

  
#### Possible fixes ####

  * igt@gem_eio@in-flight-suspend:
    - shard-snb:          FAIL [fdo#103375] -> PASS

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-apl:          FAIL [fdo#103191] / [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-size-change:
    - shard-glk:          FAIL [fdo#103232] -> PASS

  * igt@kms_flip@blocking-wf_vblank:
    - shard-apl:          INCOMPLETE [fdo#103927] -> PASS

  * igt@kms_flip@dpms-vs-vblank-race-interruptible:
    - shard-apl:          FAIL [fdo#103060] -> PASS

  * {igt@kms_flip@flip-vs-suspend}:
    - shard-iclb:         INCOMPLETE [fdo#109507] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
    - shard-apl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +1

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-iclb:         INCOMPLETE [fdo#107713] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-none:
    - shard-glk:          FAIL [fdo#103166] -> PASS

  * igt@kms_psr@no_drrs:
    - shard-iclb:         FAIL [fdo#108341] -> PASS

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         FAIL [fdo#100047] -> PASS

  * igt@kms_universal_plane@universal-plane-pipe-b-functional:
    - shard-apl:          FAIL [fdo#103166] -> PASS

  * igt@pm_rpm@basic-pci-d3-state:
    - shard-iclb:         DMESG-WARN [fdo#107724] -> PASS +6

  * igt@pm_rpm@legacy-planes:
    - shard-iclb:         INCOMPLETE [fdo#108840] / [fdo#109369] -> PASS

  
#### Warnings ####

  * igt@pm_backlight@fade_with_suspend:
    - shard-iclb:         FAIL [fdo#107847] -> DMESG-FAIL [fdo#107724] / [fdo#107847]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
  [fdo#107847]: https://bugs.freedesktop.org/show_bug.cgi?id=107847
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109277]: https://bugs.freedesktop.org/show_bug.cgi?id=109277
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109281]: https://bugs.freedesktop.org/show_bug.cgi?id=109281
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109292]: https://bugs.freedesktop.org/show_bug.cgi?id=109292
  [fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
  [fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
  [fdo#109310]: https://bugs.freedesktop.org/show_bug.cgi?id=109310
  [fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350
  [fdo#109369]: https://bugs.freedesktop.org/show_bug.cgi?id=109369
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
  [fdo#109567]: https://bugs.freedesktop.org/show_bug.cgi?id=109567
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 6)
------------------------------

  Missing    (1): shard-skl 


Build changes
-------------

    * Linux: CI_DRM_5604 -> Patchwork_12225

  CI_DRM_5604: bf979a24473a1b23df86ff3fd0ceb33dc1d62e82 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4827: 395eaffd7e1390c9d6043c2980dc14ce3e08b154 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12225: fbd14aaf53bce8c461f3bbff795d10ccc29721dc @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12225/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/guc: Calling guc_disable_communication in all suspend paths

[Daniele Ceraolo Spurio]

On 2/14/19 1:45 PM, Sujaritha Sundaresan wrote:
> This aim of this patch is to call guc_disable_communication in all
> suspend paths. The reason to introduce this is to resolve a bug that
> occured due to suspend late not being called in the hibernate devices
> path.
> 

And we shouldn't anyway talk to guc after telling it to prepare for suspend.

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reset.c |  2 +-
>   drivers/gpu/drm/i915/intel_uc.c   | 23 +++++++++++++++++++----
>   drivers/gpu/drm/i915/intel_uc.h   |  1 +
>   3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 12e74decd7a2..36e5c9c64285 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -673,7 +673,7 @@ static void reset_prepare(struct drm_i915_private *i915)
>   	for_each_engine(engine, i915, id)
>   		reset_prepare_engine(engine);
>   
> -	intel_uc_sanitize(i915);
> +	intel_uc_reset_prepare(i915);
>   	revoke_mmaps(i915);
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index e711eb3268bc..2d360d53757f 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -332,8 +332,6 @@ void intel_uc_sanitize(struct drm_i915_private *i915)
>   
>   	GEM_BUG_ON(!HAS_GUC(i915));
>   
> -	guc_disable_communication(guc);
> -
>   	intel_huc_sanitize(huc);
>   	intel_guc_sanitize(guc);
>   
> @@ -451,6 +449,23 @@ void intel_uc_fini_hw(struct drm_i915_private *i915)
>   	guc_disable_communication(guc);
>   }
>   
> +/**
> + * intel_uc_reset_prepare - Prepare for reset
> + * @i915: device private
> + *
> + * Preparing for full gpu reset.
> + */
> +void intel_uc_reset_prepare(struct drm_i915_private *i915)
> +{
> +	struct intel_guc *guc = &i915->guc;
> +
> +	if (!USES_GUC(i915))
> +		return;
> +
> +	guc_disable_communication(guc);
> +	intel_uc_sanitize(i915);
> +}
> +
>   int intel_uc_suspend(struct drm_i915_private *i915)
>   {
>   	struct intel_guc *guc = &i915->guc;
> @@ -468,7 +483,7 @@ int intel_uc_suspend(struct drm_i915_private *i915)
>   		return err;
>   	}
>   
> -	gen9_disable_guc_interrupts(i915);
> +	guc_disable_communication(guc);
>   
>   	return 0;
>   }
> @@ -484,7 +499,7 @@ int intel_uc_resume(struct drm_i915_private *i915)
>   	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>   		return 0;
>   
> -	gen9_enable_guc_interrupts(i915);
> +	guc_enable_communication(guc);
>   
>   	err = intel_guc_resume(guc);
>   	if (err) {
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 870faf9011b9..c14729786652 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -38,6 +38,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>   void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
>   int intel_uc_init(struct drm_i915_private *dev_priv);
>   void intel_uc_fini(struct drm_i915_private *dev_priv);
> +void intel_uc_reset_prepare(struct drm_i915_private *i915);
>   int intel_uc_suspend(struct drm_i915_private *dev_priv);
>   int intel_uc_resume(struct drm_i915_private *dev_priv);
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/guc: Splitting CT channel open/close functions

[Sujaritha]

On 2/14/19 2:46 PM, Daniele Ceraolo Spurio wrote:
>
>
> On 2/14/19 1:45 PM, Sujaritha Sundaresan wrote:
>> The aim of this patch is to allow enabling and disabling
>> of CTB without requiring the mutex lock.
>>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc.c    | 12 ++++
>>   drivers/gpu/drm/i915/intel_guc_ct.c | 85 +++++++++++++++++++++--------
>>   drivers/gpu/drm/i915/intel_guc_ct.h |  3 +
>>   3 files changed, 77 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 8660af3fd755..8ecb47087457 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -203,11 +203,19 @@ int intel_guc_init(struct intel_guc *guc)
>>           goto err_log;
>>       GEM_BUG_ON(!guc->ads_vma);
>>   +    if (HAS_GUC_CT(dev_priv)) {
>> +        ret = intel_guc_ct_init(&guc->ct);
>> +        if (ret)
>> +            goto err_ads;
>> +    }
>> +
>>       /* We need to notify the guc whenever we change the GGTT */
>>       i915_ggtt_enable_guc(dev_priv);
>>         return 0;
>>   +err_ads:
>> +    intel_guc_ads_destroy(guc);
>>   err_log:
>>       intel_guc_log_destroy(&guc->log);
>>   err_shared:
>> @@ -222,6 +230,10 @@ void intel_guc_fini(struct intel_guc *guc)
>>       struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>         i915_ggtt_disable_guc(dev_priv);
>> +
>> +    if (HAS_GUC_CT(dev_priv))
>> +        intel_guc_ct_fini(&guc->ct);
>> +
>>       intel_guc_ads_destroy(guc);
>>       intel_guc_log_destroy(&guc->log);
>>       guc_shared_data_destroy(guc);
>> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c 
>> b/drivers/gpu/drm/i915/intel_guc_ct.c
>> index a52883e9146f..fbf9da247975 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
>> @@ -140,9 +140,9 @@ static int guc_action_deregister_ct_buffer(struct 
>> intel_guc *guc,
>>       return err;
>>   }
>>   -static bool ctch_is_open(struct intel_guc_ct_channel *ctch)
>> +static bool ctch_is_enabled(struct intel_guc_ct_channel *ctch)
>>   {
>> -    return ctch->vma != NULL;
>> +    return ctch->is_enabled;
>
> Bikeshed: now that the check is simpler, doing ctch->enabled is 
> actually simpler then ctch_is_enabled(ctch), so I'd prefer to just 
> switch.

Will make this change.

>
>>   }
>>     static int ctch_init(struct intel_guc *guc,
>> @@ -217,22 +217,14 @@ static void ctch_fini(struct intel_guc *guc,
>>       i915_vma_unpin_and_release(&ctch->vma, I915_VMA_RELEASE_MAP);
>>   }
>>   -static int ctch_open(struct intel_guc *guc,
>> +static int ctch_enable(struct intel_guc *guc,
>>                struct intel_guc_ct_channel *ctch)
>>   {
>>       u32 base;
>>       int err;
>>       int i;
>>   -    CT_DEBUG_DRIVER("CT: channel %d reopen=%s\n",
>> -            ctch->owner, yesno(ctch_is_open(ctch)));
>> -
>> -    if (!ctch->vma) {
>> -        err = ctch_init(guc, ctch);
>> -        if (unlikely(err))
>> -            goto err_out;
>> -        GEM_BUG_ON(!ctch->vma);
>> -    }
>> +    GEM_BUG_ON(!ctch->vma);
>
> GEM_BUG_ON(ctch->enabled) as well?


Why would this change ?

>
>>         /* vma should be already allocated and map'ed */
>>       base = intel_guc_ggtt_offset(guc, ctch->vma);
>> @@ -255,7 +247,7 @@ static int ctch_open(struct intel_guc *guc,
>>                           base + PAGE_SIZE/4 * CTB_RECV,
>>                           INTEL_GUC_CT_BUFFER_TYPE_RECV);
>>       if (unlikely(err))
>> -        goto err_fini;
>> +        goto err_out;
>>         err = guc_action_register_ct_buffer(guc,
>>                           base + PAGE_SIZE/4 * CTB_SEND,
>> @@ -263,23 +255,25 @@ static int ctch_open(struct intel_guc *guc,
>>       if (unlikely(err))
>>           goto err_deregister;
>>   +    ctch->is_enabled = true;
>> +
>>       return 0;
>>     err_deregister:
>>       guc_action_deregister_ct_buffer(guc,
>>                       ctch->owner,
>>                       INTEL_GUC_CT_BUFFER_TYPE_RECV);
>> -err_fini:
>> -    ctch_fini(guc, ctch);
>>   err_out:
>>       DRM_ERROR("CT: can't open channel %d; err=%d\n", ctch->owner, 
>> err);
>>       return err;
>>   }
>>   -static void ctch_close(struct intel_guc *guc,
>> +static void ctch_disable(struct intel_guc *guc,
>>                  struct intel_guc_ct_channel *ctch)
>>   {
>> -    GEM_BUG_ON(!ctch_is_open(ctch));
>> +    GEM_BUG_ON(!ctch_is_enabled(ctch));
>> +
>> +    ctch->is_enabled = false;
>>         guc_action_deregister_ct_buffer(guc,
>>                       ctch->owner,
>> @@ -287,7 +281,6 @@ static void ctch_close(struct intel_guc *guc,
>>       guc_action_deregister_ct_buffer(guc,
>>                       ctch->owner,
>>                       INTEL_GUC_CT_BUFFER_TYPE_RECV);
>> -    ctch_fini(guc, ctch);
>>   }
>>     static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
>> @@ -481,7 +474,7 @@ static int ctch_send(struct intel_guc_ct *ct,
>>       u32 fence;
>>       int err;
>>   -    GEM_BUG_ON(!ctch_is_open(ctch));
>> +    GEM_BUG_ON(!ctch_is_enabled(ctch));
>>       GEM_BUG_ON(!len);
>>       GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>>       GEM_BUG_ON(!response_buf && response_buf_size);
>> @@ -817,7 +810,7 @@ static void ct_process_host_channel(struct 
>> intel_guc_ct *ct)
>>       u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the 
>> header */
>>       int err = 0;
>>   -    if (!ctch_is_open(ctch))
>> +    if (!ctch_is_enabled(ctch))
>>           return;
>>         do {
>> @@ -848,6 +841,49 @@ static void 
>> intel_guc_to_host_event_handler_ct(struct intel_guc *guc)
>>       ct_process_host_channel(ct);
>>   }
>>   +/**
>> + * intel_guc_ct_init - Allocate CT
>
> You're not really allocating the CT struct, but the internal channel. 
> Maybe just say "init CT communication" here and then say in the 
> description below "allocate memory required for communication via the 
> CT channel".
>
> Similar thing for the fini path.


Will fix the kernel doc.

>
>> + * @ct: pointer to CT struct
>> + *
>> + * Shall only be called for platforms with HAS_GUC_CT.
>> + *
>> + * Return: 0 on success, a negative errno code on failure.
>> + */
>> +int intel_guc_ct_init(struct intel_guc_ct *ct)
>> +{
>> +    struct intel_guc *guc = ct_to_guc(ct);
>> +    struct intel_guc_ct_channel *ctch = &ct->host_channel;
>> +    int err;
>> +
>> +    GEM_BUG_ON(ctch->vma);
>
> the BUG_ON is already in ctch_init(), no need to duplicate.


Will remove this BUG_ON.

>
>> +
>> +    err = ctch_init(guc, ctch);
>> +    if (unlikely(err)) {
>> +        DRM_ERROR("CT: can't open channel %d; err=%d\n",
>> +            ctch->owner, err);
>> +        return err;
>> +    }
>> +
>> +    GEM_BUG_ON(!ctch->vma);
>> +    return 0;
>> +}
>> +
>> +/**
>> + * intel_guc_ct_fini - Deallocate CT
>> + * @ct: pointer to CT struct
>> + *
>> + * Shall only be called for platforms with HAS_GUC_CT.
>> + */
>> +void intel_guc_ct_fini(struct intel_guc_ct *ct)
>> +{
>> +    struct intel_guc *guc = ct_to_guc(ct);
>> +    struct intel_guc_ct_channel *ctch = &ct->host_channel;
>> +
>> +    GEM_BUG_ON(ctch_is_enabled(ctch));
>
> We could move this inside ctch_fini() to keep all the ctch BUG_ONs 
> inside ctch_* functions.

Will make this change as well.

>
>> +
>> +    ctch_fini(guc, ctch);
>> +}
>> +
>>   /**
>>    * intel_guc_ct_enable - Enable buffer based command transport.
>>    * @ct: pointer to CT struct
>> @@ -865,7 +901,10 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
>>         GEM_BUG_ON(!HAS_GUC_CT(i915));
>>   -    err = ctch_open(guc, ctch);
>> +    if (ctch_is_enabled(ctch))
>> +        return 0;
>> +
>> +    err = ctch_enable(guc, ctch);
>>       if (unlikely(err))
>>           return err;
>>   @@ -890,10 +929,10 @@ void intel_guc_ct_disable(struct intel_guc_ct 
>> *ct)
>>         GEM_BUG_ON(!HAS_GUC_CT(i915));
>>   -    if (!ctch_is_open(ctch))
>> +    if (!ctch_is_enabled(ctch))
>>           return;
>>   -    ctch_close(guc, ctch);
>> +    ctch_disable(guc, ctch);
>>         /* Disable send */
>>       guc->send = intel_guc_send_nop;
>> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h 
>> b/drivers/gpu/drm/i915/intel_guc_ct.h
>> index d774895ab143..e05fdc3cf0b0 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_ct.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
>> @@ -64,6 +64,7 @@ struct intel_guc_ct_buffer {
>>   struct intel_guc_ct_channel {
>>       struct i915_vma *vma;
>>       struct intel_guc_ct_buffer ctbs[2];
>> +    bool is_enabled;
>
> just "enabled" is fine.
> Daniele


Will make it "enabled". Thanks for the review.

Sujaritha

>
>>       u32 owner;
>>       u32 next_fence;
>>   };
>> @@ -90,6 +91,8 @@ struct intel_guc_ct {
>>   };
>>     void intel_guc_ct_init_early(struct intel_guc_ct *ct);
>> +int intel_guc_ct_init(struct intel_guc_ct *ct);
>> +void intel_guc_ct_fini(struct intel_guc_ct *ct);
>>   int intel_guc_ct_enable(struct intel_guc_ct *ct);
>>   void intel_guc_ct_disable(struct intel_guc_ct *ct);
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/guc: Calling guc_disable_communication in all suspend paths

[Sujaritha]

On 2/15/19 5:28 PM, Daniele Ceraolo Spurio wrote:
>
>
> On 2/14/19 1:45 PM, Sujaritha Sundaresan wrote:
>> This aim of this patch is to call guc_disable_communication in all
>> suspend paths. The reason to introduce this is to resolve a bug that
>> occured due to suspend late not being called in the hibernate devices
>> path.
>>
>
> And we shouldn't anyway talk to guc after telling it to prepare for 
> suspend.
>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> Daniele


Will include this in the commit message. Thanks for the review.

Sujaritha

>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reset.c |  2 +-
>>   drivers/gpu/drm/i915/intel_uc.c   | 23 +++++++++++++++++++----
>>   drivers/gpu/drm/i915/intel_uc.h   |  1 +
>>   3 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reset.c 
>> b/drivers/gpu/drm/i915/i915_reset.c
>> index 12e74decd7a2..36e5c9c64285 100644
>> --- a/drivers/gpu/drm/i915/i915_reset.c
>> +++ b/drivers/gpu/drm/i915/i915_reset.c
>> @@ -673,7 +673,7 @@ static void reset_prepare(struct drm_i915_private 
>> *i915)
>>       for_each_engine(engine, i915, id)
>>           reset_prepare_engine(engine);
>>   -    intel_uc_sanitize(i915);
>> +    intel_uc_reset_prepare(i915);
>>       revoke_mmaps(i915);
>>   }
>>   diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index e711eb3268bc..2d360d53757f 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -332,8 +332,6 @@ void intel_uc_sanitize(struct drm_i915_private 
>> *i915)
>>         GEM_BUG_ON(!HAS_GUC(i915));
>>   -    guc_disable_communication(guc);
>> -
>>       intel_huc_sanitize(huc);
>>       intel_guc_sanitize(guc);
>>   @@ -451,6 +449,23 @@ void intel_uc_fini_hw(struct drm_i915_private 
>> *i915)
>>       guc_disable_communication(guc);
>>   }
>>   +/**
>> + * intel_uc_reset_prepare - Prepare for reset
>> + * @i915: device private
>> + *
>> + * Preparing for full gpu reset.
>> + */
>> +void intel_uc_reset_prepare(struct drm_i915_private *i915)
>> +{
>> +    struct intel_guc *guc = &i915->guc;
>> +
>> +    if (!USES_GUC(i915))
>> +        return;
>> +
>> +    guc_disable_communication(guc);
>> +    intel_uc_sanitize(i915);
>> +}
>> +
>>   int intel_uc_suspend(struct drm_i915_private *i915)
>>   {
>>       struct intel_guc *guc = &i915->guc;
>> @@ -468,7 +483,7 @@ int intel_uc_suspend(struct drm_i915_private *i915)
>>           return err;
>>       }
>>   -    gen9_disable_guc_interrupts(i915);
>> +    guc_disable_communication(guc);
>>         return 0;
>>   }
>> @@ -484,7 +499,7 @@ int intel_uc_resume(struct drm_i915_private *i915)
>>       if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>           return 0;
>>   -    gen9_enable_guc_interrupts(i915);
>> +    guc_enable_communication(guc);
>>         err = intel_guc_resume(guc);
>>       if (err) {
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h 
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index 870faf9011b9..c14729786652 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -38,6 +38,7 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv);
>>   void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
>>   int intel_uc_init(struct drm_i915_private *dev_priv);
>>   void intel_uc_fini(struct drm_i915_private *dev_priv);
>> +void intel_uc_reset_prepare(struct drm_i915_private *i915);
>>   int intel_uc_suspend(struct drm_i915_private *dev_priv);
>>   int intel_uc_resume(struct drm_i915_private *dev_priv);
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx