[Intel-gfx] [PATCH v2 4/6] drm/i915/guc: doorbell checking cleanup
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Oct 18 12:50:31 UTC 2018
On Thu, 18 Oct 2018 02:46:08 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio at 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 at intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at 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 at intel.com>
Thanks,
Michal
More information about the Intel-gfx
mailing list