[PATCH 1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

[PATCH 1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

[Michał Winiarski]

We're seeing "RPM wakelock ref not held during HW access" warning
otherwise. And since IRQ are synced for runtime suspend, we can use the
variant without wakeref assert.

Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Marta Löfstedt <marta.lofstedt@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 8f93f5bef8fd..6787a3116783 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -391,9 +391,9 @@ void intel_guc_to_host_event_handler(struct intel_guc *guc)
 	 * clears out the bit on handling the 1st interrupt.
 	 */
 	spin_lock(&guc->irq_lock);
-	val = I915_READ(SOFT_SCRATCH(15));
+	val = I915_READ_FW(SOFT_SCRATCH(15));
 	msg = val & guc->msg_enabled_mask;
-	I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
+	I915_WRITE_FW(SOFT_SCRATCH(15), val & ~msg);
 	spin_unlock(&guc->irq_lock);
 
 	if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
-- 
2.14.3

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

[PATCH 2/8] drm/i915/guc: Move FW size sanity check back to fetch

[Michał Winiarski]

While we need to know WOPCM size to do this sanity check, it has more to
do with FW than with WOPCM. Let's move the check to fetch phase, it's
not like WOPCM is going to grow in the meantime. We're also reducing
some of the code movement in following patches.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jackie Li <yaodong.li@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_uc_fw.c | 13 ++++++++++++-
 drivers/gpu/drm/i915/intel_wopcm.c | 12 ------------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index 30c73243f54d..2ec8ec1c8b95 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -46,6 +46,8 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	size_t size;
 	int err;
 
+	GEM_BUG_ON(!dev_priv->wopcm.size);
+
 	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
 			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
 
@@ -105,8 +107,17 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 	uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
 	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
 
+	/* Sanity check whether our fw is not larger than our whole memory */
+	size = uc_fw->header_size + uc_fw->ucode_size;
+	if (size >= dev_priv->wopcm.size) {
+		DRM_ERROR("%s: (%zuKiB) is too big to fit in WOPCM.",
+			  intel_uc_fw_type_repr(uc_fw->type), size / 1024);
+		err = -E2BIG;
+		goto fail;
+	}
+
 	/* At least, it should have header, uCode and RSA. Size of all three. */
-	size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
+	size += uc_fw->rsa_size;
 	if (fw->size < size) {
 		DRM_WARN("%s: Truncated firmware (%zu, expected %zu)\n",
 			 intel_uc_fw_type_repr(uc_fw->type), fw->size, size);
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 4117886bfb05..42876f8890e7 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -163,18 +163,6 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 
 	GEM_BUG_ON(!wopcm->size);
 
-	if (guc_fw_size >= wopcm->size) {
-		DRM_ERROR("GuC FW (%uKiB) is too big to fit in WOPCM.",
-			  guc_fw_size / 1024);
-		return -E2BIG;
-	}
-
-	if (huc_fw_size >= wopcm->size) {
-		DRM_ERROR("HuC FW (%uKiB) is too big to fit in WOPCM.",
-			  huc_fw_size / 1024);
-		return -E2BIG;
-	}
-
 	guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
 			       GUC_WOPCM_OFFSET_ALIGNMENT);
 	if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
-- 
2.14.3

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

[PATCH 3/8] drm/i915/guc: Extend the GEN9 WOPCM HW restrictions to early CNL

[Michał Winiarski]

We imposed additional restrictions to GEN9 WOPCM partitioning. However,
we ignored early steppings of CNL, to which those restrictions also
apply. Let's also tweak the logic a bit by having separate helpers for
returning extra size needed for the restriction to be satisfied, and
handle the results in the caller.

References: 6b0478fb722a ("drm/i915: Implement dynamic GuC WOPCM offset and size calculation")
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jackie Li <yaodong.li@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_wopcm.c | 63 ++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 42876f8890e7..50854a6b9493 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -84,58 +84,69 @@ static inline u32 context_reserved_size(struct drm_i915_private *i915)
 		return 0;
 }
 
-static inline int gen9_check_dword_gap(u32 guc_wopcm_base, u32 guc_wopcm_size)
+static u32
+gen9_size_for_dword_gap_restriction(u32 guc_wopcm_base, u32 guc_wopcm_size)
 {
-	u32 offset;
+	s32 additional_size;
 
 	/*
 	 * GuC WOPCM size shall be at least a dword larger than the offset from
 	 * WOPCM base (GuC WOPCM offset from WOPCM base + GEN9_GUC_WOPCM_OFFSET)
 	 * due to hardware limitation on Gen9.
 	 */
-	offset = guc_wopcm_base + GEN9_GUC_WOPCM_OFFSET;
-	if (offset > guc_wopcm_size ||
-	    (guc_wopcm_size - offset) < sizeof(u32)) {
-		DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB needed.\n",
-			  guc_wopcm_size / 1024,
-			  (u32)(offset + sizeof(u32)) / 1024);
-		return -E2BIG;
-	}
+	additional_size = guc_wopcm_base + GEN9_GUC_WOPCM_OFFSET +
+			  sizeof(u32) - guc_wopcm_size;
 
-	return 0;
+	if (additional_size <= 0)
+		return 0;
+
+	return additional_size;
 }
 
-static inline int gen9_check_huc_fw_fits(u32 guc_wopcm_size, u32 huc_fw_size)
+static u32
+gen9_size_for_huc_restriction(u32 guc_wopcm_size, u32 huc_fw_size)
 {
+	s32 additional_size;
+
 	/*
 	 * On Gen9 & CNL A0, hardware requires the total available GuC WOPCM
 	 * size to be larger than or equal to HuC firmware size. Otherwise,
 	 * firmware uploading would fail.
 	 */
-	if (huc_fw_size > guc_wopcm_size - GUC_WOPCM_RESERVED) {
-		DRM_ERROR("HuC FW (%uKiB) won't fit in GuC WOPCM (%uKiB).\n",
-			  huc_fw_size / 1024,
-			  (guc_wopcm_size - GUC_WOPCM_RESERVED) / 1024);
-		return -E2BIG;
-	}
+	additional_size = huc_fw_size - (guc_wopcm_size - GUC_WOPCM_RESERVED);
 
-	return 0;
+	if (additional_size <= 0)
+		return 0;
+
+	return additional_size;
 }
 
 static inline int check_hw_restriction(struct drm_i915_private *i915,
 				       u32 guc_wopcm_base, u32 guc_wopcm_size,
 				       u32 huc_fw_size)
 {
-	int err = 0;
+	u32 size;
+
+	if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) {
+		size = gen9_size_for_dword_gap_restriction(guc_wopcm_base,
+							   guc_wopcm_size);
+		if (size)
+			goto err;
+
+		size = gen9_size_for_huc_restriction(guc_wopcm_size,
+						     huc_fw_size);
+		if (size)
+			goto err;
+	}
 
-	if (IS_GEN9(i915))
-		err = gen9_check_dword_gap(guc_wopcm_base, guc_wopcm_size);
+	return 0;
 
-	if (!err &&
-	    (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)))
-		err = gen9_check_huc_fw_fits(guc_wopcm_size, huc_fw_size);
+err:
+	DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
+		  guc_wopcm_size / 1024,
+		  size / 1024);
 
-	return err;
+	return -E2BIG;
 }
 
 /**
-- 
2.14.3

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

Re: [PATCH 3/8] drm/i915/guc: Extend the GEN9 WOPCM HW restrictions to early CNL

[Daniele Ceraolo Spurio]

On 23/03/18 05:34, Michał Winiarski wrote:
> We imposed additional restrictions to GEN9 WOPCM partitioning. However,
> we ignored early steppings of CNL, to which those restrictions also
> apply. Let's also tweak the logic a bit by having separate helpers for
> returning extra size needed for the restriction to be satisfied, and
> handle the results in the caller.
> 
> References: 6b0478fb722a ("drm/i915: Implement dynamic GuC WOPCM offset and size calculation")
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jackie Li <yaodong.li@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_wopcm.c | 63 ++++++++++++++++++++++----------------
>   1 file changed, 37 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
> index 42876f8890e7..50854a6b9493 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -84,58 +84,69 @@ static inline u32 context_reserved_size(struct drm_i915_private *i915)
>   		return 0;
>   }
>   
> -static inline int gen9_check_dword_gap(u32 guc_wopcm_base, u32 guc_wopcm_size)
> +static u32
> +gen9_size_for_dword_gap_restriction(u32 guc_wopcm_base, u32 guc_wopcm_size)
>   {
> -	u32 offset;
> +	s32 additional_size;
>   
>   	/*
>   	 * GuC WOPCM size shall be at least a dword larger than the offset from
>   	 * WOPCM base (GuC WOPCM offset from WOPCM base + GEN9_GUC_WOPCM_OFFSET)
>   	 * due to hardware limitation on Gen9.
>   	 */
> -	offset = guc_wopcm_base + GEN9_GUC_WOPCM_OFFSET;
> -	if (offset > guc_wopcm_size ||
> -	    (guc_wopcm_size - offset) < sizeof(u32)) {
> -		DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB needed.\n",
> -			  guc_wopcm_size / 1024,
> -			  (u32)(offset + sizeof(u32)) / 1024);
> -		return -E2BIG;
> -	}
> +	additional_size = guc_wopcm_base + GEN9_GUC_WOPCM_OFFSET +

GUC_FW_RESERVED is 256k for CNL (Bspec: 1184) so we need a 
GEN10_GUC_WOPCM_OFFSET.

Daniele

> +			  sizeof(u32) - guc_wopcm_size;
>   
> -	return 0;
> +	if (additional_size <= 0)
> +		return 0;
> +
> +	return additional_size;
>   }
>   
> -static inline int gen9_check_huc_fw_fits(u32 guc_wopcm_size, u32 huc_fw_size)
> +static u32
> +gen9_size_for_huc_restriction(u32 guc_wopcm_size, u32 huc_fw_size)
>   {
> +	s32 additional_size;
> +
>   	/*
>   	 * On Gen9 & CNL A0, hardware requires the total available GuC WOPCM
>   	 * size to be larger than or equal to HuC firmware size. Otherwise,
>   	 * firmware uploading would fail.
>   	 */
> -	if (huc_fw_size > guc_wopcm_size - GUC_WOPCM_RESERVED) {
> -		DRM_ERROR("HuC FW (%uKiB) won't fit in GuC WOPCM (%uKiB).\n",
> -			  huc_fw_size / 1024,
> -			  (guc_wopcm_size - GUC_WOPCM_RESERVED) / 1024);
> -		return -E2BIG;
> -	}
> +	additional_size = huc_fw_size - (guc_wopcm_size - GUC_WOPCM_RESERVED);
>   
> -	return 0;
> +	if (additional_size <= 0)
> +		return 0;
> +
> +	return additional_size;
>   }
>   
>   static inline int check_hw_restriction(struct drm_i915_private *i915,
>   				       u32 guc_wopcm_base, u32 guc_wopcm_size,
>   				       u32 huc_fw_size)
>   {
> -	int err = 0;
> +	u32 size;
> +
> +	if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) {
> +		size = gen9_size_for_dword_gap_restriction(guc_wopcm_base,
> +							   guc_wopcm_size);
> +		if (size)
> +			goto err;
> +
> +		size = gen9_size_for_huc_restriction(guc_wopcm_size,
> +						     huc_fw_size);
> +		if (size)
> +			goto err;
> +	}
>   
> -	if (IS_GEN9(i915))
> -		err = gen9_check_dword_gap(guc_wopcm_base, guc_wopcm_size);
> +	return 0;
>   
> -	if (!err &&
> -	    (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)))
> -		err = gen9_check_huc_fw_fits(guc_wopcm_size, huc_fw_size);
> +err:
> +	DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
> +		  guc_wopcm_size / 1024,
> +		  size / 1024);
>   
> -	return err;
> +	return -E2BIG;
>   }
>   
>   /**
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/8] drm/i915/guc: Separate WOPCM partitioning from constraints check

[Michał Winiarski]

In the following patches we're going to support constraints checking on
an already locked partitioning. Let's structure the code now to allow
for code reuse and reduce the churn later on.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jackie Li <yaodong.li@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_wopcm.c | 141 +++++++++++++++++++++++++++----------
 1 file changed, 102 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 50854a6b9493..52841d340002 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -84,6 +84,17 @@ static inline u32 context_reserved_size(struct drm_i915_private *i915)
 		return 0;
 }
 
+static inline u32 guc_fw_size_in_wopcm(u32 guc_fw_size)
+{
+	return ALIGN(guc_fw_size + GUC_WOPCM_RESERVED +
+		     GUC_WOPCM_STACK_RESERVED, PAGE_SIZE);
+}
+
+static inline u32 huc_fw_size_in_wopcm(u32 huc_fw_size)
+{
+	return huc_fw_size + WOPCM_RESERVED_SIZE;
+}
+
 static u32
 gen9_size_for_dword_gap_restriction(u32 guc_wopcm_base, u32 guc_wopcm_size)
 {
@@ -121,19 +132,54 @@ gen9_size_for_huc_restriction(u32 guc_wopcm_size, u32 huc_fw_size)
 	return additional_size;
 }
 
-static inline int check_hw_restriction(struct drm_i915_private *i915,
-				       u32 guc_wopcm_base, u32 guc_wopcm_size,
-				       u32 huc_fw_size)
+static int check_huc_fw_fits(struct intel_wopcm *wopcm, u32 huc_fw_size)
+{
+	if (huc_fw_size_in_wopcm(huc_fw_size) > wopcm->guc.base) {
+		DRM_ERROR("Need %uKiB WOPCM for HuC, %uKiB available.\n",
+			  huc_fw_size_in_wopcm(huc_fw_size) / 1024,
+			  wopcm->guc.base / 1024);
+		return -E2BIG;
+	}
+
+	return 0;
+}
+
+static int check_guc_fw_fits(struct intel_wopcm *wopcm, u32 guc_fw_size)
+{
+	if (guc_fw_size_in_wopcm(guc_fw_size) > wopcm->guc.size) {
+		DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
+			  huc_fw_size_in_wopcm(guc_fw_size) / 1024,
+			  wopcm->guc.size / 1024);
+		return -E2BIG;
+	}
+
+	return 0;
+}
+
+static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)
+{
+	if ((wopcm->guc.base + wopcm->guc.size + ctx_rsvd) > wopcm->size) {
+		DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
+			  wopcm->guc.base / 1024);
+		return -E2BIG;
+	}
+
+	return 0;
+}
+
+static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
 {
+	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
 	u32 size;
 
 	if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) {
-		size = gen9_size_for_dword_gap_restriction(guc_wopcm_base,
-							   guc_wopcm_size);
+		size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
+							   wopcm->guc.size);
 		if (size)
 			goto err;
 
-		size = gen9_size_for_huc_restriction(guc_wopcm_size,
+		size = gen9_size_for_huc_restriction(wopcm->guc.size,
 						     huc_fw_size);
 		if (size)
 			goto err;
@@ -143,12 +189,54 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
 
 err:
 	DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
-		  guc_wopcm_size / 1024,
+		  wopcm->guc.size / 1024,
 		  size / 1024);
 
 	return -E2BIG;
 }
 
+static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
+{
+	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
+	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
+	u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw);
+	u32 ctx_rsvd = context_reserved_size(i915);
+	int err;
+
+	err = check_huc_fw_fits(wopcm, huc_fw_size);
+	if (err)
+		return err;
+
+	err = check_guc_fw_fits(wopcm, guc_fw_size);
+	if (err)
+		return err;
+
+	err = check_ctx_rsvd_fits(wopcm, ctx_rsvd);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int wopcm_guc_init(struct intel_wopcm *wopcm)
+{
+	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
+	u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
+	u32 ctx_rsvd = context_reserved_size(dev_priv);
+
+	wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),
+				     GUC_WOPCM_OFFSET_ALIGNMENT);
+
+	wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,
+				PAGE_SIZE);
+
+	DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
+			 wopcm->guc.base / 1024,
+			 (wopcm->guc.base + wopcm->guc.size) / 1024);
+
+	return 0;
+}
+
 /**
  * intel_wopcm_init() - Initialize the WOPCM structure.
  * @wopcm: pointer to intel_wopcm.
@@ -163,46 +251,21 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
  */
 int intel_wopcm_init(struct intel_wopcm *wopcm)
 {
-	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
-	u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw);
-	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
-	u32 ctx_rsvd = context_reserved_size(i915);
-	u32 guc_wopcm_base;
-	u32 guc_wopcm_size;
-	u32 guc_wopcm_rsvd;
 	int err;
 
 	GEM_BUG_ON(!wopcm->size);
 
-	guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
-			       GUC_WOPCM_OFFSET_ALIGNMENT);
-	if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
-		DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
-			  guc_wopcm_base / 1024);
-		return -E2BIG;
-	}
-
-	guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
-	guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
-
-	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
-			 guc_wopcm_base / 1024, guc_wopcm_size / 1024);
-
-	guc_wopcm_rsvd = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
-	if ((guc_fw_size + guc_wopcm_rsvd) > guc_wopcm_size) {
-		DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
-			  (guc_fw_size + guc_wopcm_rsvd) / 1024,
-			  guc_wopcm_size / 1024);
-		return -E2BIG;
-	}
+	err = wopcm_guc_init(wopcm);
+	if (err)
+		return err;
 
-	err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
-				   huc_fw_size);
+	err = wopcm_check_hw_restrictions(wopcm);
 	if (err)
 		return err;
 
-	wopcm->guc.base = guc_wopcm_base;
-	wopcm->guc.size = guc_wopcm_size;
+	err = wopcm_check_components_fit(wopcm);
+	if (err)
+		return err;
 
 	return 0;
 }
-- 
2.14.3

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

Re: [PATCH 4/8] drm/i915/guc: Separate WOPCM partitioning from constraints check

[Yaodong Li]

On 03/23/2018 05:34 AM, Michał Winiarski wrote:
> In the following patches we're going to support constraints checking on
> an already locked partitioning. Let's structure the code now to allow
> for code reuse and reduce the churn later on.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jackie Li <yaodong.li@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_wopcm.c | 141 +++++++++++++++++++++++++++----------
>   1 file changed, 102 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
> index 50854a6b9493..52841d340002 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -84,6 +84,17 @@ static inline u32 context_reserved_size(struct drm_i915_private *i915)
>   		return 0;
>   }
>   
> +static inline u32 guc_fw_size_in_wopcm(u32 guc_fw_size)
> +{
> +	return ALIGN(guc_fw_size + GUC_WOPCM_RESERVED +
> +		     GUC_WOPCM_STACK_RESERVED, PAGE_SIZE);
> +}
> +
> +static inline u32 huc_fw_size_in_wopcm(u32 huc_fw_size)
> +{
> +	return huc_fw_size + WOPCM_RESERVED_SIZE;
> +}
> +
>   static u32
>   gen9_size_for_dword_gap_restriction(u32 guc_wopcm_base, u32 guc_wopcm_size)
>   {
> @@ -121,19 +132,54 @@ gen9_size_for_huc_restriction(u32 guc_wopcm_size, u32 huc_fw_size)
>   	return additional_size;
>   }
>   
> -static inline int check_hw_restriction(struct drm_i915_private *i915,
> -				       u32 guc_wopcm_base, u32 guc_wopcm_size,
> -				       u32 huc_fw_size)
> +static int check_huc_fw_fits(struct intel_wopcm *wopcm, u32 huc_fw_size)
inline?
> +{
> +	if (huc_fw_size_in_wopcm(huc_fw_size) > wopcm->guc.base) {
> +		DRM_ERROR("Need %uKiB WOPCM for HuC, %uKiB available.\n",
> +			  huc_fw_size_in_wopcm(huc_fw_size) / 1024,
> +			  wopcm->guc.base / 1024);
> +		return -E2BIG;
> +	}
> +
> +	return 0;
> +}
> +
> +static int check_guc_fw_fits(struct intel_wopcm *wopcm, u32 guc_fw_size)
inline?
> +{
> +	if (guc_fw_size_in_wopcm(guc_fw_size) > wopcm->guc.size) {
> +		DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
> +			  huc_fw_size_in_wopcm(guc_fw_size) / 1024,
> +			  wopcm->guc.size / 1024);
> +		return -E2BIG;
> +	}
> +
> +	return 0;
> +}
> +
> +static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)
inline?
> +{
> +	if ((wopcm->guc.base + wopcm->guc.size + ctx_rsvd) > wopcm->size) {
> +		DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
> +			  wopcm->guc.base / 1024);
> +		return -E2BIG;
> +	}
> +
> +	return 0;
> +}
> +
> +static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
>   {
We are repeating the the old discussion over the parameters here.

we wanted to keep wopcm contains either valid (non-zero) values
or to be zero all the time, so that we can make a check such as
if (!wopcm->guc.size) then it's valid. Originally, we needed check this
before accessing it in guc_ggtt_offset. Since we are not doing so
maybe we can change it back in this way, or just keep it as it is now
to continue gaining the benefit that it will continue contains valid data?
> +	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> +	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
>   	u32 size;
>   
>   	if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) {
> -		size = gen9_size_for_dword_gap_restriction(guc_wopcm_base,
> -							   guc_wopcm_size);
> +		size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
> +							   wopcm->guc.size);
>   		if (size)
>   			goto err;
>   
> -		size = gen9_size_for_huc_restriction(guc_wopcm_size,
> +		size = gen9_size_for_huc_restriction(wopcm->guc.size,
>   						     huc_fw_size);
>   		if (size)
>   			goto err;
> @@ -143,12 +189,54 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
>   
>   err:
>   	DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
> -		  guc_wopcm_size / 1024,
> +		  wopcm->guc.size / 1024,
>   		  size / 1024);
>   
>   	return -E2BIG;
>   }
>   
> +static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
> +{
> +	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> +	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
> +	u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw);
> +	u32 ctx_rsvd = context_reserved_size(i915);
> +	int err;
> +
> +	err = check_huc_fw_fits(wopcm, huc_fw_size);
> +	if (err)
> +		return err;
> +
> +	err = check_guc_fw_fits(wopcm, guc_fw_size);
> +	if (err)
> +		return err;
> +
> +	err = check_ctx_rsvd_fits(wopcm, ctx_rsvd);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static int wopcm_guc_init(struct intel_wopcm *wopcm)
> +{
> +	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
> +	u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
> +	u32 ctx_rsvd = context_reserved_size(dev_priv);
> +
> +	wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),
> +				     GUC_WOPCM_OFFSET_ALIGNMENT);
I think we should grow it to the alignment boundary instead of
trim it down.
> +
> +	wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,
> +				PAGE_SIZE);
Hmm, what if wopcm->guc.base + ctx_rsvd > wopcm->size? we have
no guarantee on what the value would be after doing an add:)
Plus, we should trim it down instead of growing it up:) since we've already
got the maximum available size with wopcm->size - wopcm->guc.base - 
ctx_rsvd.

Plus, No PAGE_SIZE since it's not fixed to 4K.
> +
> +	DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
> +			 wopcm->guc.base / 1024,
> +			 (wopcm->guc.base + wopcm->guc.size) / 1024);
> +
> +	return 0;
> +}
> +
>   /**
>    * intel_wopcm_init() - Initialize the WOPCM structure.
>    * @wopcm: pointer to intel_wopcm.
> @@ -163,46 +251,21 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
>    */
>   int intel_wopcm_init(struct intel_wopcm *wopcm)
>   {
> -	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> -	u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915->guc.fw);
> -	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
> -	u32 ctx_rsvd = context_reserved_size(i915);
> -	u32 guc_wopcm_base;
> -	u32 guc_wopcm_size;
> -	u32 guc_wopcm_rsvd;
>   	int err;
>   
>   	GEM_BUG_ON(!wopcm->size);
>   
> -	guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
> -			       GUC_WOPCM_OFFSET_ALIGNMENT);
> -	if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
> -		DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
> -			  guc_wopcm_base / 1024);
> -		return -E2BIG;
> -	}
> -
> -	guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
> -	guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
> -
> -	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
> -			 guc_wopcm_base / 1024, guc_wopcm_size / 1024);
> -
> -	guc_wopcm_rsvd = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
> -	if ((guc_fw_size + guc_wopcm_rsvd) > guc_wopcm_size) {
> -		DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
> -			  (guc_fw_size + guc_wopcm_rsvd) / 1024,
> -			  guc_wopcm_size / 1024);
> -		return -E2BIG;
> -	}
> +	err = wopcm_guc_init(wopcm);
> +	if (err)
> +		return err;
>   
> -	err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
> -				   huc_fw_size);
> +	err = wopcm_check_hw_restrictions(wopcm);
>   	if (err)
>   		return err;
>   
> -	wopcm->guc.base = guc_wopcm_base;
> -	wopcm->guc.size = guc_wopcm_size;
> +	err = wopcm_check_components_fit(wopcm);
Check fits here seems a little bit overkill, especially for the checks
such as check_huc_fw_fit() and check_ctx_rsvd_fit() against the calculated
values at this points since these values are actually calculated by the 
facts that
guc_wopcm_base should definitely larger than huc_fw_size since we 
calculated it
with huc_fw_size + WOPCM_RESERVED_SIZE which means
check_huc_fw_fits should always returns true
and guc_wopcm_size is based on wopcm->size - wopcm->guc.base - ctx_rsvd
which means check_ctx_rsvd_fit() will always return true for this check 
as well.

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

[PATCH 5/8] drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition

[Michał Winiarski]

We need GuC to load HuC, but it's also possible for GuC to operate on
its own. We don't know what the size of HuC FW may be, so, not wanting
to make any platform-specific hardcoded guesses, we assume that its size
is 0... Which is a very bad approximation.
This has a very unfortunate consequence - once we've booted with GuC and
no HuC, we'll never be able to load HuC (unless we reboot, since the
registers are locked).
Rather than using unknown values in our computations - let's partition
based on GuC size.
We have one HW restriction where we're using HuC size (GuC size needs to
be roughly similar to HuC size - which may be unknown at this point),
luckily, another HW restriction makes it very unlikely to run in any
sort of issues in this case.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jackie Li <yaodong.li@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 52841d340002..295d302e97b9 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)
 	return 0;
 }
 
-static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
+static inline void
+__guc_region_grow(struct intel_wopcm *wopcm, u32 size)
+{
+	/*
+	 * We're growing guc region in the direction of lower addresses.
+	 * We need to use multiples of base alignment, because it has more
+	 * strict alignment rules.
+	 */
+	size = DIV_ROUND_UP(size, 2);
+	size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT);
+
+	wopcm->guc.base -= size;
+	wopcm->guc.size += size;
+}
+
+static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
 	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
@@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
 		size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
 							   wopcm->guc.size);
 		if (size)
-			goto err;
+			__guc_region_grow(wopcm, size);
 
 		size = gen9_size_for_huc_restriction(wopcm->guc.size,
 						     huc_fw_size);
 		if (size)
-			goto err;
-	}
-
-	return 0;
+			__guc_region_grow(wopcm, size);
 
-err:
-	DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
-		  wopcm->guc.size / 1024,
-		  size / 1024);
-
-	return -E2BIG;
+		GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base,
+							       wopcm->guc.size));
+		GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size,
+							 huc_fw_size));
+	}
 }
 
 static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
@@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
 	return 0;
 }
 
-static int wopcm_guc_init(struct intel_wopcm *wopcm)
+static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
-	u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
+	u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw);
 	u32 ctx_rsvd = context_reserved_size(dev_priv);
 
-	wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),
-				     GUC_WOPCM_OFFSET_ALIGNMENT);
+	wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size);
 
-	wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,
-				PAGE_SIZE);
-
-	DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
-			 wopcm->guc.base / 1024,
-			 (wopcm->guc.base + wopcm->guc.size) / 1024);
+	wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size,
+				     GUC_WOPCM_OFFSET_ALIGNMENT);
 
 	return 0;
 }
@@ -255,18 +261,20 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 
 	GEM_BUG_ON(!wopcm->size);
 
-	err = wopcm_guc_init(wopcm);
+	err = wopcm_guc_region_init(wopcm);
 	if (err)
 		return err;
 
-	err = wopcm_check_hw_restrictions(wopcm);
-	if (err)
-		return err;
+	wopcm_adjust_for_hw_restrictions(wopcm);
 
 	err = wopcm_check_components_fit(wopcm);
 	if (err)
 		return err;
 
+	DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
+			 wopcm->guc.base / 1024,
+			 (wopcm->guc.base + wopcm->guc.size) / 1024);
+
 	return 0;
 }
 
-- 
2.14.3

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

Re: [PATCH 5/8] drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition

[Yaodong Li]

On 03/23/2018 05:34 AM, Michał Winiarski wrote:
> We need GuC to load HuC, but it's also possible for GuC to operate on
> its own. We don't know what the size of HuC FW may be, so, not wanting
> to make any platform-specific hardcoded guesses, we assume that its size
> is 0... Which is a very bad approximation.
> This has a very unfortunate consequence - once we've booted with GuC and
> no HuC, we'll never be able to load HuC (unless we reboot, since the
> registers are locked).
> Rather than using unknown values in our computations - let's partition
> based on GuC size.
we can do this based on known GuC and HuC sizes without
any assumption on FW sizes :)
please also refer to: https://patchwork.freedesktop.org/series/40541/
> We have one HW restriction where we're using HuC size (GuC size needs to
> be roughly similar to HuC size - which may be unknown at this point),
> luckily, another HW restriction makes it very unlikely to run in any
> sort of issues in this case.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jackie Li <yaodong.li@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++-----------------
>   1 file changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
> index 52841d340002..295d302e97b9 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)
>   	return 0;
>   }
>   
> -static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
> +static inline void
> +__guc_region_grow(struct intel_wopcm *wopcm, u32 size)
> +{
> +	/*
> +	 * We're growing guc region in the direction of lower addresses.
> +	 * We need to use multiples of base alignment, because it has more
> +	 * strict alignment rules.
> +	 */
> +	size = DIV_ROUND_UP(size, 2);
A bit confused here. Don't we just want to push the base downward for
*size* bytes?
> +	size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT);
Hmm, I think we only need align it to 4K boundary.
> +
> +	wopcm->guc.base -= size;
> +	wopcm->guc.size += size;
> +}
> +
> +static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
>   {
>   	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
>   	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
> @@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
>   		size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
>   							   wopcm->guc.size);
>   		if (size)
> -			goto err;
> +			__guc_region_grow(wopcm, size);
>   
>   		size = gen9_size_for_huc_restriction(wopcm->guc.size,
>   						     huc_fw_size);
Note that huc_fw_size might be zero in enable_guc=1 (GuC only) case.
Once the registers were locked, enable_guc=3 may still fail since 
huc_fw_size
may still large enough to break the restriction we want to enforce that:
hw_fw_size + 16KB > guc_fw_size.
>   		if (size)
> -			goto err;
> -	}
> -
> -	return 0;
> +			__guc_region_grow(wopcm, size);
>   
> -err:
> -	DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
> -		  wopcm->guc.size / 1024,
> -		  size / 1024);
> -
> -	return -E2BIG;
> +		GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base,
> +							       wopcm->guc.size));
> +		GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size,
> +							 huc_fw_size));
> +	}
>   }
>   
>   static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
> @@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
>   	return 0;
>   }
>   
> -static int wopcm_guc_init(struct intel_wopcm *wopcm)
> +static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
>   {
>   	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
> -	u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
> +	u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw);
>   	u32 ctx_rsvd = context_reserved_size(dev_priv);
>   
> -	wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),
> -				     GUC_WOPCM_OFFSET_ALIGNMENT);
> +	wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size);
>   
> -	wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,
> -				PAGE_SIZE);
> -
> -	DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
> -			 wopcm->guc.base / 1024,
> -			 (wopcm->guc.base + wopcm->guc.size) / 1024);
> +	wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size,
> +				     GUC_WOPCM_OFFSET_ALIGNMENT);
guc.size + ctx_rsvd > wopcm->size check?
>   
>   	return 0;
>   }
> @@ -255,18 +261,20 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>   
>   	GEM_BUG_ON(!wopcm->size);
>   
> -	err = wopcm_guc_init(wopcm);
> +	err = wopcm_guc_region_init(wopcm);
Again, wopcm will contain invalid data on failure. But maybe we can
go with it now :)

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

Re: [PATCH 5/8] drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition

[Michał Winiarski]

On Fri, Mar 23, 2018 at 01:00:35PM -0700, Yaodong Li wrote:
> On 03/23/2018 05:34 AM, Michał Winiarski wrote:
> > We need GuC to load HuC, but it's also possible for GuC to operate on
> > its own. We don't know what the size of HuC FW may be, so, not wanting
> > to make any platform-specific hardcoded guesses, we assume that its size
> > is 0... Which is a very bad approximation.
> > This has a very unfortunate consequence - once we've booted with GuC and
> > no HuC, we'll never be able to load HuC (unless we reboot, since the
> > registers are locked).
> > Rather than using unknown values in our computations - let's partition
> > based on GuC size.
> we can do this based on known GuC and HuC sizes without
> any assumption on FW sizes :)
> please also refer to: https://patchwork.freedesktop.org/series/40541/

You need to have HuC FW present in the filesystem do do that though.
(IIUC we still get 0 if it's not there)

> > We have one HW restriction where we're using HuC size (GuC size needs to
> > be roughly similar to HuC size - which may be unknown at this point),
> > luckily, another HW restriction makes it very unlikely to run in any
> > sort of issues in this case.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jackie Li <yaodong.li@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++-----------------
> >   1 file changed, 34 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
> > index 52841d340002..295d302e97b9 100644
> > --- a/drivers/gpu/drm/i915/intel_wopcm.c
> > +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> > @@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)
> >   	return 0;
> >   }
> > -static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
> > +static inline void
> > +__guc_region_grow(struct intel_wopcm *wopcm, u32 size)
> > +{
> > +	/*
> > +	 * We're growing guc region in the direction of lower addresses.
> > +	 * We need to use multiples of base alignment, because it has more
> > +	 * strict alignment rules.
> > +	 */
> > +	size = DIV_ROUND_UP(size, 2);
> A bit confused here. Don't we just want to push the base downward for
> *size* bytes?

Starting from the following:

    +--------------+           
    |--------------|           
    ||            ||           
    ||            ||           
    ||  GuC       ||           
    ||  region    ||           
    ||            ||           
    ||            ||           
    ||            ||           
    ||            ||           
    +--------------+           
    |              |           
    |              |           
    |              |           
    |              |           
    |              |           
    |              |           
    |              |           
    |              |           
    |              |           
    +--------------+           

We want to grow the whole region by size bytes. Pushing the base downward gives
us this:

    +--------------+        
    |  Empty       |        
    |  space       |        
    +--------------+        
    ||            ||        
    ||            ||        
    ||  GuC       ||        
    ||  region    ||        
    ||            ||        
    ||            ||        
    ||            ||        
    ||            ||        
    +--------------+        
    |              |        
    |              |        
    |              |        
    |              |        
    |              |        
    |              |        
    |              |        
    +--------------+        

Which leaves less space for HuC (and we're also leaving a bunch of unused space
in our partitioning).

If we modify both base and size to end up with this:

    +--------------+
    |--------------|
    ||            ||
    ||            ||
    ||  GuC       ||
    ||  region    ||
    ||            ||
    ||            ||
    ||            ||
    ||            ||
    ||            ||
    +--------------+
    |              |
    |              |
    |              |
    |              |
    |              |
    |              |
    |              |
    |              |
    +--------------+

We're still satisfying the HW restriciton, but we're not wasting any space from
the top (and also we're leaving more space for HuC).

> > +	size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT);
> Hmm, I think we only need align it to 4K boundary.

No - because we're modifying both base (16K alignment) and size (4K
alignment). 

> > +
> > +	wopcm->guc.base -= size;
> > +	wopcm->guc.size += size;
> > +}
> > +
> > +static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
> >   {
> >   	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> >   	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
> > @@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
> >   		size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
> >   							   wopcm->guc.size);
> >   		if (size)
> > -			goto err;
> > +			__guc_region_grow(wopcm, size);
> >   		size = gen9_size_for_huc_restriction(wopcm->guc.size,
> >   						     huc_fw_size);
> Note that huc_fw_size might be zero in enable_guc=1 (GuC only) case.
> Once the registers were locked, enable_guc=3 may still fail since
> huc_fw_size
> may still large enough to break the restriction we want to enforce that:
> hw_fw_size + 16KB > guc_fw_size.

Well - as mentioned in the commit message, since we also have a "dword gap"
restriction, we're left with pretty large GuC size, which makes this problem
dissaperar for real-world HuC FW sizes.

But yeah - I agree, we're not able to guarantee this.

> >   		if (size)
> > -			goto err;
> > -	}
> > -
> > -	return 0;
> > +			__guc_region_grow(wopcm, size);
> > -err:
> > -	DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
> > -		  wopcm->guc.size / 1024,
> > -		  size / 1024);
> > -
> > -	return -E2BIG;
> > +		GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base,
> > +							       wopcm->guc.size));
> > +		GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size,
> > +							 huc_fw_size));
> > +	}
> >   }
> >   static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
> > @@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
> >   	return 0;
> >   }
> > -static int wopcm_guc_init(struct intel_wopcm *wopcm)
> > +static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
> >   {
> >   	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
> > -	u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
> > +	u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw);
> >   	u32 ctx_rsvd = context_reserved_size(dev_priv);
> > -	wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),
> > -				     GUC_WOPCM_OFFSET_ALIGNMENT);
> > +	wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size);
> > -	wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,
> > -				PAGE_SIZE);
> > -
> > -	DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
> > -			 wopcm->guc.base / 1024,
> > -			 (wopcm->guc.base + wopcm->guc.size) / 1024);
> > +	wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size,
> > +				     GUC_WOPCM_OFFSET_ALIGNMENT);
> guc.size + ctx_rsvd > wopcm->size check?

It's being done in check_ctx_rsvd_fits later on (in wopcm_check_components_fit).

> >   	return 0;
> >   }
> > @@ -255,18 +261,20 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
> >   	GEM_BUG_ON(!wopcm->size);
> > -	err = wopcm_guc_init(wopcm);
> > +	err = wopcm_guc_region_init(wopcm);
> Again, wopcm will contain invalid data on failure. But maybe we can
> go with it now :)

I could go back to passing around a bunch of locals, but since we're only
accessing wopcm here I think passing around struct intel_wopcm makes the code a
bit cleaner. And we're failing the driver load on error anyways.

-Michał

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

Re: [PATCH 5/8] drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition

[Yaodong Li]

On 03/26/2018 03:04 AM, Michał Winiarski wrote:
> On Fri, Mar 23, 2018 at 01:00:35PM -0700, Yaodong Li wrote:
>> On 03/23/2018 05:34 AM, Michał Winiarski wrote:
>>> We need GuC to load HuC, but it's also possible for GuC to operate on
>>> its own. We don't know what the size of HuC FW may be, so, not wanting
>>> to make any platform-specific hardcoded guesses, we assume that its size
>>> is 0... Which is a very bad approximation.
>>> This has a very unfortunate consequence - once we've booted with GuC and
>>> no HuC, we'll never be able to load HuC (unless we reboot, since the
>>> registers are locked).
>>> Rather than using unknown values in our computations - let's partition
>>> based on GuC size.
>> we can do this based on known GuC and HuC sizes without
>> any assumption on FW sizes :)
>> please also refer to: https://patchwork.freedesktop.org/series/40541/
> You need to have HuC FW present in the filesystem do do that though.
> (IIUC we still get 0 if it's not there)
Yes. we cannot make any assumption to the status of the FW files as well.
so I think we should expect a system reboot for any FW updates.
>>> We have one HW restriction where we're using HuC size (GuC size needs to
>>> be roughly similar to HuC size - which may be unknown at this point),
>>> luckily, another HW restriction makes it very unlikely to run in any
>>> sort of issues in this case.
>>>
>>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Jackie Li <yaodong.li@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++-----------------
>>>    1 file changed, 34 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
>>> index 52841d340002..295d302e97b9 100644
>>> --- a/drivers/gpu/drm/i915/intel_wopcm.c
>>> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
>>> @@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)
>>>    	return 0;
>>>    }
>>> -static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
>>> +static inline void
>>> +__guc_region_grow(struct intel_wopcm *wopcm, u32 size)
>>> +{
>>> +	/*
>>> +	 * We're growing guc region in the direction of lower addresses.
>>> +	 * We need to use multiples of base alignment, because it has more
>>> +	 * strict alignment rules.
>>> +	 */
>>> +	size = DIV_ROUND_UP(size, 2);
>> A bit confused here. Don't we just want to push the base downward for
>> *size* bytes?
> Starting from the following:
>
>      +--------------+
>      |--------------|
>      ||            ||
>      ||            ||
>      ||  GuC       ||
>      ||  region    ||
>      ||            ||
>      ||            ||
>      ||            ||
>      ||            ||
>      +--------------+
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      +--------------+
>
> We want to grow the whole region by size bytes. Pushing the base downward gives
> us this:
>
>      +--------------+
>      |  Empty       |
>      |  space       |
>      +--------------+
>      ||            ||
>      ||            ||
>      ||  GuC       ||
>      ||  region    ||
>      ||            ||
>      ||            ||
>      ||            ||
>      ||            ||
>      +--------------+
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      +--------------+
>
> Which leaves less space for HuC (and we're also leaving a bunch of unused space
> in our partitioning).
>
> If we modify both base and size to end up with this:
>
>      +--------------+
>      |--------------|
>      ||            ||
>      ||            ||
>      ||  GuC       ||
>      ||  region    ||
>      ||            ||
>      ||            ||
>      ||            ||
>      ||            ||
>      ||            ||
>      +--------------+
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      |              |
>      +--------------+
>
> We're still satisfying the HW restriciton, but we're not wasting any space from
> the top (and also we're leaving more space for HuC).
>
>>> +	size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT);
>> Hmm, I think we only need align it to 4K boundary.
> No - because we're modifying both base (16K alignment) and size (4K
> alignment).
Got it:-) Thanks!
>
>>> +
>>> +	wopcm->guc.base -= size;
>>> +	wopcm->guc.size += size;
>>> +}
>>> +
>>> +static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
>>>    {
>>>    	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
>>>    	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
>>> @@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
>>>    		size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
>>>    							   wopcm->guc.size);
>>>    		if (size)
>>> -			goto err;
>>> +			__guc_region_grow(wopcm, size);
>>>    		size = gen9_size_for_huc_restriction(wopcm->guc.size,
>>>    						     huc_fw_size);
>> Note that huc_fw_size might be zero in enable_guc=1 (GuC only) case.
>> Once the registers were locked, enable_guc=3 may still fail since
>> huc_fw_size
>> may still large enough to break the restriction we want to enforce that:
>> hw_fw_size + 16KB > guc_fw_size.
> Well - as mentioned in the commit message, since we also have a "dword gap"
> restriction, we're left with pretty large GuC size, which makes this problem
> dissaperar for real-world HuC FW sizes.
>
> But yeah - I agree, we're not able to guarantee this.
Hmm, Still I think we shall not make any assumption on this which
driver has no control. It seems that partitioning based on real FW sizes 
+ expecting
system reboot for FW file changes should be a driver side solution to 
support
enable_guc=1 then enable_guc=3 case.
>
>>>    		if (size)
>>> -			goto err;
>>> -	}
>>> -
>>> -	return 0;
>>> +			__guc_region_grow(wopcm, size);
>>> -err:
>>> -	DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
>>> -		  wopcm->guc.size / 1024,
>>> -		  size / 1024);
>>> -
>>> -	return -E2BIG;
>>> +		GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base,
>>> +							       wopcm->guc.size));
>>> +		GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size,
>>> +							 huc_fw_size));
>>> +	}
>>>    }
>>>    static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
>>> @@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
>>>    	return 0;
>>>    }
>>> -static int wopcm_guc_init(struct intel_wopcm *wopcm)
>>> +static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
>>>    {
>>>    	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
>>> -	u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
>>> +	u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw);
>>>    	u32 ctx_rsvd = context_reserved_size(dev_priv);
>>> -	wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),
>>> -				     GUC_WOPCM_OFFSET_ALIGNMENT);
>>> +	wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size);
>>> -	wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,
>>> -				PAGE_SIZE);
>>> -
>>> -	DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
>>> -			 wopcm->guc.base / 1024,
>>> -			 (wopcm->guc.base + wopcm->guc.size) / 1024);
>>> +	wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size,
>>> +				     GUC_WOPCM_OFFSET_ALIGNMENT);
>> guc.size + ctx_rsvd > wopcm->size check?
> It's being done in check_ctx_rsvd_fits later on (in wopcm_check_components_fit).
>
Hmm, Just worried about wopcm->size - ctx_rsvd - wopcm->guc.size
may be incorrect (wrap around) since we don't have any checks on
the value of  ctx_rsvd  + wopcm->guc.size (it might be larger than wopcm 
size since
the checks we've done before could only guarantee wopcm->guc.size < 
wopcm.size).

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

[PATCH 6/8] drm/i915/guc: Don't give up on WOPCM partitioning regs mismatch

[Michał Winiarski]

It's possible that when we're being loaded, the write-once registers were
already programmed. It's also possible, that values present in those registers
match our FW size and HW restrictions.
Rather than failing the module load when we detect any differences from our
preferred values, let's replace them with what's present in the registers,
failing the load only if we're unable to satisfy the constraints.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jackie Li <yaodong.li@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c    |   6 +--
 drivers/gpu/drm/i915/intel_wopcm.c | 107 +++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_wopcm.h |   6 ++-
 3 files changed, 60 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 802df8e1a544..1b25eadc5940 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5154,11 +5154,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 		goto out;
 	}
 
-	ret = intel_wopcm_init_hw(&dev_priv->wopcm);
-	if (ret) {
-		DRM_ERROR("Enabling WOPCM failed (%d)\n", ret);
-		goto out;
-	}
+	intel_wopcm_init_hw(&dev_priv->wopcm);
 
 	/* We can't enable contexts until all firmware is loaded */
 	ret = intel_uc_init_hw(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 295d302e97b9..be8fca80aeca 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -182,7 +182,7 @@ __guc_region_grow(struct intel_wopcm *wopcm, u32 size)
 	wopcm->guc.size += size;
 }
 
-static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
+static int wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
 	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
@@ -191,19 +191,29 @@ static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
 	if (IS_GEN9(i915) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)) {
 		size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
 							   wopcm->guc.size);
-		if (size)
+		if (size) {
+			if (wopcm->guc.base_locked || wopcm->guc.size_locked)
+				return -E2BIG;
+
 			__guc_region_grow(wopcm, size);
+		}
 
 		size = gen9_size_for_huc_restriction(wopcm->guc.size,
 						     huc_fw_size);
-		if (size)
+		if (size) {
+			if (wopcm->guc.base_locked || wopcm->guc.size_locked)
+				return -E2BIG;
+
 			__guc_region_grow(wopcm, size);
+		}
 
 		GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base,
 							       wopcm->guc.size));
 		GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size,
 							 huc_fw_size));
 	}
+
+	return 0;
 }
 
 static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
@@ -233,12 +243,30 @@ static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
 	u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw);
+	u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
 	u32 ctx_rsvd = context_reserved_size(dev_priv);
+	u32 reg_val;
 
-	wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size);
+	reg_val = I915_READ(GUC_WOPCM_SIZE);
+	if (reg_val & GUC_WOPCM_SIZE_LOCKED) {
+		wopcm->guc.size = (reg_val & GUC_WOPCM_SIZE_MASK);
+		wopcm->guc.size_locked = true;
+	} else {
+		wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size);
+	}
 
-	wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size,
-				     GUC_WOPCM_OFFSET_ALIGNMENT);
+	reg_val = I915_READ(DMA_GUC_WOPCM_OFFSET);
+	if (reg_val & GUC_WOPCM_OFFSET_VALID) {
+		if (huc_fw_size && !(reg_val & HUC_LOADING_AGENT_GUC))
+			return -EIO;
+
+		wopcm->guc.base = (reg_val & GUC_WOPCM_OFFSET_MASK);
+		wopcm->guc.base_locked = true;
+	} else {
+		wopcm->guc.base = ALIGN_DOWN(wopcm->size - wopcm->guc.size -
+					     ctx_rsvd,
+					     GUC_WOPCM_OFFSET_ALIGNMENT);
+	}
 
 	return 0;
 }
@@ -265,7 +293,9 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 	if (err)
 		return err;
 
-	wopcm_adjust_for_hw_restrictions(wopcm);
+	err = wopcm_adjust_for_hw_restrictions(wopcm);
+	if (err)
+		return err;
 
 	err = wopcm_check_components_fit(wopcm);
 	if (err)
@@ -278,66 +308,37 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
 	return 0;
 }
 
-static inline int write_and_verify(struct drm_i915_private *dev_priv,
-				   i915_reg_t reg, u32 val, u32 mask,
-				   u32 locked_bit)
-{
-	u32 reg_val;
-
-	GEM_BUG_ON(val & ~mask);
-
-	I915_WRITE(reg, val);
-
-	reg_val = I915_READ(reg);
-
-	return (reg_val & mask) != (val | locked_bit) ? -EIO : 0;
-}
-
 /**
  * intel_wopcm_init_hw() - Setup GuC WOPCM registers.
  * @wopcm: pointer to intel_wopcm.
  *
- * Setup the GuC WOPCM size and offset registers with the calculated values. It
- * will verify the register values to make sure the registers are locked with
- * correct values.
+ * Setup the GuC WOPCM size and offset registers with the calculated values.
  *
- * Return: 0 on success. -EIO if registers were locked with incorrect values.
  */
-int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
+void intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
-	u32 huc_agent;
-	u32 mask;
-	int err;
 
 	if (!USES_GUC(dev_priv))
-		return 0;
+		return;
 
 	GEM_BUG_ON(!HAS_GUC(dev_priv));
 	GEM_BUG_ON(!wopcm->guc.size);
 	GEM_BUG_ON(!wopcm->guc.base);
 
-	err = write_and_verify(dev_priv, GUC_WOPCM_SIZE, wopcm->guc.size,
-			       GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
-			       GUC_WOPCM_SIZE_LOCKED);
-	if (err)
-		goto err_out;
-
-	huc_agent = USES_HUC(dev_priv) ? HUC_LOADING_AGENT_GUC : 0;
-	mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
-	err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
-			       wopcm->guc.base | huc_agent, mask,
-			       GUC_WOPCM_OFFSET_VALID);
-	if (err)
-		goto err_out;
-
-	return 0;
-
-err_out:
-	DRM_ERROR("Failed to init WOPCM registers:\n");
-	DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
-		  I915_READ(DMA_GUC_WOPCM_OFFSET));
-	DRM_ERROR("GUC_WOPCM_SIZE=%#x\n", I915_READ(GUC_WOPCM_SIZE));
+	if (!wopcm->guc.size_locked) {
+		I915_WRITE(GUC_WOPCM_SIZE, wopcm->guc.size &
+					   GUC_WOPCM_SIZE_MASK);
+		wopcm->guc.size_locked = true;
+		GEM_BUG_ON(!I915_READ(GUC_WOPCM_SIZE) & GUC_WOPCM_SIZE_LOCKED);
+	}
 
-	return err;
+	if (!wopcm->guc.base_locked) {
+		I915_WRITE(DMA_GUC_WOPCM_OFFSET,
+			   (GUC_WOPCM_OFFSET_MASK & wopcm->guc.base) |
+			   HUC_LOADING_AGENT_GUC);
+		wopcm->guc.size_locked = true;
+		GEM_BUG_ON(!I915_READ(DMA_GUC_WOPCM_OFFSET) &
+			   GUC_WOPCM_OFFSET_VALID);
+	}
 }
diff --git a/drivers/gpu/drm/i915/intel_wopcm.h b/drivers/gpu/drm/i915/intel_wopcm.h
index 6298910a384c..144232999bfe 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.h
+++ b/drivers/gpu/drm/i915/intel_wopcm.h
@@ -14,18 +14,22 @@
  * @size: Size of overall WOPCM.
  * @guc: GuC WOPCM Region info.
  * @guc.base: GuC WOPCM base which is offset from WOPCM base.
+ * @guc.base_locked: True if base register was locked during partitioning.
  * @guc.size: Size of the GuC WOPCM region.
+ * @guc.size_locked: True if size register was locked during partitioning.
  */
 struct intel_wopcm {
 	u32 size;
 	struct {
 		u32 base;
+		bool base_locked;
 		u32 size;
+		bool size_locked;
 	} guc;
 };
 
 void intel_wopcm_init_early(struct intel_wopcm *wopcm);
 int intel_wopcm_init(struct intel_wopcm *wopcm);
-int intel_wopcm_init_hw(struct intel_wopcm *wopcm);
+void intel_wopcm_init_hw(struct intel_wopcm *wopcm);
 
 #endif
-- 
2.14.3

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

[PATCH 7/8] drm/i915/guc: Don't touch WOPCM if we're not using GuC

[Michał Winiarski]

We probably shouldn't print out WOPCM size on platforms that don't have
GuC. We also want to make sure we don't hit any asserts if user explicitly
sets enable_guc != 0 on non-guc platforms.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jackie Li <yaodong.li@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_wopcm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index be8fca80aeca..828800ca119c 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -69,6 +69,9 @@
  */
 void intel_wopcm_init_early(struct intel_wopcm *wopcm)
 {
+	if (!HAS_GUC(wopcm_to_i915(wopcm)))
+		return;
+
 	wopcm->size = GEN9_WOPCM_SIZE;
 
 	DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024);
@@ -285,8 +288,12 @@ static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
  */
 int intel_wopcm_init(struct intel_wopcm *wopcm)
 {
+	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
 	int err;
 
+	if (!HAS_GUC(dev_priv) || !USES_GUC(dev_priv))
+		return 0;
+
 	GEM_BUG_ON(!wopcm->size);
 
 	err = wopcm_guc_region_init(wopcm);
@@ -319,10 +326,9 @@ void intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
 
-	if (!USES_GUC(dev_priv))
+	if (!HAS_GUC(dev_priv) || !USES_GUC(dev_priv))
 		return;
 
-	GEM_BUG_ON(!HAS_GUC(dev_priv));
 	GEM_BUG_ON(!wopcm->guc.size);
 	GEM_BUG_ON(!wopcm->guc.base);
 
-- 
2.14.3

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

Re: [PATCH 7/8] drm/i915/guc: Don't touch WOPCM if we're not using GuC

[Yaodong Li]

On 03/23/2018 05:34 AM, Michał Winiarski wrote:
> We probably shouldn't print out WOPCM size on platforms that don't have
> GuC. We also want to make sure we don't hit any asserts if user explicitly
> sets enable_guc != 0 on non-guc platforms.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jackie Li <yaodong.li@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_wopcm.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
> index be8fca80aeca..828800ca119c 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -69,6 +69,9 @@
>    */
>   void intel_wopcm_init_early(struct intel_wopcm *wopcm)
>   {
> +	if (!HAS_GUC(wopcm_to_i915(wopcm)))
> +		return;
> +
>   	wopcm->size = GEN9_WOPCM_SIZE;
>   
>   	DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024);
> @@ -285,8 +288,12 @@ static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
>    */
>   int intel_wopcm_init(struct intel_wopcm *wopcm)
>   {
> +	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
>   	int err;
>   
> +	if (!HAS_GUC(dev_priv) || !USES_GUC(dev_priv))
> +		return 0;
> +
I guess I have to bring up an old question here: if we want to
use this only for enable_guc > 0. Why not make it as a part of
uc layer?

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

[PATCH 8/8] HAX enable guc for CI

[Michał Winiarski]

---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c96360398072..53037b5eff22 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -47,7 +47,7 @@ struct drm_printer;
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
+	param(int, enable_guc, -1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
-- 
2.14.3

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

[Patchwork]

== Series Details ==

Series: series starting with [1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler
URL   : https://patchwork.freedesktop.org/series/40568/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
e70f1d562ad5 drm/i915/guc: Use _FW variants for mmio access in GuC irq handler
6abf5f1d30ce drm/i915/guc: Move FW size sanity check back to fetch
f5c0dbe6fa46 drm/i915/guc: Extend the GEN9 WOPCM HW restrictions to early CNL
-:16: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#16: 
References: 6b0478fb722a ("drm/i915: Implement dynamic GuC WOPCM offset and size calculation")

-:16: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 6b0478fb722a ("drm/i915: Implement dynamic GuC WOPCM offset and size calculation")'
#16: 
References: 6b0478fb722a ("drm/i915: Implement dynamic GuC WOPCM offset and size calculation")

total: 1 errors, 1 warnings, 0 checks, 95 lines checked
2d7726fb5a16 drm/i915/guc: Separate WOPCM partitioning from constraints check
96b4234f0965 drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition
9d8c2037aae4 drm/i915/guc: Don't give up on WOPCM partitioning regs mismatch
-:11: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#11: 
already programmed. It's also possible, that values present in those registers

total: 0 errors, 1 warnings, 0 checks, 200 lines checked
a4aa3bdd063f drm/i915/guc: Don't touch WOPCM if we're not using GuC
9842fe063a6c HAX enable guc for CI
-:19: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)

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

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

✗ Fi.CI.SPARSE: warning for series starting with [1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

[Patchwork]

== Series Details ==

Series: series starting with [1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler
URL   : https://patchwork.freedesktop.org/series/40568/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/guc: Use _FW variants for mmio access in GuC irq handler
Okay!

Commit: drm/i915/guc: Move FW size sanity check back to fetch
Okay!

Commit: drm/i915/guc: Extend the GEN9 WOPCM HW restrictions to early CNL
Okay!

Commit: drm/i915/guc: Separate WOPCM partitioning from constraints check
Okay!

Commit: drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition
Okay!

Commit: drm/i915/guc: Don't give up on WOPCM partitioning regs mismatch
+drivers/gpu/drm/i915/intel_wopcm.c:333:17: warning: dubious: !x & y
+drivers/gpu/drm/i915/intel_wopcm.c:341:17: warning: dubious: !x & y

Commit: drm/i915/guc: Don't touch WOPCM if we're not using GuC
Okay!

Commit: HAX enable guc for CI
Okay!

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

✗ Fi.CI.BAT: failure for series starting with [1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

[Patchwork]

== Series Details ==

Series: series starting with [1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler
URL   : https://patchwork.freedesktop.org/series/40568/
State : failure

== Summary ==

Series 40568v1 series starting with [1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler
https://patchwork.freedesktop.org/api/1.0/series/40568/revisions/1/mbox/

---- Possible new issues:

Test drv_module_reload:
        Subgroup basic-no-display:
                pass       -> DMESG-FAIL (fi-bxt-j4205)
                pass       -> DMESG-FAIL (fi-cfl-8700k)
                pass       -> DMESG-FAIL (fi-cfl-u)
                pass       -> DMESG-FAIL (fi-kbl-7500u)
                pass       -> DMESG-FAIL (fi-kbl-7567u)
                pass       -> DMESG-FAIL (fi-skl-6260u)
                pass       -> DMESG-FAIL (fi-skl-6600u)
                pass       -> DMESG-FAIL (fi-skl-6700k2)
                pass       -> DMESG-FAIL (fi-skl-6770hq)
                pass       -> DMESG-FAIL (fi-skl-guc)
                pass       -> DMESG-FAIL (fi-skl-gvtdvm)
        Subgroup basic-reload:
                pass       -> DMESG-FAIL (fi-bxt-j4205)
                pass       -> DMESG-FAIL (fi-cfl-8700k)
                pass       -> DMESG-FAIL (fi-cfl-u)
                pass       -> DMESG-FAIL (fi-kbl-7500u)
                pass       -> DMESG-FAIL (fi-skl-6260u)
                pass       -> DMESG-FAIL (fi-skl-6600u)
                pass       -> DMESG-FAIL (fi-skl-6700k2)
                pass       -> DMESG-FAIL (fi-skl-guc)
                pass       -> DMESG-FAIL (fi-skl-gvtdvm)
        Subgroup basic-reload-inject:
                pass       -> DMESG-FAIL (fi-bxt-j4205)
                pass       -> DMESG-FAIL (fi-cfl-8700k)
                pass       -> DMESG-FAIL (fi-cfl-u)
                pass       -> DMESG-FAIL (fi-kbl-7500u)
                pass       -> DMESG-FAIL (fi-kbl-7567u)
                pass       -> DMESG-FAIL (fi-skl-6260u)
                pass       -> DMESG-FAIL (fi-skl-6600u)
                pass       -> DMESG-FAIL (fi-skl-6700k2)
                pass       -> DMESG-FAIL (fi-skl-6770hq)
                pass       -> DMESG-FAIL (fi-skl-guc)
                pass       -> DMESG-FAIL (fi-skl-gvtdvm)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> SKIP       (fi-bxt-dsi)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-cfl-8700k)
                pass       -> SKIP       (fi-cfl-u)
                pass       -> SKIP       (fi-kbl-7500u)
        Subgroup basic-s4-devices:
                pass       -> SKIP       (fi-bxt-dsi)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-cfl-8700k)
                pass       -> SKIP       (fi-cfl-u)
                pass       -> SKIP       (fi-kbl-7567u)
Test gem_linear_blits:
        Subgroup basic:
                pass       -> SKIP       (fi-bxt-dsi)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-cfl-8700k)
                pass       -> SKIP       (fi-cfl-u)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-kbl-7567u)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6600u)
                pass       -> SKIP       (fi-skl-6700k2)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-guc)
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_render_linear_blits:
        Subgroup basic:
                pass       -> SKIP       (fi-bxt-dsi)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-cfl-8700k)
                pass       -> SKIP       (fi-cfl-u)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-kbl-7567u)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6600u)
                pass       -> SKIP       (fi-skl-6700k2)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-guc)
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_render_tiled_blits:
        Subgroup basic:
                pass       -> SKIP       (fi-bxt-dsi)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-cfl-8700k)
                pass       -> SKIP       (fi-cfl-u)
                pass       -> SKIP       (fi-kbl-7500u)
                pass       -> SKIP       (fi-kbl-7567u)
                pass       -> SKIP       (fi-skl-6260u)
                pass       -> SKIP       (fi-skl-6600u)
                pass       -> SKIP       (fi-skl-6700k2)
                pass       -> SKIP       (fi-skl-6770hq)
                pass       -> SKIP       (fi-skl-guc)
                pass       -> SKIP       (fi-skl-gvtdvm)
Test gem_ringfill:
        Subgroup basic-default:
                pass       -> SKIP       (fi-bxt-dsi)
                pass       -> SKIP       (fi-bxt-j4205)
                pass       -> SKIP       (fi-cfl-8700k)
WARNING: Long output truncated

157295bdb3516bc0972daffcef016d668f549d79 drm-tip: 2018y-03m-23d-15h-22m-25s UTC integration manifest
9842fe063a6c HAX enable guc for CI
a4aa3bdd063f drm/i915/guc: Don't touch WOPCM if we're not using GuC
9d8c2037aae4 drm/i915/guc: Don't give up on WOPCM partitioning regs mismatch
96b4234f0965 drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition
2d7726fb5a16 drm/i915/guc: Separate WOPCM partitioning from constraints check
f5c0dbe6fa46 drm/i915/guc: Extend the GEN9 WOPCM HW restrictions to early CNL
6abf5f1d30ce drm/i915/guc: Move FW size sanity check back to fetch
e70f1d562ad5 drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

== Logs ==

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

Re: [PATCH 1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

[Daniele Ceraolo Spurio]

On 23/03/18 05:34, Michał Winiarski wrote:
> We're seeing "RPM wakelock ref not held during HW access" warning
> otherwise. And since IRQ are synced for runtime suspend, we can use the
> variant without wakeref assert.
> 
> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 8f93f5bef8fd..6787a3116783 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -391,9 +391,9 @@ void intel_guc_to_host_event_handler(struct intel_guc *guc)
>   	 * clears out the bit on handling the 1st interrupt.
>   	 */
>   	spin_lock(&guc->irq_lock);
> -	val = I915_READ(SOFT_SCRATCH(15));
> +	val = I915_READ_FW(SOFT_SCRATCH(15));

GuC registers are in forcewake range, so don't we need to manually grab 
forcewake if we use the _FW variant of the read/write macros?

Daniele

>   	msg = val & guc->msg_enabled_mask;
> -	I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
> +	I915_WRITE_FW(SOFT_SCRATCH(15), val & ~msg);
>   	spin_unlock(&guc->irq_lock);
>   
>   	if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

[Joonas Lahtinen]

Quoting Daniele Ceraolo Spurio (2018-03-23 19:17:49)
> 
> 
> On 23/03/18 05:34, Michał Winiarski wrote:
> > We're seeing "RPM wakelock ref not held during HW access" warning
> > otherwise. And since IRQ are synced for runtime suspend, we can use the
> > variant without wakeref assert.
> > 
> > Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_guc.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> > index 8f93f5bef8fd..6787a3116783 100644
> > --- a/drivers/gpu/drm/i915/intel_guc.c
> > +++ b/drivers/gpu/drm/i915/intel_guc.c
> > @@ -391,9 +391,9 @@ void intel_guc_to_host_event_handler(struct intel_guc *guc)
> >        * clears out the bit on handling the 1st interrupt.
> >        */
> >       spin_lock(&guc->irq_lock);
> > -     val = I915_READ(SOFT_SCRATCH(15));
> > +     val = I915_READ_FW(SOFT_SCRATCH(15));
> 
> GuC registers are in forcewake range, so don't we need to manually grab 
> forcewake if we use the _FW variant of the read/write macros?

Hmm, with the Bugzilla tag and all, wasn't this patch tested
specifically to fix the bug?

Regards, Joonas

PS. If there's a bugfix, it should really be a separate patch that can
be immediately merged and the bug should get fixed by the patch with
Bugzilla: is merged.

> 
> Daniele
> 
> >       msg = val & guc->msg_enabled_mask;
> > -     I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
> > +     I915_WRITE_FW(SOFT_SCRATCH(15), val & ~msg);
> >       spin_unlock(&guc->irq_lock);
> >   
> >       if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> > 
> _______________________________________________
> 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 1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

[Michał Winiarski]

On Wed, Mar 28, 2018 at 04:51:55PM +0300, Joonas Lahtinen wrote:
> Quoting Daniele Ceraolo Spurio (2018-03-23 19:17:49)
> > 
> > 
> > On 23/03/18 05:34, Michał Winiarski wrote:
> > > We're seeing "RPM wakelock ref not held during HW access" warning
> > > otherwise. And since IRQ are synced for runtime suspend, we can use the
> > > variant without wakeref assert.
> > > 
> > > Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710
> > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_guc.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> > > index 8f93f5bef8fd..6787a3116783 100644
> > > --- a/drivers/gpu/drm/i915/intel_guc.c
> > > +++ b/drivers/gpu/drm/i915/intel_guc.c
> > > @@ -391,9 +391,9 @@ void intel_guc_to_host_event_handler(struct intel_guc *guc)
> > >        * clears out the bit on handling the 1st interrupt.
> > >        */
> > >       spin_lock(&guc->irq_lock);
> > > -     val = I915_READ(SOFT_SCRATCH(15));
> > > +     val = I915_READ_FW(SOFT_SCRATCH(15));
> > 
> > GuC registers are in forcewake range, so don't we need to manually grab 
> > forcewake if we use the _FW variant of the read/write macros?
> 
> Hmm, with the Bugzilla tag and all, wasn't this patch tested
> specifically to fix the bug?
> 
> Regards, Joonas
> 
> PS. If there's a bugfix, it should really be a separate patch that can
> be immediately merged and the bug should get fixed by the patch with
> Bugzilla: is merged.

Daniele is correct - we do need the forcewake. The WARN is caused by the fact
that we're not doing pm_get (and we don't need to do it).

The correct fix would be to do the disable/enable_rpm_wakeref_asserts dance,
however Chris suggested that since the forcewake takes time, maybe we could move
the logic to tasklet. And... why not?

I'll follow your advice and post it as a separate patch.

-Michał

> 
> > 
> > Daniele
> > 
> > >       msg = val & guc->msg_enabled_mask;
> > > -     I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
> > > +     I915_WRITE_FW(SOFT_SCRATCH(15), val & ~msg);
> > >       spin_unlock(&guc->irq_lock);
> > >   
> > >       if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> > > 
> > _______________________________________________
> > 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 1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

[Joonas Lahtinen]

Quoting Michał Winiarski (2018-03-23 14:34:04)
> We're seeing "RPM wakelock ref not held during HW access" warning
> otherwise. And since IRQ are synced for runtime suspend, we can use the
> variant without wakeref assert.
> 
> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>

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

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