Re: [Intel-gfx] [PATCH 0/4] Drop wbinvd_on_all_cpus usage

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

On 21/03/2022 11:03, Thomas Hellström wrote:
> Hi, Tvrtko.
> 
> On 3/21/22 11:27, Tvrtko Ursulin wrote:
>>
>> On 19/03/2022 19:42, Michael Cheng wrote:
>>> To align with the discussion in [1][2], this patch series drops all 
>>> usage of
>>> wbvind_on_all_cpus within i915 by either replacing the call with certain
>>> drm clflush helpers, or reverting to a previous logic.
>>
>> AFAIU, complaint from [1] was that it is wrong to provide non x86 
>> implementations under the wbinvd_on_all_cpus name. Instead an arch 
>> agnostic helper which achieves the same effect could be created. Does 
>> Arm have such concept?
> 
> I also understand Linus' email like we shouldn't leak incoherent IO to 
> other architectures, meaning any remaining wbinvd()s should be X86 only.

The last part is completely obvious since it is a x86 instruction name.

But I think we can't pick a solution until we know how the concept maps 
to Arm and that will also include seeing how the drm_clflush_sg for Arm 
would look. Is there a range based solution, or just a big hammer there. 
If the latter, then it is no good to churn all these reverts but instead 
an arch agnostic wrapper, with a generic name, would be the way to go.

Regards,

Tvrtko

> Also, wbinvd_on_all_cpus() can become very costly, hence prefer the 
> range apis when possible if they can be verified not to degrade 
> performance.
> 
> 
>>
>> Given that the series seems to be taking a different route, avoiding 
>> the need to call wbinvd_on_all_cpus rather than what [1] suggests 
>> (note drm_clflush_sg can still call it!?), concern is that the series 
>> has a bunch of reverts and each one needs to be analyzed.
> 
> 
> Agreed.
> 
> /Thomas
> 
> 
> 
>>
>> For instance looking at just the last one, 64b95df91f44, who has 
>> looked at the locking consequences that commit describes:
>>
>> """
>>     Inside gtt_restore_mappings() we currently take the 
>> obj->resv->lock, but
>>     in the future we need to avoid taking this fs-reclaim tainted lock 
>> as we
>>     need to extend the coverage of the vm->mutex. Take advantage of the
>>     single-threaded nature of the early resume phase, and do a single
>>     wbinvd() to flush all the GTT objects en masse.
>>
>> """
>>
>> ?
>>
>> Then there are suspend and freeze reverts which presumably can regress 
>> the suspend times. Any data on those?
>>
>> Adding Matt since he was the reviewer for that work so might remember 
>> something.
>>
>> Regards,
>>
>> Tvrtko
>>
>>
>>> [1]. 
>>> https://lists.freedesktop.org/archives/dri-devel/2021-November/330928.html 
>>>
>>> [2]. https://patchwork.freedesktop.org/patch/475752/?series=99991&rev=5
>>>
>>> Michael Cheng (4):
>>>    i915/gem: drop wbinvd_on_all_cpus usage
>>>    Revert "drm/i915/gem: Almagamate clflushes on suspend"
>>>    i915/gem: Revert i915_gem_freeze to previous logic
>>>    drm/i915/gt: Revert ggtt_resume to previous logic
>>>
>>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  9 +---
>>>   drivers/gpu/drm/i915/gem/i915_gem_pm.c     | 56 ++++++++++++++--------
>>>   drivers/gpu/drm/i915/gt/intel_ggtt.c       | 17 +++----
>>>   drivers/gpu/drm/i915/gt/intel_gtt.h        |  2 +-
>>>   4 files changed, 46 insertions(+), 38 deletions(-)
>>>