diff mbox series

[v2,11/22] drm/i915/guc: Reset GuC ADS during sanitize

Message ID 20190411084436.24384-12-michal.wajdeczko@intel.com (mailing list archive)
State New, archived
Headers show
Series GuC 32.0.3 | expand

Commit Message

Michal Wajdeczko April 11, 2019, 8:44 a.m. UTC 
GuC stores some data in there, which might be stale after a reset.
Reinitialize whole ADS in case any part of it was corrupted during
previous GuC run.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: MichaĹ Winiarski <michal.winiarski@intel.com>
Cc: Tomasz Lis <tomasz.lis@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.h     |  2 +
 drivers/gpu/drm/i915/intel_guc_ads.c | 85 ++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_guc_ads.h |  1 +
 3 files changed, 57 insertions(+), 31 deletions(-)

Comments

Lis, Tomasz April 16, 2019, 11:44 a.m. UTC | #1 
Only comment issues. Besides these:

Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>

On 2019-04-11 10:44, Michal Wajdeczko wrote:
> GuC stores some data in there, which might be stale after a reset.
> Reinitialize whole ADS in case any part of it was corrupted during
> previous GuC run.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: MichaĹ Winiarski <michal.winiarski@intel.com>
> Cc: Tomasz Lis <tomasz.lis@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc.h     |  2 +
>   drivers/gpu/drm/i915/intel_guc_ads.c | 85 ++++++++++++++++++----------
>   drivers/gpu/drm/i915/intel_guc_ads.h |  1 +
>   3 files changed, 57 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 2c59ff8d9f39..4f3cf8eddfe6 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -26,6 +26,7 @@
>   #define _INTEL_GUC_H_
>   
>   #include "intel_uncore.h"
> +#include "intel_guc_ads.h"
>   #include "intel_guc_fw.h"
>   #include "intel_guc_fwif.h"
>   #include "intel_guc_ct.h"
> @@ -177,6 +178,7 @@ u32 intel_guc_reserved_gtt_size(struct intel_guc *guc);
>   static inline int intel_guc_sanitize(struct intel_guc *guc)
>   {
>   	intel_uc_fw_sanitize(&guc->fw);
> +	intel_guc_ads_reset(guc);
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c b/drivers/gpu/drm/i915/intel_guc_ads.c
> index abab5cb6909a..97926effb944 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
> @@ -72,43 +72,28 @@ static void guc_ct_pool_entries_init(struct guc_ct_pool_entry *pool, u32 num)
>    */
>   #define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
>   
> -/**
> - * intel_guc_ads_create() - creates GuC ADS
> - * @guc: intel_guc struct
> - *
> - */
> -int intel_guc_ads_create(struct intel_guc *guc)
> +/* The ads obj includes the struct itself and buffers passed to GuC */
> +struct __guc_ads_blob {
> +	struct guc_ads ads;
> +	struct guc_policies policies;
> +	struct guc_mmio_reg_state reg_state;
> +	struct guc_gt_system_info system_info;
> +	struct guc_clients_info clients_info;
> +	struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
> +	u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
> +} __packed;
> +
> +static int __guc_ads_reinit(struct intel_guc *guc)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	struct i915_vma *vma;
> -	/* The ads obj includes the struct itself and buffers passed to GuC */
> -	struct {
> -		struct guc_ads ads;
> -		struct guc_policies policies;
> -		struct guc_mmio_reg_state reg_state;
> -		struct guc_gt_system_info system_info;
> -		struct guc_clients_info clients_info;
> -		struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
> -		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
> -	} __packed *blob;
> +	struct __guc_ads_blob *blob;
>   	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
>   	u32 base;
>   	u8 engine_class;
> -	int ret;
> -
> -	GEM_BUG_ON(guc->ads_vma);
> -
> -	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
> -	if (IS_ERR(vma))
> -		return PTR_ERR(vma);
> -
> -	guc->ads_vma = vma;
>   
>   	blob = i915_gem_object_pin_map(guc->ads_vma->obj, I915_MAP_WB);
> -	if (IS_ERR(blob)) {
> -		ret = PTR_ERR(blob);
> -		goto err_vma;
> -	}
> +	if (IS_ERR(blob))
> +		return PTR_ERR(blob);
>   
>   	/* GuC scheduling policies */
>   	guc_policies_init(&blob->policies);
> @@ -142,7 +127,7 @@ int intel_guc_ads_create(struct intel_guc *guc)
>   	blob->system_info.vebox_enable_mask = VEBOX_MASK(dev_priv);
>   	blob->system_info.vdbox_sfc_support_mask = RUNTIME_INFO(dev_priv)->vdbox_sfc_access;
>   
> -	base = intel_guc_ggtt_offset(guc, vma);
> +	base = intel_guc_ggtt_offset(guc, guc->ads_vma);
>   
>   	/* Clients info  */
>   	guc_ct_pool_entries_init(blob->ct_pool, ARRAY_SIZE(blob->ct_pool));
> @@ -161,6 +146,32 @@ int intel_guc_ads_create(struct intel_guc *guc)
>   	i915_gem_object_unpin_map(guc->ads_vma->obj);
>   
>   	return 0;
> +}
> +
> +/**
> + * intel_guc_ads_create() - creates GuC ADS
> + * @guc: intel_guc struct
Are we really ok with documentation which just reads names with spaces 
instead of underscores?
I believe the description should go deeper, or at least use different 
words to describe the thing.
ie. here:

intel_guc_ads_create() - allocates and initializes GuC Additional Data Struct

@guc: instance of intel_guc which will own the ADS
> + *
> + */
> +int intel_guc_ads_create(struct intel_guc *guc)
> +{
> +	const u32 size = PAGE_ALIGN(sizeof(struct __guc_ads_blob));
> +	struct i915_vma *vma;
> +	int ret;
> +
> +	GEM_BUG_ON(guc->ads_vma);
> +
> +	vma = intel_guc_allocate_vma(guc, size);
> +	if (IS_ERR(vma))
> +		return PTR_ERR(vma);
> +
> +	guc->ads_vma = vma;
> +
> +	ret = __guc_ads_reinit(guc);
> +	if (ret)
> +		goto err_vma;
> +
> +	return 0;
>   
>   err_vma:
>   	i915_vma_unpin_and_release(&guc->ads_vma, 0);
> @@ -171,3 +182,15 @@ void intel_guc_ads_destroy(struct intel_guc *guc)
>   {
>   	i915_vma_unpin_and_release(&guc->ads_vma, 0);
>   }
> +
> +/**
> + * intel_guc_ads_reset() - resets GuC ADS
Again:

intel_guc_ads_reset() - prepares GuC Additional Data Struct for reuse

-Tomasz

> + * @guc: intel_guc struct
> + *
> + */
> +void intel_guc_ads_reset(struct intel_guc *guc)
> +{
> +	if (!guc->ads_vma)
> +		return;
> +	__guc_ads_reinit(guc);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.h b/drivers/gpu/drm/i915/intel_guc_ads.h
> index c4735742c564..7f40f9cd5fb9 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ads.h
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.h
> @@ -29,5 +29,6 @@ struct intel_guc;
>   
>   int intel_guc_ads_create(struct intel_guc *guc);
>   void intel_guc_ads_destroy(struct intel_guc *guc);
> +void intel_guc_ads_reset(struct intel_guc *guc);
>   
>   #endif
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 2c59ff8d9f39..4f3cf8eddfe6 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -26,6 +26,7 @@ 
 #define _INTEL_GUC_H_
 
 #include "intel_uncore.h"
+#include "intel_guc_ads.h"
 #include "intel_guc_fw.h"
 #include "intel_guc_fwif.h"
 #include "intel_guc_ct.h"
@@ -177,6 +178,7 @@  u32 intel_guc_reserved_gtt_size(struct intel_guc *guc);
 static inline int intel_guc_sanitize(struct intel_guc *guc)
 {
 	intel_uc_fw_sanitize(&guc->fw);
+	intel_guc_ads_reset(guc);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c b/drivers/gpu/drm/i915/intel_guc_ads.c
index abab5cb6909a..97926effb944 100644
--- a/drivers/gpu/drm/i915/intel_guc_ads.c
+++ b/drivers/gpu/drm/i915/intel_guc_ads.c
@@ -72,43 +72,28 @@  static void guc_ct_pool_entries_init(struct guc_ct_pool_entry *pool, u32 num)
  */
 #define LR_HW_CONTEXT_SIZE	(80 * sizeof(u32))
 
-/**
- * intel_guc_ads_create() - creates GuC ADS
- * @guc: intel_guc struct
- *
- */
-int intel_guc_ads_create(struct intel_guc *guc)
+/* The ads obj includes the struct itself and buffers passed to GuC */
+struct __guc_ads_blob {
+	struct guc_ads ads;
+	struct guc_policies policies;
+	struct guc_mmio_reg_state reg_state;
+	struct guc_gt_system_info system_info;
+	struct guc_clients_info clients_info;
+	struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
+	u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
+} __packed;
+
+static int __guc_ads_reinit(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	struct i915_vma *vma;
-	/* The ads obj includes the struct itself and buffers passed to GuC */
-	struct {
-		struct guc_ads ads;
-		struct guc_policies policies;
-		struct guc_mmio_reg_state reg_state;
-		struct guc_gt_system_info system_info;
-		struct guc_clients_info clients_info;
-		struct guc_ct_pool_entry ct_pool[GUC_CT_POOL_SIZE];
-		u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
-	} __packed *blob;
+	struct __guc_ads_blob *blob;
 	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
 	u32 base;
 	u8 engine_class;
-	int ret;
-
-	GEM_BUG_ON(guc->ads_vma);
-
-	vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
-	if (IS_ERR(vma))
-		return PTR_ERR(vma);
-
-	guc->ads_vma = vma;
 
 	blob = i915_gem_object_pin_map(guc->ads_vma->obj, I915_MAP_WB);
-	if (IS_ERR(blob)) {
-		ret = PTR_ERR(blob);
-		goto err_vma;
-	}
+	if (IS_ERR(blob))
+		return PTR_ERR(blob);
 
 	/* GuC scheduling policies */
 	guc_policies_init(&blob->policies);
@@ -142,7 +127,7 @@  int intel_guc_ads_create(struct intel_guc *guc)
 	blob->system_info.vebox_enable_mask = VEBOX_MASK(dev_priv);
 	blob->system_info.vdbox_sfc_support_mask = RUNTIME_INFO(dev_priv)->vdbox_sfc_access;
 
-	base = intel_guc_ggtt_offset(guc, vma);
+	base = intel_guc_ggtt_offset(guc, guc->ads_vma);
 
 	/* Clients info  */
 	guc_ct_pool_entries_init(blob->ct_pool, ARRAY_SIZE(blob->ct_pool));
@@ -161,6 +146,32 @@  int intel_guc_ads_create(struct intel_guc *guc)
 	i915_gem_object_unpin_map(guc->ads_vma->obj);
 
 	return 0;
+}
+
+/**
+ * intel_guc_ads_create() - creates GuC ADS
+ * @guc: intel_guc struct
+ *
+ */
+int intel_guc_ads_create(struct intel_guc *guc)
+{
+	const u32 size = PAGE_ALIGN(sizeof(struct __guc_ads_blob));
+	struct i915_vma *vma;
+	int ret;
+
+	GEM_BUG_ON(guc->ads_vma);
+
+	vma = intel_guc_allocate_vma(guc, size);
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
+
+	guc->ads_vma = vma;
+
+	ret = __guc_ads_reinit(guc);
+	if (ret)
+		goto err_vma;
+
+	return 0;
 
 err_vma:
 	i915_vma_unpin_and_release(&guc->ads_vma, 0);
@@ -171,3 +182,15 @@  void intel_guc_ads_destroy(struct intel_guc *guc)
 {
 	i915_vma_unpin_and_release(&guc->ads_vma, 0);
 }
+
+/**
+ * intel_guc_ads_reset() - resets GuC ADS
+ * @guc: intel_guc struct
+ *
+ */
+void intel_guc_ads_reset(struct intel_guc *guc)
+{
+	if (!guc->ads_vma)
+		return;
+	__guc_ads_reinit(guc);
+}
diff --git a/drivers/gpu/drm/i915/intel_guc_ads.h b/drivers/gpu/drm/i915/intel_guc_ads.h
index c4735742c564..7f40f9cd5fb9 100644
--- a/drivers/gpu/drm/i915/intel_guc_ads.h
+++ b/drivers/gpu/drm/i915/intel_guc_ads.h
@@ -29,5 +29,6 @@  struct intel_guc;
 
 int intel_guc_ads_create(struct intel_guc *guc);
 void intel_guc_ads_destroy(struct intel_guc *guc);
+void intel_guc_ads_reset(struct intel_guc *guc);
 
 #endif