[Intel-gfx] [PATCH v4 08/13] drm/i915/guc: Implement response handling in send_ct()
Michel Thierry
michel.thierry at intel.com
Fri Mar 23 22:55:18 UTC 2018
On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> Instead of returning small data in response status dword,
> GuC may append longer data as response message payload.
> If caller provides response buffer, we will copy received
> data and use number of received data dwords as new success
> return value. We will WARN if response from GuC does not
> match caller expectation.
>
> v2: fix timeout and checkpatch warnings (Michal)
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Michel Thierry <michel.thierry at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> ---
> drivers/gpu/drm/i915/intel_guc_ct.c | 137 ++++++++++++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_guc_ct.h | 5 ++
> 2 files changed, 128 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index b1ab28f..6a9aad0 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -24,6 +24,14 @@
> #include "i915_drv.h"
> #include "intel_guc_ct.h"
>
> +struct ct_request {
> + struct list_head link;
> + u32 fence;
> + u32 status;
> + u32 response_len;
> + u32 *response_buf;
> +};
> +
> enum { CTB_SEND = 0, CTB_RECV = 1 };
>
> enum { CTB_OWNER_HOST = 0 };
> @@ -36,6 +44,9 @@ 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);
> }
>
> static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct)
> @@ -276,7 +287,8 @@ static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
> static int ctb_write(struct intel_guc_ct_buffer *ctb,
> const u32 *action,
> u32 len /* in dwords */,
> - u32 fence)
> + u32 fence,
> + bool want_response)
> {
> struct guc_ct_buffer_desc *desc = ctb->desc;
> u32 head = desc->head / 4; /* in dwords */
> @@ -312,6 +324,7 @@ static int ctb_write(struct intel_guc_ct_buffer *ctb,
> */
> header = (len << GUC_CT_MSG_LEN_SHIFT) |
> (GUC_CT_MSG_WRITE_FENCE_TO_DESC) |
> + (want_response ? GUC_CT_MSG_SEND_STATUS : 0) |
> (action[0] << GUC_CT_MSG_ACTION_SHIFT);
>
> cmds[tail] = header;
> @@ -380,36 +393,104 @@ static int wait_for_desc_update(struct guc_ct_buffer_desc *desc,
> return err;
> }
>
> -static int ctch_send(struct intel_guc *guc,
> +/**
> + * wait_for_response_msg - Wait for the Guc response.
> + * @req: pointer to pending request
> + * @status: placeholder for status
> + *
> + * We will update request status from the response message handler.
> + * Returns:
> + * * 0 response received (status is valid)
> + * * -ETIMEDOUT no response within hardcoded timeout
> + */
> +static int wait_for_response_msg(struct ct_request *req, u32 *status)
> +{
> + int err;
> +
> + /*
> + * Fast commands should complete in less than 10us, so sample quickly
> + * up to that length of time, then switch to a slower sleep-wait loop.
> + * No GuC command should ever take longer than 10ms.
> + */
> +#define done INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status))
> + err = wait_for_us(done, 10);
> + if (err)
> + err = wait_for(done, 10);
> +#undef done
> +
> + if (unlikely(err))
> + DRM_ERROR("CT: fence %u err %d\n", req->fence, err);
> +
> + *status = req->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)
> {
> struct intel_guc_ct_buffer *ctb = &ctch->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_is_open(ctch));
> 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);
> - err = ctb_write(ctb, action, len, fence);
> + 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);
> +
> + err = ctb_write(ctb, action, len, fence, !!response_buf);
> if (unlikely(err))
> - return err;
> + goto unlink;
>
> - intel_guc_notify(guc);
> + intel_guc_notify(ct_to_guc(ct));
>
> - err = wait_for_desc_update(desc, fence, status);
> + if (response_buf)
> + err = wait_for_response_msg(&request, status);
> + else
> + err = wait_for_desc_update(desc, fence, status);
> if (unlikely(err))
> - return err;
> - if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status))
> - return -EIO;
> + goto unlink;
> +
> + if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status)) {
> + err = -EIO;
> + goto unlink;
> + }
> +
> + if (response_buf) {
> + /* There shall be no data in the status */
> + WARN_ON(INTEL_GUC_MSG_TO_DATA(request.status));
> + /* Return actual response len */
> + err = request.response_len;
> + } else {
> + /* There shall be no response payload */
> + WARN_ON(request.response_len);
> + /* Return data decoded from the status dword */
> + err = INTEL_GUC_MSG_TO_DATA(*status);
> + }
>
> - /* Use data from the GuC status as our return value */
> - return INTEL_GUC_MSG_TO_DATA(*status);
> +unlink:
> + spin_lock_irqsave(&ct->lock, flags);
> + list_del(&request.link);
> + spin_unlock_irqrestore(&ct->lock, flags);
> +
> + return err;
> }
>
> /*
> @@ -418,13 +499,15 @@ static int ctch_send(struct intel_guc *guc,
> static 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_channel *ctch = &guc->ct.host_channel;
> + 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(guc, ctch, action, len, &status);
> + ret = ctch_send(ct, ctch, 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);
> @@ -503,8 +586,13 @@ static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
> static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
> {
> u32 header = msg[0];
> + u32 fence = msg[1];
> u32 status = msg[2];
> u32 len = ct_header_get_len(header) + 1; /* total len with header */
> + u32 payload_len = len - 3; /* len<3 is treated as protocol error */
> + struct ct_request *req;
> + bool found = false;
> + unsigned long flags;
>
> GEM_BUG_ON(!ct_header_is_response(header));
>
> @@ -518,8 +606,29 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
> DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
> return -EPROTO;
> }
> + spin_lock_irqsave(&ct->lock, flags);
> + list_for_each_entry(req, &ct->pending_requests, link) {
> + if (req->fence != fence) {
> + DRM_DEBUG_DRIVER("CT: request %u awaits response\n",
> + req->fence);
> + continue;
> + }
> + if (unlikely(payload_len > req->response_len)) {
> + DRM_ERROR("CT: response %u too long %*phn\n",
> + req->fence, 4*len, msg);
> + payload_len = 0;
> + }
> + if (payload_len)
> + memcpy(req->response_buf, msg + 3, 4*payload_len);
> + req->response_len = payload_len;
> + WRITE_ONCE(req->status, status);
> + found = true;
> + break;
> + }
> + spin_unlock_irqrestore(&ct->lock, flags);
>
> - /* XXX */
> + if (!found)
> + DRM_ERROR("CT: unsolicited response %*phn\n", 4*len, msg);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
> index 595c8ad..905566b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
> @@ -71,10 +71,15 @@ struct intel_guc_ct_channel {
> /** Holds all command transport channels.
> *
> * @host_channel: main channel used by the host
> + * @lock: spin lock for pending requests list
> + * @pending_requests: list of pending requests
> */
> struct intel_guc_ct {
> struct intel_guc_ct_channel host_channel;
> /* other channels are tbd */
> +
> + spinlock_t lock;
I know you documented that this lock is for in the header, but the
checkpatch.pl wants it in the line before the definition :|
And there are a couple of 4*len and 4*payload_len
Reviewed-by: Michel Thierry <michel.thierry at intel.com>
> + struct list_head pending_requests;
> };
>
> void intel_guc_ct_init_early(struct intel_guc_ct *ct);
>
More information about the Intel-gfx
mailing list