| Message ID | 20181017001718.11511-3-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/guc: rename __create/destroy_doorbell | expand |
On Wed, 17 Oct 2018 02:17:17 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> 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@intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > 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@intel.com> Regards, Michal
On 17/10/18 09:37, Michal Wajdeczko wrote: > On Wed, 17 Oct 2018 02:17:17 +0200, Daniele Ceraolo Spurio > <daniele.ceraolospurio@intel.com> 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@intel.com> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> --- >> 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 ? > Can do. I didn't to begin with because we should catch invalid IDs before we reach here, but an extra debug check won't hurt >> + 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 > It is, but even in FW it is defined as equal to GUC_NUM_DOORBELLS > 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 > The problem with that is that we'd have to either move GUC_DOORBELL_INVALID as well or include intel_guc_reg.h from intel_guc_fwif.h. The latter should be semantically ok I think, since some aspects of the interface are dependent on HW >> - 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 > ack >> 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? ack Thanks, Daniele > > with all these nitpicks hopefully accepted, > > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Regards, > Michal
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); + 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); - 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)); return false; }
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. While at it, move the checking of the valid bit to a common helper. Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/intel_guc_submission.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)