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

[Michael Cheng <michael.cheng@intel.com>]

+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@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
>>>
>>>