[Intel-gfx] [PATCH v3 2/3] drm/i915/gem: Remove logic for wbinvd_on_all_cpus

[Thomas Hellström (Intel)]

Hi, Michael, others.

This is the response from Linus last time we copied that already 
pre-existing wbinvd_on_all_cpus() macro to another place in the driver:

https://lists.freedesktop.org/archives/dri-devel/2021-November/330928.html

My first interpretation of this is that even if there currently are 
similar patterns in drm_cache.c, we shouldn't introduce more, 
encouraging other drivers to use incoherent IO.

Other than that I think we should move whatever wbinvds we can over to 
the ranged versions, unless that is proven to be a performance drop.

Finally for any wbinvds left in our driver, ensure that they are never 
executed for any gpu where we provide full coherency. That is all 
discrete gpus (and to be discussed integrated gpus moving forward).

Might be that drm maintainers want to chime in here with other views.

Thanks,

Thomas



On 3/15/22 17:59, Michael Cheng wrote:
> +Daniel for additional feedback!
>
> On 2022-03-14 4:06 p.m., Michael Cheng wrote:
>
>> On 2022-03-08 10:58 a.m., Lucas De Marchi wrote:
>>
>>> On Tue, Feb 22, 2022 at 08:24:31PM +0100, Thomas Hellström (Intel) 
>>> wrote:
>>>> Hi, Michael,
>>>>
>>>> On 2/22/22 18:26, Michael Cheng wrote:
>>>>> This patch removes logic for wbinvd_on_all_cpus and brings in
>>>>> drm_cache.h. This header has the logic that outputs a warning
>>>>> when wbinvd_on_all_cpus when its being used on a non-x86 platform.
>>>>>
>>>>> Signed-off-by: Michael Cheng <michael.cheng at intel.com>
>>>>
>>>> Linus has been pretty clear that he won't accept patches that add 
>>>> macros that works on one arch and warns on others anymore in i915 
>>>> and I figure even less so in drm code.
>>>>
>>>> So we shouldn't try to move this out to drm. Instead we should 
>>>> restrict the wbinvd() inside our driver to integrated and X86 only. 
>>>> For discrete on all architectures we should be coherent and hence 
>>>> not be needing wbinvd().
>>>
>>> the warn is there to guarantee we don't forget a code path. However
>>> simply adding the warning is the real issue: we should rather guarantee
>>> we can't take that code path. I.e., as you said refactor the code to
>>> guarantee it works on discrete without that logic.
>>>
>>>     $ git grep wbinvd_on_all_cpus -- drivers/gpu/drm/
>>>     drivers/gpu/drm/drm_cache.c:    if (wbinvd_on_all_cpus())
>>>     drivers/gpu/drm/drm_cache.c:    if (wbinvd_on_all_cpus())
>>>     drivers/gpu/drm/drm_cache.c:    if (wbinvd_on_all_cpus())
>>>
>>>     drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c:      * Currently we 
>>> just do a heavy handed wbinvd_on_all_cpus() here since
>>>     drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: wbinvd_on_all_cpus();
>>>
>>> It looks like we actually go through this on other discrete 
>>> graphics. Is
>>> this missing an update like s/IS_DG1/IS_DGFX/? Or should we be doing
>>> something else?
>>>
>>>     drivers/gpu/drm/i915/gem/i915_gem_pm.c:#define 
>>> wbinvd_on_all_cpus() \
>>>     drivers/gpu/drm/i915/gem/i915_gem_pm.c: wbinvd_on_all_cpus();
>>>
>>> Those are for suspend. Revert ac05a22cd07a ("drm/i915/gem: 
>>> Almagamate clflushes on suspend")
>>> or extract that part to a helper function and implement it differently
>>> for arches != x86?
>>>
>>>     drivers/gpu/drm/i915/gem/i915_gem_pm.c: wbinvd_on_all_cpus();
>>>
>>> Probably take a similar approach to the suspend case?
>>>
>>>     drivers/gpu/drm/i915/gt/intel_ggtt.c: wbinvd_on_all_cpus();
>>
>> For a helper function, I have a #define for all non x86 architecture 
>> that gives a warn on [1] within drm_cache.h Or would it be better to 
>> implement a helper function instead?
>>
>> [1]. https://patchwork.freedesktop.org/patch/475750/?series=99991&rev=5
>>
>>>
>>> This one comes from 64b95df91f44 ("drm/i915: Assume exclusive access 
>>> to objects inside resume")
>>> Shouldn't that be doing the invalidate if the write domain is 
>>> I915_GEM_DOMAIN_CPU
>>>
>>> In the end I think the warning would be ok if it was the cherry on top,
>>> to guarantee we don't take those paths. We should probably have a
>>> warn_once() to avoid spamming the console. But we  also have to rework
>>> the code to guarantee we are the only ones who may eventually get that
>>> warning, and not the end user.
>> Could we first add the helper function/#define for now, and rework 
>> the code in a different patch series?
>>>
>>> Lucas De Marchi
>>>
>>>>
>>>> Thanks,
>>>>
>>>> /Thomas
>>>>
>>>>