On Wed, 17 Oct 2018 02:17:17 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio@xxxxxxxxx> wrote:
The only content of the register apart from the valid bit is the lower
part of the physical memory address. If the valid bit is 0 the address
is meaningless, while if it is 1 we don't know which descriptor it came
from (since the doorbell is in an unexpected state) so we can't match it
to an expected value. Since we already print the state of the valid bit,
stop priniting the full register contents as they're just confusing.
typo in priniting
While at it, move the checking of the valid bit to a common helper.
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_guc_submission.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c
b/drivers/gpu/drm/i915/intel_guc_submission.c
index 8c3b5a9facee..cba84480dad9 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -192,6 +192,12 @@ 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);
should we add GEM_BUG_ON(db_id > GUC_NUM_DOORBELLS) here ?
+ 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 +209,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 +219,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 +871,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);
btw, maybe better to check against GUC_NUM_DOORBELLS ?
GUC_DOORBELL_INVALID looks like a fw defined value
and is it ok that we define GUC_NUM_DOORBELLS in intel_guc_fwif.h ?
maybe better place for it is near GEN8_DRBREGL in intel_guc_reg.h
- 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 %d has unexpected state: valid=%s\n",
+ db_id, yesno(valid));
db_id is unsigned so you can use %u (or %hu) for it
return false;
}
later in guc_verify_doorbells() we are stopping after detecting first
mismatched doorbell - maybe we should scan all doorbells to get debug
logs for all unexpected states?
with all these nitpicks hopefully accepted,
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Regards,
Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
