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 18:23, Michael Cheng wrote:
> 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

>> include/linux/cacheflush.h:12:46: warning: declaration of 'struct folio' will not be visible outside of this function [-Wvisibility]

That?

Looks like the #else path needs to forward declare struct folio or include the relevant header.

+Matthew Wilcox

Matthew, what do you think fix for this build warning on h8300 and s390 should be? Or perhaps a build environment issue with kernel test robot?

Regards,

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