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

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Mon, 26 Mar 2018 17:29:32 +0200, Michał Winiarski  
<michal.winiarski@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.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@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.

Well, it looks that detailed CTB documentation can't wait any more...
(simplified doc was already there:

	/* Response message shall at least include header, fence and status */

I'll try to include more docs in next series spin to make these numbers  
more
clear for everyone

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

yes, will update, thanks

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

rather not, tempting to mark that with 'unlikely'

> In other words - do we expect out of order responses?

not today, since we're using send_mutex to serialize all requests
(serialization was required for MMIO based comm, but not for CT)

> Can we extract this into a helper (find request)?

will try, but can't promise

thanks,
/m
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx