| Message ID | 1490086977-9282-9-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/21/2017 11:20 AM, Ceraolo Spurio, Daniele wrote: > > > On 3/21/2017 2:02 AM, Oscar Mateo wrote: >> Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go >> to zero after updating db_status before we call the GuC to release the >> doorbell. >> >> Kudos to Daniele for finding this out. >> >> v2: WARN instead of DRM_ERROR (Joonas) >> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > I'd like Joonas to give his ack on not returning after the wait since > he commented on it in the previous version. I personally prefer it > this way because we gain a bit more time for the doorbell to disable > itself (there is no spec in regards to how long this should take, but > it should be quick) while the GuC processes the H2G. If we ever see a > case where the warn fires but the H2G is successful it would mean that > we have to tune our wait time. > > Thanks, > Daniele True. I went with his second option (WARN instead of DRM_ERROR) without asking him first... Thanks, -- Oscar
On 3/21/2017 2:02 AM, Oscar Mateo wrote: > Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go > to zero after updating db_status before we call the GuC to release the > doorbell. > > Kudos to Daniele for finding this out. > > v2: WARN instead of DRM_ERROR (Joonas) > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> I'd like Joonas to give his ack on not returning after the wait since he commented on it in the previous version. I personally prefer it this way because we gain a bit more time for the doorbell to disable itself (there is no spec in regards to how long this should take, but it should be quick) while the GuC processes the H2G. If we ever see a case where the warn fires but the H2G is successful it would mean that we have to tune our wait time. Thanks, Daniele > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 855112d..143ef26 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -206,12 +206,22 @@ static int __create_doorbell(struct i915_guc_client *client) > > static int __destroy_doorbell(struct i915_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; > + > + GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID); > > doorbell = __get_doorbell(client); > doorbell->db_status = GUC_DOORBELL_DISABLED; > doorbell->cookie = 0; > > + /* Doorbell release flow requires that we wait for GEN8_DRB_VALID bit > + * 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)) > + WARN_ONCE(true, "Doorbell never became invalid after disable\n"); > + > return __guc_deallocate_doorbell(client->guc, client->ctx_index); > } > >
On ti, 2017-03-21 at 04:24 -0700, Oscar Mateo wrote: > > True. I went with his second option (WARN instead of DRM_ERROR) without > asking him first... Without knowing more of the innards of GuC, it's hard to say which will be more appropriate action. WARN should be enough for now. Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 855112d..143ef26 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -206,12 +206,22 @@ static int __create_doorbell(struct i915_guc_client *client) static int __destroy_doorbell(struct i915_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; + + GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID); doorbell = __get_doorbell(client); doorbell->db_status = GUC_DOORBELL_DISABLED; doorbell->cookie = 0; + /* Doorbell release flow requires that we wait for GEN8_DRB_VALID bit + * 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)) + WARN_ONCE(true, "Doorbell never became invalid after disable\n"); + return __guc_deallocate_doorbell(client->guc, client->ctx_index); }
Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go to zero after updating db_status before we call the GuC to release the doorbell. Kudos to Daniele for finding this out. v2: WARN instead of DRM_ERROR (Joonas) Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 10 ++++++++++ 1 file changed, 10 insertions(+)