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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
