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

[Michal Wajdeczko]

On Wed, May 24, 2017 at 11:35:15AM -0700, Daniele Ceraolo Spurio wrote:
> <snip>
> 
> > +
> > +static int ctch_init(struct intel_guc *guc,
> > +		     struct intel_guc_ct_channel *ctch)
> > +{
> > +	struct i915_vma *vma;
> > +	void *blob;
> > +	int err;
> > +	int i;
> > +
> > +	GEM_BUG_ON(ctch->vma);
> > +
> > +	/* We allocate 1 page to hold both descriptors and both buffers.
> > +	 *       ___________.....................
> > +	 *      |desc (SEND)|                   :
> > +	 *      |___________|                   PAGE/4
> > +	 *      :___________....................:
> > +	 *      |desc (RECV)|                   :
> > +	 *      |___________|                   PAGE/4
> > +	 *      :_______________________________:
> > +	 *      |cmds (SEND)                    |
> > +	 *      |                               PAGE/4
> > +	 *      |_______________________________|
> > +	 *      |cmds (RECV)                    |
> > +	 *      |                               PAGE/4
> > +	 *      |_______________________________|
> > +	 *
> > +	 * Each message can use a maximum of 32 dwords and we don't expect to
> > +	 * have more than 1 in flight at any time, so we have enough space.
> > +	 * Some logic further ahead will rely on the fact that there is only 1
> > +	 * page and that it is always mapped, so if the size is changed the
> > +	 * other code will need updating as well.
> > +	 */
> > +
> > +	/* allocate vma */
> > +	vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
> > +	if (IS_ERR(vma)) {
> > +		err = PTR_ERR(vma);
> > +		goto err_out;
> > +	}
> > +	ctch->vma = vma;
> > +
> > +	/* map first page */
> > +	blob = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> > +	if (IS_ERR(blob)) {
> > +		err = PTR_ERR(blob);
> > +		goto err_vma;
> > +	}
> > +	DRM_DEBUG_DRIVER("CT: vma base=%#x\n", guc_ggtt_offset(ctch->vma));
> > +
> > +	/* store pointers to desc and cmds */
> > +	for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) {
> > +		GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
> > +		ctch->ctbs[i].desc = blob + PAGE_SIZE/4 * i;
> > +		ctch->ctbs[i].cmds = blob + PAGE_SIZE/4 * i + PAGE_SIZE/2;
> > +	}
> > +
> > +	ctch->owner = CTB_OWNER_HOST;
> 
> mmmh, this function gets a generic struct intel_guc_ct_channel as input but
> here it assumes that it is the host one. If we want it to be generic then we
> need to pass the owner from outside, otherwise if we assume that this is the
> host channel then we can just do:
> 
> 	ctch = &guc->ct.channel;
> 
> without passing it as a parameter. My preference goes to option 1.
> 

Yeah, this was just too easy shortcut. Now as we agree that owners ID are static,
I've moved this initialization to the reincarnated guc_ct_init_early(), where
we'll be able to pre-assign owners to all future channels.


> 
> <snip>
> 
> > diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
> > new file mode 100644
> > index 0000000..8df759d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
> > @@ -0,0 +1,80 @@
> > +/*
> > + * Copyright © 2016-2017 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef _INTEL_GUC_CT_H_
> > +#define _INTEL_GUC_CT_H_
> > +
> > +struct intel_guc;
> > +struct i915_vma;
> > +
> > +#include "intel_guc_fwif.h"
> > +
> > +/**
> > + * DOC: Command Transport (CT).
> > + *
> > + * Buffer based command transport is a replacement for MMIO based mechanism.
> > + * It can be used to perform both host-2-guc and guc-to-host communication.
> > + */
> > +
> > +/** Represents single command transport buffer.
> > + *
> > + * A single command transport buffer consists of two parts, the header
> > + * record (command transport buffer descriptor) and the actual buffer which
> > + * holds the commands.
> > + *
> > + * @desc: pointer to the buffer descriptor
> > + * @cmds: pointer to the commands buffer
> > + */
> > +struct intel_guc_ct_buffer {
> > +	struct guc_ct_buffer_desc *desc;
> > +	u32 *cmds;
> > +};
> > +
> > +/** 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];
> > +	u32 owner;
> > +	u32 next_fence;
> > +};
> > +
> > +/* */
> > +struct intel_guc_ct {
> > +	struct intel_guc_ct_channel channel;
> > +};
> 
> Do we still need this structure now that there is no ida? Can't we just use
> intel_guc_ct_channel directly?

Single ct_channel is our (low level) representation of the single CT channel.
As we expect more possible channels, we must be prepared where we locate them.
This higher level guc_ct struct will be a perfect place.


> 
> > +
> > +/* XXX: move to intel_uc.h ? don't fit there either */
> > +int intel_guc_enable_ct(struct intel_guc *guc);
> > +void intel_guc_disable_ct(struct intel_guc *guc);
> > +
> > +#endif /* _INTEL_GUC_CT_H_ */
> > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> > index 6156845..c252e90 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> > @@ -331,6 +331,46 @@ struct guc_stage_desc {
> >  	u64 desc_private;
> >  } __packed;
> > 
> > +/*
> > + * Describes single command transport buffer.
> > + * Used by both guc-master and clients.
> > + */
> > +struct guc_ct_buffer_desc {
> > +	u32 addr;		/* gfx address */
> > +	u64 host_private;	/* owner private data */
> > +	u32 size;		/* size in bytes */
> > +	u32 head;		/* offset updated by GuC*/
> > +	u32 tail;		/* offset updated by owner */
> > +	u32 is_in_error;	/* error indicator */
> > +	u32 fence;		/* fence updated by GuC */
> > +	u32 status;		/* status updated by GuC */
> > +	u32 owner;		/* id assigned by owner */
> 
> maybe something like
> 	
> 	/* id of the channel owner */
> 
> might be clearer as the owner does not decide its id, it can only decide the
> sub_ids
> 
> > +	u32 reserved[6];
> 
> I know from our IM conversation that you prefer to not define the
> subtracking_id field because we do not use it. However, until now we're
> always kept our definitions in sync with the GuC FW so I'd prefer to do the
> same here. We can use a comment to make the field use clear even if we don't
> use it at the moment.

Ok, I'll add this field.

Thanks,
Michal