[Intel-gfx] [PATCH 2/3] drm/i915/guc: Introduce buffer based cmd transport

[Chris Wilson]

On Fri, May 12, 2017 at 08:53:15PM +0200, Michal Wajdeczko wrote:
> On Fri, May 12, 2017 at 04:52:04PM +0100, Chris Wilson wrote:
> > On Fri, May 12, 2017 at 03:02:59PM +0000, Michal Wajdeczko wrote:
> > > +	if (!ctch->vma) {
> > > +		/* allocate vma */
> > > +		vma = intel_guc_allocate_vma(guc, sizeof(*blob));
> > > +		if (IS_ERR(vma))
> > > +			return PTR_ERR(vma);
> > > +		ctch->vma = vma;
> > > +
> > > +		/* get unique owner id */
> > > +		err = ida_simple_get(&guc->ct.owner_ida,
> > > +				     0, INTEL_GUC_CT_MAX_CHANNELS,
> > > +				     GFP_KERNEL);
> > 
> > What's the point of an ida if MAX is 1!
> 
> We may have MAX > 1 in the future.

MAX should reflect the firmware limits. Because at the moment it looks
like this is imposing a restriction far above and beyond the existing
limits. If it just required a unique identifier and you plan to have one
per client, why we do more than one unique identifier?

> > > +		base = guc_ggtt_offset(ctch->vma);
> > > +		DRM_DEBUG_DRIVER("CT: vma base=%#x\n", base);
> > > +
> > > +		/* initialize descriptors */
> > > +		for (i = 0; i < ARRAY_SIZE(*blob); i++) {
> > > +			struct guc_ct_buffer_desc *desc = &(*blob)[i].desc;
> > > +
> > > +			guc_ct_buffer_desc_init(desc,
> > > +					base + arr_offset(*blob, i, cmds),
> > > +					sizeof((*blob)[0]) -
> > > +					ptr_offset(&(*blob)[0], cmds),
> > 
> > So chosing a non-power-of-two ringbuf was entirely your own fault, as
> > we tell the guc the size. Similarly the position of the desc.
> > 
> > I would have picked
> > 
> > struct guc_ct_buffer_desc *desc[] __cacheline_aligned;
> > 
> > desc = kmap(vma);
> > for (i = 0; i < 2; i+)
> > 	guc_ct_buffer_desc_init(&desc[i],
> > 				(char *)desc + 1024 * (i + 1),
> > 				1024);
> > (You can then replace arr_offset with just desc[CT_RECV] and
> > desc[CT_SEND].)
> > 
> > Oh, and move the allocation branch to a new function and always do the
> > reset.
> 
> Moving allocation branch to a new function can be tricky unless we decide
> to share blob definition or implictly assume that cmds are just PAGE/4.
> Will try to rework this.
> 
> And btw, plese note that desc_init() is already doing full reset.

Exactly with a bit of rework, reduce the code duplication. And you don't
need to make assumptions as you already record the offsets, or you can
use simplified layout above.

> > 
> > > +					ctch->owner);
> > > +
> > > +			ctch->ctbs[i].desc = desc;
> > > +			ctch->ctbs[i].cmds = (*blob)[i].cmds;
> > > +		}
> > > +
> > > +	} else {
> > > +		/* vma is already allocated and kmap'ed */
> > > +		base = guc_ggtt_offset(ctch->vma);
> > > +
> > > +		/* reset descriptors */
> > > +		for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) {
> > > +			guc_ct_buffer_desc_reset(ctch->ctbs[i].desc);
> > > +		}
> > > +	}
> > > +
> > > +
> > > +	/* register buffers, recv first */
> > > +	err = guc_action_register_ct_buffer(guc,
> > > +					    base +
> > > +					    arr_offset(*blob, 1, desc),
> > > +					    INTEL_GUC_CT_BUFFER_TYPE_RECV);
> > > +	if (unlikely(err))
> > > +		goto err_kunmap;
> > > +
> > > +	err = guc_action_register_ct_buffer(guc,
> > > +					    base +
> > > +					    arr_offset(*blob, 0, desc),
> > > +					    INTEL_GUC_CT_BUFFER_TYPE_SEND);
> > > +	if (unlikely(err))
> > > +		goto err_deregister;
> > > +
> > > +	return 0;
> > > +
> > > +err_deregister:
> > > +	guc_action_deregister_ct_buffer(guc,
> > > +					ctch->owner,
> > > +					INTEL_GUC_CT_BUFFER_TYPE_RECV);
> > > +err_kunmap:
> > > +	kunmap(i915_vma_first_page(ctch->vma));
> > > +	ida_simple_remove(&guc->ct.owner_ida, ctch->owner);
> > > +err_vma:
> > > +	i915_vma_unpin_and_release(&ctch->vma);
> > > +	return err;
> > > +}
> > > +
> > > +static void ctch_close(struct intel_guc *guc,
> > > +		       struct intel_guc_ct_channel *ctch)
> > > +{
> > > +	GEM_BUG_ON(!ctch_is_open(ctch));
> > > +
> > > +	guc_action_deregister_ct_buffer(guc,
> > > +					ctch->owner,
> > > +					INTEL_GUC_CT_BUFFER_TYPE_SEND);
> > > +	guc_action_deregister_ct_buffer(guc,
> > > +					ctch->owner,
> > > +					INTEL_GUC_CT_BUFFER_TYPE_RECV);
> > > +
> > > +	ida_simple_remove(&guc->ct.owner_ida, ctch->owner);
> > > +
> > > +	/* Unmap the page we mapped in the _open() */
> > > +	kunmap(i915_vma_first_page(ctch->vma));
> > > +
> > > +	/* Now we can safely release the vma */
> > > +	i915_vma_unpin_and_release(&ctch->vma);
> > > +}
> > > +
> > > +static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
> > > +{
> > > +	u32 fence;
> > > +
> > > +	fence = ctch->next_fence++;
> > > +	if (fence == 0)
> > > +		fence = ctch->next_fence++;
> > 
> > Why not zero? You are not using it as debug feature, so is the hw
> > unaccepting of zeroes?
> 
> It looks that it was added just to avoid false fence match of the
> first request. Will initialize next_fence in _open() instead.

++ctch->next_fence.

> > > +/*
> > > + * Command Transport (CT) buffer based GuC send function.
> > > + */
> > > +static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len)
> > > +{
> > > +	struct intel_guc_ct_channel *ctch = &guc->ct.channel;
> > > +	u32 status = ~0; /* undefined */
> > > +	int err;
> > > +
> > > +	mutex_lock(&guc->send_mutex);
> > > +	guc->action_count += 1;
> > > +	guc->action_cmd = action[0];
> > 
> > Please, please stop it with these. And go remove all the other garbage.
> > If you want to trace it, trace it. If you want just to debug, printk
> > and remove when done.
> 
> Hmm, but they are used by the existing i915_guc_info in debugfs.
> Do you really want to remove all that from debugfs and send_mmio()?

Yes. I dislike intensely the prevalence of poor debugfs stats throughout
guc. Mostly looking along the guc submission path where the majority of
the information was already available, and what little novelty might
have been a contender for adding to the existing tracing.

> > > +/** Represents pair of command transport buffers.
> > > + *
> > > + * Buffers go in pairs to allow bi-directional communication.
> > > + * To simplify the code we place both of them in the same vma.
> > > + * Buffers from the same pair must share unique owner id.
> > > + *
> > > + * @vma: pointer to the vma with pair of CT buffers
> > > + * @ctbs: buffers for sending(0) and receiving(1) commands
> > > + * @owner: unique identifier
> > > + * @next_fence: fence to be used with next send command
> > > + */
> > > +struct intel_guc_ct_channel {
> > > +	struct i915_vma *vma;
> > > +	struct intel_guc_ct_buffer ctbs[2]; /* 0=SEND 1=RECV */
> > 
> > You've almost written the enum already.
> 
> Well, similar values are already defined in fwif.h as
> 	INTEL_GUC_CT_BUFFER_TYPE_SEND|RECV
> but as they are part of the fwif, we can't trust them to be always 0|1.
> We may try to use common READ|WRITE if you don't like raw 0|1 indices.

The buffers are encoding the direction, and the special 0/1 are already
referred to in comments, so it would be nice to have the code describe
itself.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre