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

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

On pe, 2017-02-10 at 11:55 -0800, Daniele Ceraolo Spurio wrote:
> 
> On 10/02/17 05:30, Joonas Lahtinen wrote:

<SNIP>

> > +static bool __test_doorbell(struct i915_guc_client *client)
> > +{
> > +	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> > +}
> 
> bikeshed: this helper is only used in one place inside a very small 
> function, so I'd prefer to drop it.

Good catch, the need disappeared during refactoring.

> 
> > 
> > +
> > +static void __release_doorbell(struct i915_guc_client *client)
> 
> bikeshed: in other places we use "unreserve" instead of "release" for 
> the symmetrical function of "reserve". That would be clearer here as 
> well IMHO.

That's true, fixed.

> 
> > 
> > +{
> > +	GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
> > +
> > +	__clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> 
> I would set client->doorbell_id = GUC_DOORBELL_INVALID here.

Yeah, it's better for symmetry, fixed that.

> > +static int __reserve_doorbell(struct i915_guc_client *client)
> > +{
> > +	unsigned long offset;
> > +	unsigned long end;
> > +	u16 id;
> > +
> > +	GEM_BUG_ON(client->doorbell_id != GUC_DOORBELL_INVALID);
> > +
> > +	/*
> > +	 * The bitmap tracks which doorbell registers are currently in use.
> > +	 * It is split into two halves; the first half is used for normal
> > +	 * priority contexts, the second half for high-priority ones.
> > +	 * Note that logically higher priorities are numerically less than
> > +	 * normal ones, so the test below means "is it high-priority?"
> > +	 */
> > +
> > +	offset = 0;
> > +	if (is_high_priority(client))
> > +		offset = GUC_NUM_DOORBELLS/2;
> > +
> > +	end = GUC_NUM_DOORBELLS - offset;
> 
> Should this be
> 
> 	end = offset + GUC_NUM_DOORBELLS/2;
> 
> ?
> 
> Otherwise if offset=0 you'll have end=GUC_NUM_DOORBELLS

Whoops, fixed :)

> > @@ -95,104 +144,114 @@ static int guc_release_doorbell(struct intel_guc *guc,
> >   * client object which contains the page being used for the doorbell
> >   */
> > 
> > -static int guc_update_doorbell_id(struct intel_guc *guc,
> > -				  struct i915_guc_client *client,
> > -				  u16 new_id)
> > +static int __update_doorbell(struct i915_guc_client *client, u16 new_id)
> 
> I'd prefer this to be called __update_doorbell_id or 
> __update_doorbell_desc, to make it clear that it is just changing the id 
> in the descriptor and not doing any re-allocation of the doorbell.

__update_doorbell_desc it is (I'd like to split doorbell vs. client to
their own structs, but lets leave that for later).

This only gets called during

> > -static void guc_init_doorbell_hw(struct intel_guc *guc)
> > +static int guc_init_doorbell_hw(struct intel_guc *guc)
> >  {
> >  	struct i915_guc_client *client = guc->execbuf_client;
> > -	uint16_t db_id;
> > -	int i, err;
> > +	int err;
> > +	int i;
> > 
> > -	guc_disable_doorbell(guc, client);
> > +	if (has_doorbell(client))
> > +		destroy_doorbell(client);
> > 
> > -	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
> > -		/* Skip if doorbell is OK */
> > -		if (guc_doorbell_check(guc, i))
> > +	for (i = 0; i < GUC_NUM_DOORBELLS; ++i) {
> > +		if (doorbell_ok(guc, i))
> >  			continue;
> > 
> > -		err = guc_update_doorbell_id(guc, client, i);
> > -		if (err)
> > -			DRM_DEBUG_DRIVER("Doorbell %d update failed, err %d\n",
> > -					i, err);
> > +		err = __reset_doorbell(client, i);
> 
> Not sure about the logic here. If the doorbell is active when it 
> shouldn't have been, __reset_doorbell will try to acquire the doorbell 
> before releasing it, which will fail if the doorbell is active (thus 
> also skipping the release_doorbell step). If the doorbell is not active 
> when it should have been, __reset_doorbell will still leave it 
> deactivated. If we care about both cases maybe we should specialize the 
> handling for the 2 possible scenarios, while if we care only about the 
> first one we should release the doorbell first (not sure if we still 
> want to do a setup & tear-down cycle afterwards).
> 
> The original logic looks also incorrect because guc_update_doorbell_id 
> will always enable the doorbell at the end.

I was assuming we want to deactivate all doorbells from the hardware if
they are active (because they would be stale entries). If there are no
other users than the KMD, then that'd be the appropriate action, and
rename doorbell_ok check into doorbell_active, and just nuke everything
active once loading?

> > +		WARN(err, "Doorbell %d reset failed, err %d\n", i, err);
> >  	}
> > 
> > -	db_id = select_doorbell_register(guc, client->priority);
> > -	WARN_ON(db_id == GUC_INVALID_DOORBELL_ID);
> > -
> > -	err = guc_update_doorbell_id(guc, client, db_id);
> > +	err = __reserve_doorbell(client);
> 
> 
> Shouldn't this be create_doorbell() instead of reserve?

That would end up calling the __update_doorbell, which we can't as the
desc is for some strange reason not mapped yet. I think you sent a
patch to move intel_guc_allocate_vma into the init, so let me give it a
look.

Which brings us to the point that I forgot to call __update_doorbell
completely!

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