Re: [RFC/CI] drm/i915: Sanitize GuC client initialization

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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.lahtinen@linux.intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@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