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

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

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