[Intel-gfx] [PATCH] drm/i915: Unlock the shared hwsp_gtt object after pinning

[Thomas Hellström (Intel)]

On 9/3/20 4:01 PM, Chris Wilson wrote:
> Quoting Mika Kuoppala (2020-09-03 14:31:44)
>> From: Thomas Hellström <thomas.hellstrom at intel.com>
>>
>> The hwsp_gtt object is used for sub-allocation and could therefore
>> be shared by many contexts causing unnecessary contention during
>> concurrent context pinning.
>> However since we're currently locking it only for pinning, it remains
>> resident until we unpin it, and therefore it's safe to drop the
>> lock early, allowing for concurrent thread access.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom at intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_context.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
>> index 61b05cd4c47a..65e956ba19e1 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_context.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
>> @@ -271,6 +271,13 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
>>          i915_active_release(&ce->active);
>>   err_ctx_unpin:
>>          intel_context_post_unpin(ce);
>> +
>> +       /*
>> +        * Unlock the hwsp_ggtt object since it's shared. This is fine for now
>> +        * since the lock has been used for pinning only, not fencing.
>> +        */
>> +       i915_gem_ww_unlock_single(ce->timeline->hwsp_ggtt->obj);
> The timeline is not special here, the same rules for locking/unlock can
> equally be applied to all the global state. You may even go as far as
> putting a local acquire context here, which would then have avoided the
> cross driver pollution.
>
> Anyway, if it works for the timeline, there is no reason to keep the
> other globals locked. They are pinned and on completely different usage
> cycles to the user objects.

Agreed.  In principle everything we want to perma-pin we can unlock 
early, Keeping it under the same acquire context makes it possible to do 
this as part of a bigger ww transaction. Although initially this 
solution was discarded in favour of removing the suballocation to be 
able to ditch the perma-pinning in the future.

But is this acceptable to address the pi-isolated issue until we have 
something more thought-through in place? Or is there somethin specific 
you want me to change for a R-B?

Thanks,

Thomas