Re: [Intel-gfx] [PATCH v12 1/6] drm: Add arch arm64 for drm_clflush_virt_range

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 25/02/2022 17:40, Michael Cheng wrote:
> Ah, thanks for pointing that out, when I do include it though, it causes 
> a few warning other systems such as h8300 and s390.

Errors look like? I haven't heard that kernel code is not allowed to 
include something from linux/ on some arch yet.

> Since it is already pulled is, would it be OK to leave it out for this 
> case? Or we could use something like !IS_H8300 and !IS_S390
> 
> around the header file?

Unlikely, now you made me curious why it does not work.

Regards,

Tvrtko

> 
> On 2022-02-25 9:33 a.m., Tvrtko Ursulin wrote:
>>
>> On 25/02/2022 16:52, Michael Cheng wrote:
>>> Hi Tvrtko,
>>>
>>> It seems without cacheflush.h being included, when I build for arm64 
>>> or x86, it stills pulls in cacheflush.h:
>>>
>>> ./.drm_cache.o.cmd:838: include/linux/cacheflush.h \
>>> ./.drm_cache.o.cmd:839: arch/x86/include/asm/cacheflush.h \
>>> ./.drm_cache.o.cmd:920: include/asm-generic/cacheflush.h \
>>> ./.drm_cache.o.cmd:830: include/linux/cacheflush.h \
>>> ./.drm_cache.o.cmd:831: arch/arm64/include/asm/cacheflush.h \
>>> ./.drm_cache.o.cmd:1085: include/asm-generic/cacheflush.h \
>>> So it seems without including it, cacheflush.h stills get pulled in,
>>> I think its because its a required kernel source to build the kernel
>>> per specific architecture, but please correct if I am wrong,as I am 
>>> still
>>> trying to understand how things works!
>>
>> Probably:
>>
>> drm_cache.c:
>>
>> #include <linux/highmem.h>
>>
>> linux/highmem.h:
>>
>> #include <linux/cacheflush.h>
>>
>> But it is more correct to explicitly include what you use. So if 
>> drm_cache.c uses stuff declared in cacheflush.h, it should include it.
>>
>> Regards,
>>
>> Tvrtko
>>
>>> Michael Cheng
>>> On 2022-02-25 8:28 a.m., Tvrtko Ursulin wrote:
>>>>
>>>> On 25/02/2022 03:24, Michael Cheng wrote:
>>>>> Add arm64 support for drm_clflush_virt_range. caches_clean_inval_pou
>>>>> performs a flush by first performing a clean, follow by an 
>>>>> invalidation
>>>>> operation.
>>>>>
>>>>> v2 (Michael Cheng): Use correct macro for cleaning and invalidation 
>>>>> the
>>>>>             dcache. Thanks Tvrtko for the suggestion.
>>>>>
>>>>> v3 (Michael Cheng): Replace asm/cacheflush.h with linux/cacheflush.h
>>>>>
>>>>> v4 (Michael Cheng): Arm64 does not export dcache_clean_inval_poc as a
>>>>>             symbol that could be use by other modules, thus use
>>>>>             caches_clean_inval_pou instead. Also this version
>>>>>                 removes include for cacheflush, since its already
>>>>>             included base on architecture type.
>>>>
>>>> What does it mean that it is included based on architecture type? 
>>>> Some of the other header already pulls it in?
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>> Signed-off-by: Michael Cheng <michael.cheng@intel.com>
>>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_cache.c | 5 +++++
>>>>>   1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
>>>>> index c3e6e615bf09..81c28714f930 100644
>>>>> --- a/drivers/gpu/drm/drm_cache.c
>>>>> +++ b/drivers/gpu/drm/drm_cache.c
>>>>> @@ -174,6 +174,11 @@ drm_clflush_virt_range(void *addr, unsigned 
>>>>> long length)
>>>>>         if (wbinvd_on_all_cpus())
>>>>>           pr_err("Timed out waiting for cache flush\n");
>>>>> +
>>>>> +#elif defined(CONFIG_ARM64)
>>>>> +    void *end = addr + length;
>>>>> +    caches_clean_inval_pou((unsigned long)addr, (unsigned long)end);
>>>>> +
>>>>>   #else
>>>>>       WARN_ONCE(1, "Architecture has no drm_cache.c support\n");
>>>>>   #endif