Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 27/05/2021 15:35, Matthew Brost wrote:
> On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
>>
>> On 26/05/2021 19:10, Matthew Brost wrote:
>>
>> [snip]
>>
>>>>>>> +static int ct_send_nb(struct intel_guc_ct *ct,
>>>>>>> +		      const u32 *action,
>>>>>>> +		      u32 len,
>>>>>>> +		      u32 flags)
>>>>>>> +{
>>>>>>> +	struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
>>>>>>> +	unsigned long spin_flags;
>>>>>>> +	u32 fence;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	spin_lock_irqsave(&ctb->lock, spin_flags);
>>>>>>> +
>>>>>>> +	ret = ctb_has_room(ctb, len + 1);
>>>>>>> +	if (unlikely(ret))
>>>>>>> +		goto out;
>>>>>>> +
>>>>>>> +	fence = ct_get_next_fence(ct);
>>>>>>> +	ret = ct_write(ct, action, len, fence, flags);
>>>>>>> +	if (unlikely(ret))
>>>>>>> +		goto out;
>>>>>>> +
>>>>>>> +	intel_guc_notify(ct_to_guc(ct));
>>>>>>> +
>>>>>>> +out:
>>>>>>> +	spin_unlock_irqrestore(&ctb->lock, spin_flags);
>>>>>>> +
>>>>>>> +	return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>>      static int ct_send(struct intel_guc_ct *ct,
>>>>>>>      		   const u32 *action,
>>>>>>>      		   u32 len,
>>>>>>> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
>>>>>>>      		   u32 response_buf_size,
>>>>>>>      		   u32 *status)
>>>>>>>      {
>>>>>>> +	struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
>>>>>>>      	struct ct_request request;
>>>>>>>      	unsigned long flags;
>>>>>>>      	u32 fence;
>>>>>>> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
>>>>>>>      	GEM_BUG_ON(!len);
>>>>>>>      	GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>>>>>>>      	GEM_BUG_ON(!response_buf && response_buf_size);
>>>>>>> +	might_sleep();
>>>>>>
>>>>>> Sleep is just cond_resched below or there is more?
>>>>>>
>>>>>
>>>>> Yes, the cond_resched.
>>>>>
>>>>>>> +	/*
>>>>>>> +	 * We use a lazy spin wait loop here as we believe that if the CT
>>>>>>> +	 * buffers are sized correctly the flow control condition should be
>>>>>>> +	 * rare.
>>>>>>> +	 */
>>>>>>> +retry:
>>>>>>>      	spin_lock_irqsave(&ct->ctbs.send.lock, flags);
>>>>>>> +	if (unlikely(!ctb_has_room(ctb, len + 1))) {
>>>>>>> +		spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
>>>>>>> +		cond_resched();
>>>>>>> +		goto retry;
>>>>>>> +	}
>>>>>>
>>>>>> If this patch is about adding a non-blocking send function, and below we can
>>>>>> see that it creates a fork:
>>>>>>
>>>>>> intel_guc_ct_send:
>>>>>> ...
>>>>>> 	if (flags & INTEL_GUC_SEND_NB)
>>>>>> 		return ct_send_nb(ct, action, len, flags);
>>>>>>
>>>>>>     	ret = ct_send(ct, action, len, response_buf, response_buf_size, &status);
>>>>>>
>>>>>> Then why is there a change in ct_send here, which is not the new
>>>>>> non-blocking path?
>>>>>>
>>>>>
>>>>> There is not a change to ct_send(), just to intel_guc_ct_send.
>>>>
>>>> I was doing by the diff which says:
>>>>
>>>>    static int ct_send(struct intel_guc_ct *ct,
>>>>    		   const u32 *action,
>>>>    		   u32 len,
>>>> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
>>>>    		   u32 response_buf_size,
>>>>    		   u32 *status)
>>>>    {
>>>> +	struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
>>>>    	struct ct_request request;
>>>>    	unsigned long flags;
>>>>    	u32 fence;
>>>> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
>>>>    	GEM_BUG_ON(!len);
>>>>    	GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>>>>    	GEM_BUG_ON(!response_buf && response_buf_size);
>>>> +	might_sleep();
>>>> +	/*
>>>> +	 * We use a lazy spin wait loop here as we believe that if the CT
>>>> +	 * buffers are sized correctly the flow control condition should be
>>>> +	 * rare.
>>>> +	 */
>>>> +retry:
>>>>    	spin_lock_irqsave(&ct->ctbs.send.lock, flags);
>>>> +	if (unlikely(!ctb_has_room(ctb, len + 1))) {
>>>> +		spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
>>>> +		cond_resched();
>>>> +		goto retry;
>>>> +	}
>>>>
>>>> So it looks like a change to ct_send to me. Is that wrong?
>>
>> What about this part - is the patch changing the blocking ct_send or not,
>> and if it is why?
>>
> 
> Yes, ct_send() changes. Sorry for the confusion.
> 
> This function needs to be updated to account for the H2G space and
> backoff if no space is available.

Since this one is the sleeping path, it probably can and needs to be 
smarter than having a cond_resched busy loop added. Like sleep and get 
woken up when there is space. Otherwise it can degenerate to busy 
looping via contention with the non-blocking path.

Regards,

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