[Intel-gfx] [PATCH 24/47] drm/i915/guc: Add several request trace points

[Tvrtko Ursulin]

On 20/07/2021 02:59, Matthew Brost wrote:
> On Tue, Jul 13, 2021 at 10:06:17AM +0100, Tvrtko Ursulin wrote:
>>
>> On 24/06/2021 08:04, Matthew Brost wrote:
>>> Add trace points for request dependencies and GuC submit. Extended
>>> existing request trace points to include submit fence value,, guc_id,
>>> and ring tail value.
>>>
>>> Cc: John Harrison <john.c.harrison at intel.com>
>>> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>>> ---
>>>    .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  3 ++
>>>    drivers/gpu/drm/i915/i915_request.c           |  3 ++
>>>    drivers/gpu/drm/i915/i915_trace.h             | 39 ++++++++++++++++++-
>>>    3 files changed, 43 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> index 89b3c7e5d15b..c2327eebc09c 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> @@ -422,6 +422,7 @@ static int guc_dequeue_one_context(struct intel_guc *guc)
>>>    			guc->stalled_request = last;
>>>    			return false;
>>>    		}
>>> +		trace_i915_request_guc_submit(last);
>>>    	}
>>>    	guc->stalled_request = NULL;
>>> @@ -642,6 +643,8 @@ static int guc_bypass_tasklet_submit(struct intel_guc *guc,
>>>    	ret = guc_add_request(guc, rq);
>>>    	if (ret == -EBUSY)
>>>    		guc->stalled_request = rq;
>>> +	else
>>> +		trace_i915_request_guc_submit(rq);
>>>    	return ret;
>>>    }
>>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>>> index d92c9f25c9f4..7f7aa096e873 100644
>>> --- a/drivers/gpu/drm/i915/i915_request.c
>>> +++ b/drivers/gpu/drm/i915/i915_request.c
>>> @@ -1344,6 +1344,9 @@ __i915_request_await_execution(struct i915_request *to,
>>>    			return err;
>>>    	}
>>> +	trace_i915_request_dep_to(to);
>>> +	trace_i915_request_dep_from(from);
>>
>> Are those two guaranteed to be atomic ie. no other dep_to/dep_from can get
>> injected in the middle of them and if so what guarantees that?
>>
> 
> These are not atomic but in practice I've never seen an out of order
> tracepoints.
>   
>> Actually we had an internal discussion going in November 2019 on these very
>> tracepoints which I think was left hanging in the air.
>>
>> There I was suggesting you create a single tracepoint in the format of "from
>> -> to", so it's clear without any doubt what is going on.
>>
> 
> Not sure if it worth adding a custom trace point fo rthis.

Custom as in not inherit from i915_request class you mean? It's not that 
hard really.

>> I also suggested this should out outside the GuC patch since it is backend
>> agnostic.
> 
> I guess, but it really matter?

IMO following best practices and established conventions matters a lot.

Regards,

Tvrtko