[Intel-gfx] [PATCH v4 08/13] drm/i915/guc: Implement response handling in send_ct()

[Michał Winiarski]

On Fri, Mar 23, 2018 at 02:47:23PM +0000, 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(-)
> 

[SNIP]

> @@ -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 */

Magic numbers, please ether define 3 as min payload length or hide this behind
macro.

> +	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);

Isn't this called from the irq? We can use plain spin_lock here.

> +	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;

Is this expected?
In other words - do we expect out of order responses?
Can we extract this into a helper (find request)?

-Michał

> +		}
> +		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;
> +	struct list_head pending_requests;
>  };
>  
>  void intel_guc_ct_init_early(struct intel_guc_ct *ct);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx