Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

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

On 16/09/2022 11:30, Gupta, Anshuman wrote:
>> -----Original Message-----
>> From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Sent: Thursday, September 15, 2022 10:37 PM
>> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Cc: Auld, Matthew <matthew.auld@intel.com>; Vivi, Rodrigo
>> <rodrigo.vivi@intel.com>
>> Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against
>> rpm
>>
>>
>> On 15/09/2022 11:33, Anshuman Gupta wrote:
>>> If i915 gem obj lies in lmem, then i915_gem_object_pin_map need to
>>> grab a rpm wakeref to make sure gfx PCIe endpoint function stays in D0
>>> state during any access to mapping returned by
>>> i915_gem_object_pin_map().
>>> Subsequently i915_gem_object_upin_map will put the wakref as well.
>>
>> Another thing to check are perma pinned contexts. Follow the flow from
>> intel_engine_create_pinned_context to intel_engine_destroy_pinned_context.
>> If you find out that kernel (&co) contexts are pinned for the duration of i915
>> load/bind and that they use lmem objects, that would mean wakeref is held for
>> the duration of i915 loaded state. Defeating the point and making the solution
>> effectively equal to just disabling RPM.
> That’s correct  intel_ring_pin can pin_map the lmem object.
>        if (i915_vma_is_map_and_fenceable(vma)) {
>                  addr = (void __force *)i915_vma_pin_iomap(vma);
>          } else {
>                  int type = i915_coherent_map_type(vma->vm->i915, vma->obj, false);
> 
>                  addr = i915_gem_object_pin_map(vma->obj, type);
>          }
> 
> If that is the case this RFC proposal will not work and in that case 

Right, or LRC state for perma pinned contexts is probably even more 
clear cut.

every caller of  i915_gem_object_pin_map need to grab the wakreref before
> accessing the retuned address by pin_map. Any inputs from you side for any other approach.

I didn't quite get what you meant here.

My point was that if my thinking that perma pinned contexts would hold 
the wakeref permanently is correct, that would prevent the approach from 
this patch to have a different effect from just disabling RPM. Would 
unpinning the perma pinned contexts on GT park be feasible? It's not a 5 
minute question and unfortunately I don't have enough time to go deep 
into this problem space. Like Hopefully someone else can jump in.

Regards,

Tvrtko