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

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

Hi,

On Mon, 2022-03-21 at 13:12 +0000, Tvrtko Ursulin wrote:
> 
> On 21/03/2022 12:33, Thomas Hellström wrote:
> > On Mon, 2022-03-21 at 12:22 +0000, Tvrtko Ursulin wrote:
> > > 
> > > 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.
> > 
> > Yeah, I meant the function implementing wbinvd() semantics.
> > 
> > > 
> > > 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.
> > 
> > But my impression was that ARM would not need the range-based
> > interface
> > either, because ARM is only for discrete and with discrete we're
> > always
> > coherent.
> 
> Not sure what you mean here - what about flushing system memory objects
> on discrete? Those still need flushing on paths like suspend which this
> series touches. Am I missing something?

System bos on discrete should always have

I915_BO_CACHE_COHERENT_FOR_READ | I915_BO_CACHE_COHERENT_FOR_WRITE

either by the gpu being fully cache coherent (or us mapping system
write-combined). Hence no need for cache clflushes or wbinvd() for
incoherent IO.

That's adhering to Linus'

"And I sincerely hope to the gods that no cache-incoherent i915 mess
ever makes it out of the x86 world. Incoherent IO was always a
historical mistake and should never ever happen again, so we should
not spread that horrific pattern around."


/Thomas