On Mon, 26 Mar 2018, Michał Winiarski <michal.winiar...@intel.com> 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.wajdec...@intel.com> >> Cc: Oscar Mateo <oscar.ma...@intel.com> >> Cc: Michel Thierry <michel.thie...@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospu...@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.
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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx