Re: [PATCH 0/4] Drop wbinvd_on_all_cpus usage

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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.

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(-)
>>