Re: [Intel-gfx] [PATCH 18/24] drm/i915: Convert i915_perf to ww locking as well

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 12-08-2020 om 21:53 schreef Thomas Hellström (Intel):
>
> On 8/10/20 12:30 PM, Maarten Lankhorst wrote:
>> We have the ordering of timeline->mutex vs resv_lock wrong,
>> convert the i915_pin_vma and intel_context_pin as well to
>> future-proof this.
>>
>> We may need to do future changes to do this more transaction-like,
>> and only get down to a single i915_gem_ww_ctx, but for now this
>> should work.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_perf.c | 57 +++++++++++++++++++++++---------
>>   1 file changed, 42 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index c6f6370283cf..e94976976571 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -1195,24 +1195,39 @@ static struct intel_context *oa_pin_context(struct i915_perf_stream *stream)
>>       struct i915_gem_engines_iter it;
>>       struct i915_gem_context *ctx = stream->ctx;
>>       struct intel_context *ce;
>> -    int err;
>> +    struct i915_gem_ww_ctx ww;
>> +    int err = -ENODEV;
>>         for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
>>           if (ce->engine != stream->engine) /* first match! */
>>               continue;
>>   -        /*
>> -         * As the ID is the gtt offset of the context's vma we
>> -         * pin the vma to ensure the ID remains fixed.
>> -         */
>> -        err = intel_context_pin(ce);
>> -        if (err == 0) {
>> -            stream->pinned_ctx = ce;
>> -            break;
>> -        }
>> +        err = 0;
>> +        break;
>>       }
>>       i915_gem_context_unlock_engines(ctx);
>>   +    if (err)
>> +        return ERR_PTR(err);
>> +
>> +    i915_gem_ww_ctx_init(&ww, true);
>> +retry:
>> +    /*
>> +     * As the ID is the gtt offset of the context's vma we
>> +     * pin the vma to ensure the ID remains fixed.
>> +     */
>> +    err = intel_context_pin_ww(ce, &ww);
>> +    if (err == -EDEADLK) {
>> +        err = i915_gem_ww_ctx_backoff(&ww);
>> +        if (!err)
>> +            goto retry;
>> +    }
>> +    i915_gem_ww_ctx_fini(&ww);
>> +
>
> Hmm. Didn't we keep an intel_context_pin() that does exactly the above without recoding the whole ww transaction? Or do you plan to remove that?
>
> With that taken into account,
>
> Reviewed-by: Thomas Hellström <thomas.hellstrom@intel.com>
>
>
Yeah, I want to remove that eventually, might need to change i915_perf even more to fully do this. Thanks for reviewing.

~Maarten

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