On pe, 2017-02-10 at 16:11 +0100, Michal Wajdeczko wrote:
> On Fri, Feb 10, 2017 at 03:30:10PM +0200, Joonas Lahtinen wrote:
> >
> > Started adding proper teardown to guc_client_alloc, ended up removing
> > quite a few dead ends where errors communicating with the GuC were
> > silently ignored. There also seemed to be quite a few erronous
> > teardown actions performed in case of an error (ordering wrong).
> >
> > Signed-off-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
> > Cc: Arkadiusz Hiler <arkadiusz.hi...@intel.com>
> > Cc: Oscar Mateo <oscar.ma...@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
<SNIP>
> explicit inline ? or you want to let gcc decide ?
GCC should do it, these are not hot path functions.
> > -static int guc_allocate_doorbell(struct intel_guc *guc,
> > - struct i915_guc_client *client)
> > +static int __create_doorbell_hw(struct i915_guc_client *client)
>
> I would rather prefer to only change signature of this function into
>
> static int guc_allocate_doorbell(struct intel_guc *guc, u32 index)
>
> as a clean wrap around GUC_ACTION_ALLOCATE_DOORBELL. This way we also preserve
> consistency between function name and the guc action name used inside.
>
> Based on the above we can still add
>
> static int __create_doorbell_hw(struct i915_guc_client *client)
> {
> > return guc_allocate_doorbell(client->guc, client->ctx_index);
> }
>
> Note that location of the ctx_index member may change in the future, and this
> approach will minimize impact of these future changes.
That's viable, I made it;
__guc_allocate_doorbell(guc, client->ctx_index)
So it can bemoved out of the submission code in future.
> > +static bool has_doorbell(struct i915_guc_client *client)
> > +{
> > + if (client->doorbell_id == GUC_DOORBELL_INVALID)
> > + return false;
> > +
> > + return __test_doorbell(client);
> > +}
>
> Can we keep related inline helpers together ?
Moved.
> > -static void guc_disable_doorbell(struct intel_guc *guc,
> > - struct i915_guc_client *client)
> > +/*
> > +static int create_doorbell(struct i915_guc_client *client)
> > {
> > - (void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
> > + int err;
> >
> > - /* XXX: wait for any interrupts */
> > - /* XXX: wait for workqueue to drain */
> > + GEM_BUG_ON(has_doorbell(client));
> > +
> > + err = __reserve_doorbell(client);
> > + if (err)
> > + return err;
> > +
> > + return __create_doorbell(client);
> > }
> > +*/
>
> Wrong commit ?
Nuked.
> > -static void
> > -guc_client_free(struct drm_i915_private *dev_priv,
> > - struct i915_guc_client *client)
> > +static void guc_client_free(struct i915_guc_client *client)
> > {
> > - struct intel_guc *guc = &dev_priv->guc;
>
> We use guc few times, so maybe we can leave it as
>
> guc = client->guc;
Wanted to make it explicit that there are behind the scenes contracts
(allocated doorbells) between the client and guc.
> > + WARN_ON(destroy_doorbell(client));
> > + guc_ctx_desc_fini(client->guc, client);
> > + i915_gem_object_unpin_map(client->vma->obj);
> > i915_vma_unpin_and_release(&client->vma);
> > -
> > - if (client->ctx_index != GUC_INVALID_CTX_ID) {
> > - guc_ctx_desc_fini(guc, client);
> > - ida_simple_remove(&guc->ctx_ids, client->ctx_index);
> > - }
> > -
> > + ida_simple_remove(&client->guc->ctx_ids, client->ctx_index);
>
> What about adding small helper function and use it here instead of
> directly touching guc internal member:
>
> guc_release_client_index(guc, client->ctx_index);
I'll add a follow-up task for that.
> > /* Check that a doorbell register is in the expected state */
> > -static bool guc_doorbell_check(struct intel_guc *guc, uint16_t db_id)
> > +static bool doorbell_ok(struct intel_guc *guc, u8 db_id)
> > {
> > struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > - i915_reg_t drbreg = GEN8_DRBREGL(db_id);
> > - uint32_t value = I915_READ(drbreg);
> > - bool enabled = (value & GUC_DOORBELL_ENABLED) != 0;
> > - bool expected = test_bit(db_id, guc->doorbell_bitmap);
> >
> > - if (enabled == expected)
> > + u32 drbregl = I915_READ(GEN8_DRBREGL(db_id));
> > +
> > + bool valid = drbregl & GEN8_DRB_VALID;
> > +
> > + if (test_bit(db_id, guc->doorbell_bitmap) == valid)
> > return true;
> >
> > - DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) 0x%x, should be %s\n",
> > - db_id, drbreg.reg, value,
> > - expected ? "active" : "inactive");
> > + DRM_DEBUG_DRIVER("Doorbell %d has unexpected state (0x%x): valid=%s\n",
> > + db_id, drbregl, yesno(valid));
> >
> > return false;
> > }
> >
> > +static int __reset_doorbell(struct i915_guc_client* client, u16 db_id)
>
> Hmm, in previous function db_id was declared as u8.
Yeah, would be kinda mean to check the status of GUC_DOORBELL_INVALID.
But as it just happens to the current situation, I changed the
doorbell_ok signature and added GEM_BUG_ON check for future proofing.
> > @@ -978,7 +1038,7 @@ void i915_guc_submission_fini(struct drm_i915_private
> > *dev_priv)
> > if (!client)
> > return;
>
> Shouldn't we fix it now in this patch as well?
>
I'd wait for Daniele's patches to for persitent desc mapping
(allocation is currently phased rather strangely), and didn't want to
duplicate work.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
