On Mon, 26 Mar 2018, Michał Winiarski <michal.winiarski@xxxxxxxxx> wrote:
> 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@xxxxxxxxx>
>> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
>> Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>> ---
>> 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.
And check len >= 3 before substracting 3.
BR,
Jani.
>
>> + 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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
