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

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

These seem to be pretty old arch and are day0 warnings, please refer to 
[1] to see the warnings. Also I am not sure why my patch series didn't 
append to the old one.

[1] https://patchwork.freedesktop.org/patch/475829/?series=99450&rev=11

2022-02-25 10:19 a.m., Tvrtko Ursulin wrote:
>
> 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