[Intel-gfx] [PATCH v7 3/8] drm/i915: Add per context timelines to fence object

[John Harrison]

On 20/04/2016 18:44, Chris Wilson wrote:
> On Wed, Apr 20, 2016 at 06:09:50PM +0100, John.C.Harrison at Intel.com wrote:
>> From: John Harrison <John.C.Harrison at Intel.com>
>>
>> The fence object used inside the request structure requires a sequence
>> number. Although this is not used by the i915 driver itself, it could
>> potentially be used by non-i915 code if the fence is passed outside of
>> the driver. This is the intention as it allows external kernel drivers
>> and user applications to wait on batch buffer completion
>> asynchronously via the dma-buff fence API.
>>
>> To ensure that such external users are not confused by strange things
>> happening with the seqno, this patch adds in a per context timeline
>> that can provide a guaranteed in-order seqno value for the fence. This
>> is safe because the scheduler will not re-order batch buffers within a
>> context - they are considered to be mutually dependent.
>>
>> v2: New patch in series.
>>
>> v3: Renamed/retyped timeline structure fields after review comments by
>> Tvrtko Ursulin.
>>
>> Added context information to the timeline's name string for better
>> identification in debugfs output.
>>
>> v5: Line wrapping and other white space fixes to keep style checker
>> happy.
>>
>> v7: Updated to newer nightly (lots of ring -> engine renaming).
>>
>> For: VIZ-5190
>> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h         | 25 +++++++---
>>   drivers/gpu/drm/i915/i915_gem.c         | 83 +++++++++++++++++++++++++++++----
>>   drivers/gpu/drm/i915/i915_gem_context.c | 14 ++++++
>>   drivers/gpu/drm/i915/intel_lrc.c        |  8 ++++
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  1 -
>>   5 files changed, 114 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index bb18b89..1c3a1ca 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -813,6 +813,15 @@ struct i915_ctx_hang_stats {
>>   	bool banned;
>>   };
>>   
>> +struct i915_fence_timeline {
>> +	char        name[32];
>> +	unsigned    fence_context;
>> +	unsigned    next;
>> +
>> +	struct intel_context *ctx;
>> +	struct intel_engine_cs *engine;
>> +};
>> +
>>   /* This must match up with the value previously used for execbuf2.rsvd1. */
>>   #define DEFAULT_CONTEXT_HANDLE 0
>>   
>> @@ -860,6 +869,7 @@ struct intel_context {
>>   		struct i915_vma *lrc_vma;
>>   		u64 lrc_desc;
>>   		uint32_t *lrc_reg_state;
>> +		struct i915_fence_timeline fence_timeline;
> This is wrong. The timeline is actually a property of the vm, with
> contexts for each engine.
> -Chris
>

I don't get what you mean. The timeline does not have contexts. It is 
just an incrementing number. It could possible be shared between all 
engines of a single software context rather than be specific to the 
hardware context. Although I think it is safer and more future proof to 
be the latter, i.e. per engine per s/w context. I don't see where the vm 
comes into it.