[Intel-gfx] [PATCH v3 2/3] drm/i915/guc: Introduce buffer based cmd transport
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Wed May 24 18:35:15 UTC 2017
<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.
<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?
> +
> +/* 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.
Thanks,
Daniele
> +} __packed;
> +
> +/* Type of command transport buffer */
> +#define INTEL_GUC_CT_BUFFER_TYPE_SEND 0x0u
> +#define INTEL_GUC_CT_BUFFER_TYPE_RECV 0x1u
> +
> +/*
> + * Definition of the command transport message header (DW0)
> + *
> + * bit[4..0] message len (in dwords)
> + * bit[7..5] reserved
> + * bit[8] write fence to desc
> + * bit[9] write status to H2G buff
> + * bit[10] send status (via G2H)
> + * bit[15..11] reserved
> + * bit[31..16] action code
> + */
> +#define GUC_CT_MSG_LEN_SHIFT 0
> +#define GUC_CT_MSG_LEN_MASK 0x1F
> +#define GUC_CT_MSG_WRITE_FENCE_TO_DESC (1 << 8)
> +#define GUC_CT_MSG_WRITE_STATUS_TO_BUFF (1 << 9)
> +#define GUC_CT_MSG_SEND_STATUS (1 << 10)
> +#define GUC_CT_MSG_ACTION_SHIFT 16
> +#define GUC_CT_MSG_ACTION_MASK 0xFFFF
> +
> #define GUC_FORCEWAKE_RENDER (1 << 0)
> #define GUC_FORCEWAKE_MEDIA (1 << 1)
>
> @@ -515,6 +555,8 @@ enum intel_guc_action {
> INTEL_GUC_ACTION_EXIT_S_STATE = 0x502,
> INTEL_GUC_ACTION_SLPC_REQUEST = 0x3003,
> INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
> + INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,
> + INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER = 0x4506,
> INTEL_GUC_ACTION_UK_LOG_ENABLE_LOGGING = 0x0E000,
> INTEL_GUC_ACTION_LIMIT
> };
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 31dc8c3..6b8a450 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -288,14 +288,24 @@ static void guc_init_send_regs(struct intel_guc *guc)
>
> static int guc_enable_communication(struct intel_guc *guc)
> {
> - /* XXX: placeholder for alternate setup */
> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> guc_init_send_regs(guc);
> +
> + if (HAS_GUC_CT(dev_priv))
> + return intel_guc_enable_ct(guc);
> +
> guc->send = intel_guc_send_mmio;
> return 0;
> }
>
> static void guc_disable_communication(struct intel_guc *guc)
> {
> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> + if (HAS_GUC_CT(dev_priv))
> + intel_guc_disable_ct(guc);
> +
> guc->send = intel_guc_send_nop;
> }
>
> @@ -442,6 +452,11 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> GEM_BUG_ON(!len);
> GEM_BUG_ON(len > guc->send_regs.count);
>
> + /* If CT is available, we expect to use MMIO only during init/fini */
> + GEM_BUG_ON(HAS_GUC_CT(dev_priv) &&
> + *action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
> + *action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
> +
> mutex_lock(&guc->send_mutex);
> intel_uncore_forcewake_get(dev_priv, guc->send_regs.fw_domains);
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 930f2e1..fb1d640 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -27,7 +27,7 @@
> #include "intel_guc_fwif.h"
> #include "i915_guc_reg.h"
> #include "intel_ringbuffer.h"
> -
> +#include "intel_guc_ct.h"
> #include "i915_vma.h"
>
> struct drm_i915_gem_request;
> @@ -173,6 +173,7 @@ struct intel_guc_log {
> struct intel_guc {
> struct intel_uc_fw fw;
> struct intel_guc_log log;
> + struct intel_guc_ct ct;
>
> /* intel_guc_recv interrupt related state */
> bool interrupts_enabled;
>
More information about the Intel-gfx
mailing list