[Intel-gfx] [PATCH 2/5] drm/i915/guc/ct: stop expecting multiple CT channels
Michal Wajdeczko
michal.wajdeczko at intel.com
Wed Dec 11 13:43:14 UTC 2019
On Tue, 10 Dec 2019 22:09:16 +0100, Daniele Ceraolo Spurio
<daniele.ceraolospurio at intel.com> wrote:
> The GuC supports having multiple CT buffer pairs and we designed our
> implementation with that in mind. However, the different channels are not
> processed in parallel within the GuC, so there is very little advantage
> in having multiple channels (independent locks?), compared to the
> drawbacks (one channel can starve the other if messages keep being
> submitted to it). Given this, it is unlikely we'll ever add a second
> channel and therefore we can simplify our code by removing the
> flexibility.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: John Harrison <John.C.Harrison at Intel.com>
> Cc: Matthew Brost <matthew.brost at intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 276 +++++++++-------------
> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 39 +--
> 2 files changed, 118 insertions(+), 197 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index f74ba4750a94..96ce6d74f0b2 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -37,13 +37,10 @@ static void ct_incoming_request_worker_func(struct
> work_struct *w);
> */
> void intel_guc_ct_init_early(struct intel_guc_ct *ct)
> {
> - /* we're using static channel owners */
> - ct->host_channel.owner = CTB_OWNER_HOST;
> -
> - spin_lock_init(&ct->lock);
> - INIT_LIST_HEAD(&ct->pending_requests);
> - INIT_LIST_HEAD(&ct->incoming_requests);
> - INIT_WORK(&ct->worker, ct_incoming_request_worker_func);
> + spin_lock_init(&ct->requests.lock);
> + INIT_LIST_HEAD(&ct->requests.pending);
> + INIT_LIST_HEAD(&ct->requests.incoming);
> + INIT_WORK(&ct->requests.worker, ct_incoming_request_worker_func);
> }
> static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct)
> @@ -64,14 +61,14 @@ static inline const char
> *guc_ct_buffer_type_to_str(u32 type)
> }
> static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc,
> - u32 cmds_addr, u32 size, u32 owner)
> + u32 cmds_addr, u32 size)
> {
> - CT_DEBUG_DRIVER("CT: desc %p init addr=%#x size=%u owner=%u\n",
> - desc, cmds_addr, size, owner);
> + CT_DEBUG_DRIVER("CT: desc %p init addr=%#x size=%u\n",
> + desc, cmds_addr, size);
please drop %p
> memset(desc, 0, sizeof(*desc));
> desc->addr = cmds_addr;
> desc->size = size;
> - desc->owner = owner;
> + desc->owner = CTB_OWNER_HOST;
> }
> static void guc_ct_buffer_desc_reset(struct guc_ct_buffer_desc *desc)
> @@ -104,12 +101,11 @@ static int guc_action_register_ct_buffer(struct
> intel_guc *guc,
> }
> static int guc_action_deregister_ct_buffer(struct intel_guc *guc,
> - u32 owner,
> u32 type)
> {
> u32 action[] = {
> INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER,
> - owner,
> + CTB_OWNER_HOST,
> type
> };
> int err;
> @@ -117,19 +113,28 @@ static int guc_action_deregister_ct_buffer(struct
> intel_guc *guc,
> /* Can't use generic send(), CT deregistration must go over MMIO */
> err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);
> if (err)
> - DRM_ERROR("CT: deregister %s buffer failed; owner=%d err=%d\n",
> - guc_ct_buffer_type_to_str(type), owner, err);
> + DRM_ERROR("CT: deregister %s buffer failed; err=%d\n",
> + guc_ct_buffer_type_to_str(type), err);
> return err;
> }
> -static int ctch_init(struct intel_guc *guc,
> - struct intel_guc_ct_channel *ctch)
> +/**
> + * intel_guc_ct_init - Init CT communication
maybe to match other descriptions:
"intel_guc_ct_init - Init buffer based communication"
> + * @ct: pointer to CT struct
> + *
> + * Allocate memory required for communication via
> + * the CT channel.
CT channel ? maybe
"Allocate memory required for buffer based communication"
> + *
> + * Return: 0 on success, a negative errno code on failure.
> + */
> +int intel_guc_ct_init(struct intel_guc_ct *ct)
> {
> + struct intel_guc *guc = ct_to_guc(ct);
> void *blob;
> int err;
> int i;
> - GEM_BUG_ON(ctch->vma);
> + GEM_BUG_ON(ct->vma);
> /* We allocate 1 page to hold both descriptors and both buffers.
> * ___________.....................
> @@ -153,57 +158,67 @@ static int ctch_init(struct intel_guc *guc,
> * other code will need updating as well.
> */
> - err = intel_guc_allocate_and_map_vma(guc, PAGE_SIZE, &ctch->vma,
> &blob);
> + err = intel_guc_allocate_and_map_vma(guc, PAGE_SIZE, &ct->vma, &blob);
> if (err) {
> - CT_DEBUG_DRIVER("CT: channel %d initialization failed; err=%d\n",
> - ctch->owner, err);
> + DRM_ERROR("CT: channel allocation failed; err=%d\n", err);
> return err;
> }
> CT_DEBUG_DRIVER("CT: vma base=%#x\n",
> - intel_guc_ggtt_offset(guc, ctch->vma));
> + intel_guc_ggtt_offset(guc, ct->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;
> + for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
> + GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
> + ct->ctbs[i].desc = blob + PAGE_SIZE/4 * i;
> + ct->ctbs[i].cmds = blob + PAGE_SIZE/4 * i + PAGE_SIZE/2;
> }
> return 0;
> }
> -static void ctch_fini(struct intel_guc *guc,
> - struct intel_guc_ct_channel *ctch)
> +/**
> + * intel_guc_ct_fini - Fini CT communication
> + * @ct: pointer to CT struct
> + *
> + * Deallocate memory required for communication via
> + * the CT channel.
> + */
> +void intel_guc_ct_fini(struct intel_guc_ct *ct)
> {
> - GEM_BUG_ON(ctch->enabled);
> + GEM_BUG_ON(ct->enabled);
> - i915_vma_unpin_and_release(&ctch->vma, I915_VMA_RELEASE_MAP);
> + i915_vma_unpin_and_release(&ct->vma, I915_VMA_RELEASE_MAP);
> }
> -static int ctch_enable(struct intel_guc *guc,
> - struct intel_guc_ct_channel *ctch)
> +/**
> + * intel_guc_ct_enable - Enable buffer based command transport.
> + * @ct: pointer to CT struct
> + *
> + * Return: 0 on success, a negative errno code on failure.
> + */
> +int intel_guc_ct_enable(struct intel_guc_ct *ct)
> {
> + struct intel_guc *guc = ct_to_guc(ct);
> u32 base;
> int err;
> int i;
> - GEM_BUG_ON(!ctch->vma);
> -
> - GEM_BUG_ON(ctch->enabled);
> + if (ct->enabled)
> + return 0;
btw, do we still need this check?
> /* vma should be already allocated and map'ed */
> - base = intel_guc_ggtt_offset(guc, ctch->vma);
> + GEM_BUG_ON(!ct->vma);
> + base = intel_guc_ggtt_offset(guc, ct->vma);
> /* (re)initialize descriptors
> * cmds buffers are in the second half of the blob page
> */
> - for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) {
> + for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
> GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
> - guc_ct_buffer_desc_init(ctch->ctbs[i].desc,
> + guc_ct_buffer_desc_init(ct->ctbs[i].desc,
> base + PAGE_SIZE/4 * i + PAGE_SIZE/2,
> - PAGE_SIZE/4,
> - ctch->owner);
> + PAGE_SIZE/4);
> }
> /* register buffers, starting wirh RECV buffer
> @@ -221,40 +236,43 @@ static int ctch_enable(struct intel_guc *guc,
> if (unlikely(err))
> goto err_deregister;
> - ctch->enabled = true;
> + ct->enabled = true;
> return 0;
> err_deregister:
> guc_action_deregister_ct_buffer(guc,
> - ctch->owner,
> INTEL_GUC_CT_BUFFER_TYPE_RECV);
> err_out:
> - DRM_ERROR("CT: can't open channel %d; err=%d\n", ctch->owner, err);
> + DRM_ERROR("CT: can't open channel; err=%d\n", err);
> return err;
> }
> -static void ctch_disable(struct intel_guc *guc,
> - struct intel_guc_ct_channel *ctch)
> +/**
> + * intel_guc_ct_disable - Disable buffer based command transport.
> + * @ct: pointer to CT struct
> + */
> +void intel_guc_ct_disable(struct intel_guc_ct *ct)
> {
> - GEM_BUG_ON(!ctch->enabled);
> + struct intel_guc *guc = ct_to_guc(ct);
> - ctch->enabled = false;
> + if (!ct->enabled)
> + return;
> +
> + ct->enabled = false;
> if (intel_guc_is_running(guc)) {
> 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);
> }
> }
> -static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
> +static u32 ct_get_next_fence(struct intel_guc_ct *ct)
> {
> /* For now it's trivial */
> - return ++ctch->next_fence;
> + return ++ct->next_fence;
> }
> /**
> @@ -427,35 +445,34 @@ static int wait_for_ct_request_update(struct
> ct_request *req, u32 *status)
> return err;
> }
> -static int ctch_send(struct intel_guc_ct *ct,
> - struct intel_guc_ct_channel *ctch,
> - const u32 *action,
> - u32 len,
> - u32 *response_buf,
> - u32 response_buf_size,
> - u32 *status)
> +static int ct_send(struct intel_guc_ct *ct,
> + const u32 *action,
> + u32 len,
> + u32 *response_buf,
> + u32 response_buf_size,
> + u32 *status)
> {
> - struct intel_guc_ct_buffer *ctb = &ctch->ctbs[CTB_SEND];
> + struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_SEND];
> struct guc_ct_buffer_desc *desc = ctb->desc;
> struct ct_request request;
> unsigned long flags;
> u32 fence;
> int err;
> - GEM_BUG_ON(!ctch->enabled);
> + GEM_BUG_ON(!ct->enabled);
> GEM_BUG_ON(!len);
> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> GEM_BUG_ON(!response_buf && response_buf_size);
> - fence = ctch_get_next_fence(ctch);
> + fence = ct_get_next_fence(ct);
> request.fence = fence;
> request.status = 0;
> request.response_len = response_buf_size;
> request.response_buf = response_buf;
> - spin_lock_irqsave(&ct->lock, flags);
> - list_add_tail(&request.link, &ct->pending_requests);
> - spin_unlock_irqrestore(&ct->lock, flags);
> + spin_lock_irqsave(&ct->requests.lock, flags);
> + list_add_tail(&request.link, &ct->requests.pending);
> + spin_unlock_irqrestore(&ct->requests.lock, flags);
> err = ctb_write(ctb, action, len, fence, !!response_buf);
> if (unlikely(err))
> @@ -488,9 +505,9 @@ static int ctch_send(struct intel_guc_ct *ct,
> }
> unlink:
> - spin_lock_irqsave(&ct->lock, flags);
> + spin_lock_irqsave(&ct->requests.lock, flags);
> list_del(&request.link);
> - spin_unlock_irqrestore(&ct->lock, flags);
> + spin_unlock_irqrestore(&ct->requests.lock, flags);
> return err;
> }
> @@ -502,14 +519,12 @@ int intel_guc_send_ct(struct intel_guc *guc, const
> u32 *action, u32 len,
> u32 *response_buf, u32 response_buf_size)
> {
> struct intel_guc_ct *ct = &guc->ct;
> - struct intel_guc_ct_channel *ctch = &ct->host_channel;
> u32 status = ~0; /* undefined */
> int ret;
> mutex_lock(&guc->send_mutex);
> - ret = ctch_send(ct, ctch, action, len, response_buf, response_buf_size,
> - &status);
> + ret = ct_send(ct, action, len, response_buf, response_buf_size,
> &status);
> if (unlikely(ret < 0)) {
> DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
> action[0], ret, status);
> @@ -640,8 +655,8 @@ static int ct_handle_response(struct intel_guc_ct
> *ct, const u32 *msg)
> CT_DEBUG_DRIVER("CT: response fence %u status %#x\n", fence, status);
> - spin_lock(&ct->lock);
> - list_for_each_entry(req, &ct->pending_requests, link) {
> + spin_lock(&ct->requests.lock);
> + list_for_each_entry(req, &ct->requests.pending, link) {
> if (unlikely(fence != req->fence)) {
> CT_DEBUG_DRIVER("CT: request %u awaits response\n",
> req->fence);
> @@ -659,7 +674,7 @@ static int ct_handle_response(struct intel_guc_ct
> *ct, const u32 *msg)
> found = true;
> break;
> }
> - spin_unlock(&ct->lock);
> + spin_unlock(&ct->requests.lock);
> if (!found)
> DRM_ERROR("CT: unsolicited response %*ph\n", 4 * msglen, msg);
> @@ -697,13 +712,13 @@ static bool ct_process_incoming_requests(struct
> intel_guc_ct *ct)
> u32 *payload;
> bool done;
> - spin_lock_irqsave(&ct->lock, flags);
> - request = list_first_entry_or_null(&ct->incoming_requests,
> + spin_lock_irqsave(&ct->requests.lock, flags);
> + request = list_first_entry_or_null(&ct->requests.incoming,
> struct ct_incoming_request, link);
> if (request)
> list_del(&request->link);
> - done = !!list_empty(&ct->incoming_requests);
> - spin_unlock_irqrestore(&ct->lock, flags);
> + done = !!list_empty(&ct->requests.incoming);
> + spin_unlock_irqrestore(&ct->requests.lock, flags);
> if (!request)
> return true;
> @@ -721,12 +736,13 @@ static bool ct_process_incoming_requests(struct
> intel_guc_ct *ct)
> static void ct_incoming_request_worker_func(struct work_struct *w)
> {
> - struct intel_guc_ct *ct = container_of(w, struct intel_guc_ct, worker);
> + struct intel_guc_ct *ct =
> + container_of(w, struct intel_guc_ct, requests.worker);
> bool done;
> done = ct_process_incoming_requests(ct);
> if (!done)
> - queue_work(system_unbound_wq, &ct->worker);
> + queue_work(system_unbound_wq, &ct->requests.worker);
> }
> /**
> @@ -764,22 +780,26 @@ static int ct_handle_request(struct intel_guc_ct
> *ct, const u32 *msg)
> }
> memcpy(request->msg, msg, 4 * msglen);
> - spin_lock_irqsave(&ct->lock, flags);
> - list_add_tail(&request->link, &ct->incoming_requests);
> - spin_unlock_irqrestore(&ct->lock, flags);
> + spin_lock_irqsave(&ct->requests.lock, flags);
> + list_add_tail(&request->link, &ct->requests.incoming);
> + spin_unlock_irqrestore(&ct->requests.lock, flags);
> - queue_work(system_unbound_wq, &ct->worker);
> + queue_work(system_unbound_wq, &ct->requests.worker);
> return 0;
> }
> -static void ct_process_host_channel(struct intel_guc_ct *ct)
> +/*
> + * When we're communicating with the GuC over CT, GuC uses events
> + * to notify us about new messages being posted on the RECV buffer.
> + */
> +void intel_guc_to_host_event_handler_ct(struct intel_guc *guc)
> {
> - struct intel_guc_ct_channel *ctch = &ct->host_channel;
> - struct intel_guc_ct_buffer *ctb = &ctch->ctbs[CTB_RECV];
> + struct intel_guc_ct *ct = &guc->ct;
> + struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_RECV];
> u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
> int err = 0;
> - if (!ctch->enabled)
> + if (!ct->enabled)
> return;
> do {
> @@ -798,87 +818,3 @@ static void ct_process_host_channel(struct
> intel_guc_ct *ct)
> ctb->desc->is_in_error = 1;
> }
> }
> -
> -/*
> - * When we're communicating with the GuC over CT, GuC uses events
> - * to notify us about new messages being posted on the RECV buffer.
> - */
> -void intel_guc_to_host_event_handler_ct(struct intel_guc *guc)
> -{
> - struct intel_guc_ct *ct = &guc->ct;
> -
> - ct_process_host_channel(ct);
> -}
> -
> -/**
> - * intel_guc_ct_init - Init CT communication
> - * @ct: pointer to CT struct
> - *
> - * Allocate memory required for communication via
> - * the CT channel.
> - *
> - * Return: 0 on success, a negative errno code on failure.
> - */
> -int intel_guc_ct_init(struct intel_guc_ct *ct)
> -{
> - struct intel_guc *guc = ct_to_guc(ct);
> - struct intel_guc_ct_channel *ctch = &ct->host_channel;
> - int err;
> -
> - err = ctch_init(guc, ctch);
> - if (unlikely(err)) {
> - DRM_ERROR("CT: can't open channel %d; err=%d\n",
> - ctch->owner, err);
> - return err;
> - }
> -
> - GEM_BUG_ON(!ctch->vma);
> - return 0;
> -}
> -
> -/**
> - * intel_guc_ct_fini - Fini CT communication
> - * @ct: pointer to CT struct
> - *
> - * Deallocate memory required for communication via
> - * the CT channel.
> - */
> -void intel_guc_ct_fini(struct intel_guc_ct *ct)
> -{
> - struct intel_guc *guc = ct_to_guc(ct);
> - struct intel_guc_ct_channel *ctch = &ct->host_channel;
> -
> - ctch_fini(guc, ctch);
> -}
> -
> -/**
> - * intel_guc_ct_enable - Enable buffer based command transport.
> - * @ct: pointer to CT struct
> - *
> - * Return: 0 on success, a negative errno code on failure.
> - */
> -int intel_guc_ct_enable(struct intel_guc_ct *ct)
> -{
> - struct intel_guc *guc = ct_to_guc(ct);
> - struct intel_guc_ct_channel *ctch = &ct->host_channel;
> -
> - if (ctch->enabled)
> - return 0;
> -
> - return ctch_enable(guc, ctch);
> -}
> -
> -/**
> - * intel_guc_ct_disable - Disable buffer based command transport.
> - * @ct: pointer to CT struct
> - */
> -void intel_guc_ct_disable(struct intel_guc_ct *ct)
> -{
> - struct intel_guc *guc = ct_to_guc(ct);
> - struct intel_guc_ct_channel *ctch = &ct->host_channel;
> -
> - if (!ctch->enabled)
> - return;
> -
> - ctch_disable(guc, ctch);
> -}
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> index 77c80d6cc25d..4bb1d1fcc860 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> @@ -35,44 +35,29 @@ struct intel_guc_ct_buffer {
> u32 *cmds;
> };
> +
> /** Represents pair of command transport buffers.
intel_guc_ct is now little more that old ctch
> *
> * 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.
we'll have only one pair and one fixed owner,
maybe worth to rephrase whole description ?
> - *
> - * @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 intel_guc_ct {
> struct i915_vma *vma;
> - struct intel_guc_ct_buffer ctbs[2];
> - u32 owner;
> - u32 next_fence;
> bool enabled;
> -};
> -/** Holds all command transport channels.
> - *
> - * @host_channel: main channel used by the host
> - */
> -struct intel_guc_ct {
> - struct intel_guc_ct_channel host_channel;
> - /* other channels are tbd */
> -
> - /** @lock: protects pending requests list */
> - spinlock_t lock;
> -
> - /** @pending_requests: list of requests waiting for response */
> - struct list_head pending_requests;
> + /* buffers for sending(0) and receiving(1) commands */
> + struct intel_guc_ct_buffer ctbs[2];
> - /** @incoming_requests: list of incoming requests */
> - struct list_head incoming_requests;
> + /* fence to be used with next send command */
> + u32 next_fence;
fence is only used while sending requests,
so maybe move it under below requests struct ?
> - /** @worker: worker for handling incoming requests */
> - struct work_struct worker;
> + struct {
> + spinlock_t lock; /* protects pending requests list */
> + struct list_head pending; /* requests waiting for response */
> + struct list_head incoming; /* incoming requests */
> + struct work_struct worker; /* handler for incoming requests */
> + } requests;
maybe above change should be in separate patch to make diff smaller?
> };
> void intel_guc_ct_init_early(struct intel_guc_ct *ct);
More information about the Intel-gfx
mailing list