diff mbox series

[v2,4/6] drm/i915/guc: doorbell checking cleanup

Message ID 20181018004610.22895-4-daniele.ceraolospurio@intel.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/6] drm/i915/guc: drop negative doorbell alloc selftest | expand

Commit Message

Daniele Ceraolo Spurio Oct. 18, 2018, 12:46 a.m. UTC
A collection of very small cleanups/improvements around doorbell checking
that do not deserve their own patch:

- Move doorbell-related HW defs to intel_guc_reg.h

- use GUC_NUM_DOORBELLS instead of GUC_DOORBELL_INVALID where
  appropriate

- do not stop on error in guc_verify_doorbells

- do not print drbreg on error: the only content of the register
  apart from the valid bit is the lower part of the physical memory
  address, which we can't use even if valid because we don't know
  which descriptor it came from (since the doorbell is in an unexpected
  state)

- Move the checking of doorbell valid bit to a common helper.

v2: add more cleanups (move defs, use GUC_NUM_DOORBELLS, don't stop in
    guc_verify_doorbells) (Michal)

Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_fwif.h       |  6 ++---
 drivers/gpu/drm/i915/intel_guc_reg.h        |  4 +++
 drivers/gpu/drm/i915/intel_guc_submission.c | 27 ++++++++++++---------
 3 files changed, 22 insertions(+), 15 deletions(-)

Comments

Michal Wajdeczko Oct. 18, 2018, 12:50 p.m. UTC | #1
On Thu, 18 Oct 2018 02:46:08 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> A collection of very small cleanups/improvements around doorbell checking
> that do not deserve their own patch:
>
> - Move doorbell-related HW defs to intel_guc_reg.h
>
> - use GUC_NUM_DOORBELLS instead of GUC_DOORBELL_INVALID where
>   appropriate
>
> - do not stop on error in guc_verify_doorbells
>
> - do not print drbreg on error: the only content of the register
>   apart from the valid bit is the lower part of the physical memory
>   address, which we can't use even if valid because we don't know
>   which descriptor it came from (since the doorbell is in an unexpected
>   state)
>
> - Move the checking of doorbell valid bit to a common helper.
>
> v2: add more cleanups (move defs, use GUC_NUM_DOORBELLS, don't stop in
>     guc_verify_doorbells) (Michal)
>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_fwif.h       |  6 ++---
>  drivers/gpu/drm/i915/intel_guc_reg.h        |  4 +++
>  drivers/gpu/drm/i915/intel_guc_submission.c | 27 ++++++++++++---------
>  3 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h  
> b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index d1bbaba6e012..427c621f3e72 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -23,6 +23,8 @@
>  #ifndef _INTEL_GUC_FWIF_H
>  #define _INTEL_GUC_FWIF_H
> +#include "intel_guc_reg.h"

hmm, I would prefer to keep fwif definitions independent of
our other headers - see below

> +
>  #define GUC_CLIENT_PRIORITY_KMD_HIGH	0
>  #define GUC_CLIENT_PRIORITY_HIGH	1
>  #define GUC_CLIENT_PRIORITY_KMD_NORMAL	2
> @@ -59,9 +61,6 @@
>  #define WQ_RING_TAIL_MAX		0x7FF	/* 2^11 QWords */
>  #define WQ_RING_TAIL_MASK		(WQ_RING_TAIL_MAX << WQ_RING_TAIL_SHIFT)
> -#define GUC_DOORBELL_ENABLED		1
> -#define GUC_DOORBELL_DISABLED		0
> -
>  #define GUC_STAGE_DESC_ATTR_ACTIVE	BIT(0)
>  #define GUC_STAGE_DESC_ATTR_PENDING_DB	BIT(1)
>  #define GUC_STAGE_DESC_ATTR_KERNEL	BIT(2)
> @@ -233,7 +232,6 @@ union guc_doorbell_qw {
>  	u64 value_qw;
>  } __packed;
> -#define GUC_NUM_DOORBELLS	256
>  #define GUC_DOORBELL_INVALID	(GUC_NUM_DOORBELLS)

we don't have to use GUC_NUM_DOORBELLS here as it is pure
fw decision how to identify invalid index - and this could
change in future fw versions ie. to ~0, so for now:

#define GUC_DOORBELL_INVALID	256

> #define GUC_DB_SIZE			(PAGE_SIZE)
> diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h  
> b/drivers/gpu/drm/i915/intel_guc_reg.h
> index d86084742a4a..f438ee81dd1e 100644
> --- a/drivers/gpu/drm/i915/intel_guc_reg.h
> +++ b/drivers/gpu/drm/i915/intel_guc_reg.h
> @@ -104,6 +104,10 @@
>  #define GUC_SEND_INTERRUPT		_MMIO(0xc4c8)
>  #define   GUC_SEND_TRIGGER		  (1<<0)
> +#define GUC_NUM_DOORBELLS		256
> +#define GUC_DOORBELL_ENABLED		1
> +#define GUC_DOORBELL_DISABLED		0

ENABLED/DISABLED are related to guc_doorbell_info.db_status
so maybe we want to move that struct definition here too ?

> +
>  #define GEN8_DRBREGL(x)			_MMIO(0x1000 + (x) * 8)
>  #define   GEN8_DRB_VALID		  (1<<0)
>  #define GEN8_DRBREGU(x)			_MMIO(0x1000 + (x) * 8 + 4)
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c  
> b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 8c3b5a9facee..b11d5eefcc88 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -192,6 +192,14 @@ static struct guc_doorbell_info  
> *__get_doorbell(struct intel_guc_client *client)
>  	return client->vaddr + client->doorbell_offset;
>  }
> +static bool __doorbell_valid(struct intel_guc *guc, u16 db_id)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> +	GEM_BUG_ON(db_id >= GUC_NUM_DOORBELLS);
> +	return I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID;
> +}
> +
>  static void __init_doorbell(struct intel_guc_client *client)
>  {
>  	struct guc_doorbell_info *doorbell;
> @@ -203,7 +211,6 @@ static void __init_doorbell(struct intel_guc_client  
> *client)
> static void __fini_doorbell(struct intel_guc_client *client)
>  {
> -	struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
>  	struct guc_doorbell_info *doorbell;
>  	u16 db_id = client->doorbell_id;
> @@ -214,7 +221,7 @@ static void __fini_doorbell(struct intel_guc_client  
> *client)
>  	 * to go to zero after updating db_status before we call the GuC to
>  	 * release the doorbell
>  	 */
> -	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID),  
> 10))
> +	if (wait_for_us(!__doorbell_valid(client->guc, db_id), 10))
>  		WARN_ONCE(true, "Doorbell never became invalid after disable\n");
>  }
> @@ -866,20 +873,17 @@ guc_reset_prepare(struct intel_engine_cs *engine)
>  /* Check that a doorbell register is in the expected state */
>  static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
>  {
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	u32 drbregl;
>  	bool valid;
> -	GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);
> +	GEM_BUG_ON(db_id >= GUC_NUM_DOORBELLS);
> -	drbregl = I915_READ(GEN8_DRBREGL(db_id));
> -	valid = drbregl & GEN8_DRB_VALID;
> +	valid = __doorbell_valid(guc, db_id);
> 	if (test_bit(db_id, guc->doorbell_bitmap) == valid)
>  		return true;
> -	DRM_DEBUG_DRIVER("Doorbell %d has unexpected state (0x%x): valid=%s\n",
> -			 db_id, drbregl, yesno(valid));
> +	DRM_DEBUG_DRIVER("Doorbell %u has unexpected state: valid=%s\n",
> +			 db_id, yesno(valid));
> 	return false;
>  }
> @@ -887,12 +891,13 @@ static bool doorbell_ok(struct intel_guc *guc, u16  
> db_id)
>  static bool guc_verify_doorbells(struct intel_guc *guc)
>  {
>  	u16 db_id;
> +	bool doorbells_ok = true;
> 	for (db_id = 0; db_id < GUC_NUM_DOORBELLS; ++db_id)
>  		if (!doorbell_ok(guc, db_id))
> -			return false;
> +			doorbells_ok = false;
> -	return true;
> +	return doorbells_ok;
>  }
> /**

with redefined GUC_DOORBELL_INVALID and guc_doorbell_info moved,

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

Thanks,
Michal
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index d1bbaba6e012..427c621f3e72 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -23,6 +23,8 @@ 
 #ifndef _INTEL_GUC_FWIF_H
 #define _INTEL_GUC_FWIF_H
 
+#include "intel_guc_reg.h"
+
 #define GUC_CLIENT_PRIORITY_KMD_HIGH	0
 #define GUC_CLIENT_PRIORITY_HIGH	1
 #define GUC_CLIENT_PRIORITY_KMD_NORMAL	2
@@ -59,9 +61,6 @@ 
 #define WQ_RING_TAIL_MAX		0x7FF	/* 2^11 QWords */
 #define WQ_RING_TAIL_MASK		(WQ_RING_TAIL_MAX << WQ_RING_TAIL_SHIFT)
 
-#define GUC_DOORBELL_ENABLED		1
-#define GUC_DOORBELL_DISABLED		0
-
 #define GUC_STAGE_DESC_ATTR_ACTIVE	BIT(0)
 #define GUC_STAGE_DESC_ATTR_PENDING_DB	BIT(1)
 #define GUC_STAGE_DESC_ATTR_KERNEL	BIT(2)
@@ -233,7 +232,6 @@  union guc_doorbell_qw {
 	u64 value_qw;
 } __packed;
 
-#define GUC_NUM_DOORBELLS	256
 #define GUC_DOORBELL_INVALID	(GUC_NUM_DOORBELLS)
 
 #define GUC_DB_SIZE			(PAGE_SIZE)
diff --git a/drivers/gpu/drm/i915/intel_guc_reg.h b/drivers/gpu/drm/i915/intel_guc_reg.h
index d86084742a4a..f438ee81dd1e 100644
--- a/drivers/gpu/drm/i915/intel_guc_reg.h
+++ b/drivers/gpu/drm/i915/intel_guc_reg.h
@@ -104,6 +104,10 @@ 
 #define GUC_SEND_INTERRUPT		_MMIO(0xc4c8)
 #define   GUC_SEND_TRIGGER		  (1<<0)
 
+#define GUC_NUM_DOORBELLS		256
+#define GUC_DOORBELL_ENABLED		1
+#define GUC_DOORBELL_DISABLED		0
+
 #define GEN8_DRBREGL(x)			_MMIO(0x1000 + (x) * 8)
 #define   GEN8_DRB_VALID		  (1<<0)
 #define GEN8_DRBREGU(x)			_MMIO(0x1000 + (x) * 8 + 4)
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 8c3b5a9facee..b11d5eefcc88 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -192,6 +192,14 @@  static struct guc_doorbell_info *__get_doorbell(struct intel_guc_client *client)
 	return client->vaddr + client->doorbell_offset;
 }
 
+static bool __doorbell_valid(struct intel_guc *guc, u16 db_id)
+{
+	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+	GEM_BUG_ON(db_id >= GUC_NUM_DOORBELLS);
+	return I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID;
+}
+
 static void __init_doorbell(struct intel_guc_client *client)
 {
 	struct guc_doorbell_info *doorbell;
@@ -203,7 +211,6 @@  static void __init_doorbell(struct intel_guc_client *client)
 
 static void __fini_doorbell(struct intel_guc_client *client)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
 	struct guc_doorbell_info *doorbell;
 	u16 db_id = client->doorbell_id;
 
@@ -214,7 +221,7 @@  static void __fini_doorbell(struct intel_guc_client *client)
 	 * to go to zero after updating db_status before we call the GuC to
 	 * release the doorbell
 	 */
-	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID), 10))
+	if (wait_for_us(!__doorbell_valid(client->guc, db_id), 10))
 		WARN_ONCE(true, "Doorbell never became invalid after disable\n");
 }
 
@@ -866,20 +873,17 @@  guc_reset_prepare(struct intel_engine_cs *engine)
 /* Check that a doorbell register is in the expected state */
 static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
-	u32 drbregl;
 	bool valid;
 
-	GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);
+	GEM_BUG_ON(db_id >= GUC_NUM_DOORBELLS);
 
-	drbregl = I915_READ(GEN8_DRBREGL(db_id));
-	valid = drbregl & GEN8_DRB_VALID;
+	valid = __doorbell_valid(guc, db_id);
 
 	if (test_bit(db_id, guc->doorbell_bitmap) == valid)
 		return true;
 
-	DRM_DEBUG_DRIVER("Doorbell %d has unexpected state (0x%x): valid=%s\n",
-			 db_id, drbregl, yesno(valid));
+	DRM_DEBUG_DRIVER("Doorbell %u has unexpected state: valid=%s\n",
+			 db_id, yesno(valid));
 
 	return false;
 }
@@ -887,12 +891,13 @@  static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
 static bool guc_verify_doorbells(struct intel_guc *guc)
 {
 	u16 db_id;
+	bool doorbells_ok = true;
 
 	for (db_id = 0; db_id < GUC_NUM_DOORBELLS; ++db_id)
 		if (!doorbell_ok(guc, db_id))
-			return false;
+			doorbells_ok = false;
 
-	return true;
+	return doorbells_ok;
 }
 
 /**